Skip to content

[APMAPI-1258] Adjust trace sampling formula #4616

Merged
vpellan merged 8 commits into
masterfrom
ben.db/APMAPI-1258-adjust-sampling-formula
May 2, 2025
Merged

[APMAPI-1258] Adjust trace sampling formula #4616
vpellan merged 8 commits into
masterfrom
ben.db/APMAPI-1258-adjust-sampling-formula

Conversation

@genesor

@genesor genesor commented Apr 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

Adjust the knuth sampling formula to have the same one across every tracer.

Motivation:

There is slight differences between every tracer in the implementation of this formula. The Ruby one was correct but the modulo was based on the EXTERNAL_MAX_ID value that was wrong (off by one)

Change log entry

Yes. Cosmetic change on the knuth sampling formula to have a clear separation between uint64 modulo in a new const UINT64_MODULO (1<<64) and MAX_EXTERNAL_ID ((1<<64) -1)

Additional Notes:

How to test the change?

  • Test added in rate_sampler_spec
  • No impact on current tracer behavior

@genesor
genesor requested a review from vpellan April 30, 2025 12:58
@codecov-commenter

codecov-commenter commented Apr 30, 2025 •
edited by codecov Bot
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.74%. Comparing base (56a587c) to head (e39bd7a).
⚠️ Report is 2755 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4616      +/-   ##
==========================================
- Coverage   97.76%   97.74%   -0.03%     
==========================================
  Files        1415     1415              
  Lines       86264    86275      +11     
  Branches     4348     4349       +1     
==========================================
- Hits        84339    84328      -11     
- Misses       1925     1947      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Apr 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Datadog Report

Branch report: ben.db/APMAPI-1258-adjust-sampling-formula
Commit report: e39bd7a
Test service: dd-trace-rb

✅ 0 Failed, 21254 Passed, 1373 Skipped, 3m 37.28s Total Time

vpellan
vpellan approved these changes Apr 30, 2025
View reviewed changes

@vpellan vpellan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment but except the steep error, LGTM !

Comment thread spec/datadog/tracing/sampling/rate_sampler_spec.rb Outdated
@genesor
genesor marked this pull request as ready for review April 30, 2025 16:33
@genesor
genesor requested a review from a team as a code owner April 30, 2025 16:33
@pr-commenter

pr-commenter Bot commented Apr 30, 2025 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2025-05-01 21:10:27

Comparing candidate commit e39bd7a in PR branch ben.db/APMAPI-1258-adjust-sampling-formula with baseline commit 56a587c in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 31 metrics, 2 unstable metrics.

p-datadog
p-datadog approved these changes Apr 30, 2025
View reviewed changes

@p-datadog p-datadog left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved but this PR as far as I can tell does not change the modulo and hence makes no changes to sampling algorithm, it only rejects 1<<64 as a trace id (externally supplied?).

@genesor

genesor commented Apr 30, 2025

Copy link
Copy Markdown
Member Author

Approved but this PR as far as I can tell does not change the modulo and hence makes no changes to sampling algorithm, it only rejects 1<<64 as a trace id (externally supplied?).

Yes @p-datadog, there is no change in the modulo or the formula itself other than the cosmetic one of separating the modulo and MAX_EXTERNAL_ID.

The old value of MAX_EXTERNAL_ID was an uint65 instead of uint64 (hence the -1)

@p-datadog

p-datadog commented May 1, 2025

Copy link
Copy Markdown
Member

@genesor could you update the changelog entry in the PR description with this please?

@p-datadog

p-datadog commented May 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Does the 1<<64 case whose behavior is changed by the PR need test coverage?

@genesor

genesor commented May 2, 2025

Copy link
Copy Markdown
Member Author

Does the 1<<64 case whose behavior is changed by the PR need test coverage?

@p-datadog I'd say no, this is not an ID we can produce in other languages as it is bound to the limit of uint64 with the value 1<<64 -1

@vpellan
vpellan merged commit d960f15 into master May 2, 2025
@vpellan
vpellan deleted the ben.db/APMAPI-1258-adjust-sampling-formula branch May 2, 2025 15:38
@github-actions github-actions Bot added this to the 2.16.0 milestone May 2, 2025
@Strech Strech mentioned this pull request May 19, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vpellan vpellan vpellan approved these changes
@p-datadog p-datadog p-datadog approved these changes

Assignees

No one assigned

Labels

tracing

Projects

None yet

Milestone

2.16.0

Development

Successfully merging this pull request may close these issues.

5 participants

@genesor @codecov-commenter @p-datadog @vpellan @p