-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Simplify and make sample rand docs more specific #12327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
jan-auer
approved these changes
Jan 14, 2025
| The random value is set according to the following rules: | ||
| 1. When tracing is enabled and a new trace is started, the SDK generates a `sample_rand` value for the current execution context. A `sample_rand` is a float (`0.1234` notation) in the range of `[0, 1)` (including 0.0, excluding 1.0). | ||
| 1. It is _recommended_ to generate the random number deterministically using the trace ID as seed or source of randomness. The exact method by which the random number is created is implementation defined and may vary between SDK implementations. | ||
| 1. The `sample_rand` is part of the DSC (Dynamic Sampling Context) and as with other values on the `baggage` header, the `sentry-sample_rand` value from the current execution context should be propagated to downstream SDKs. |
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
| 1. The `sample_rand` is part of the DSC (Dynamic Sampling Context) and as with other values on the `baggage` header, the `sentry-sample_rand` value from the current execution context should be propagated to downstream SDKs. | |
| 1. The `sample_rand` is part of the DSC (Dynamic Sampling Context) and as with other values on the `baggage` header, the `sample_rand` value from the current execution context should be propagated to downstream SDKs. |
| The SDK should always use the stored random number (`sentry-sample_rand`) for sampling decisions and should no longer rely on `math.random()` or similar functions in tracing code: | ||
| 1. When the `tracesSampler` is invoked, the return value should be compared with `sample_rand`: `trace["sentry-sample_rand"] < tracesSampler(context)` | ||
| 1. When the SDK is the head of a trace (and `tracesSampleRate` is defined), the sampling is done comparing the `tracesSampleRate` with the `sample_rand`: `trace["sentry-sample_rand"] < config.tracesSampleRate` | ||
| 1. If no `tracesSampler` callback is configured, the SDK fully inherits incoming sampling decisions from the `sentry-trace` header for propagated traces. |
Member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested change
| 1. If no `tracesSampler` callback is configured, the SDK fully inherits incoming sampling decisions from the `sentry-trace` header for propagated traces. | |
| 1. Otherwise, there is an incoming trace and the SDK fully inherits incoming sampling decisions from the `sentry-trace` header for propagated traces. |
You might also turn 2 and 3 around for clarity to say:
- if tracesSampler, ...
- if there's an incoming trace, ...
- otherwise, the SDK is the head of the trace, ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adjusts the dev docs on sample_rand to require 30 less IQ points when reading. I used myself (borderline dyslexic) as a benchmark.