ref: Unify TracesSampler #498
Merged
Conversation
This file contains 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
4 tasks
cleptric
commented
Nov 28, 2022
cleptric
commented
Nov 28, 2022
AbhiPrasad
reviewed
Nov 29, 2022
AbhiPrasad
approved these changes
Nov 29, 2022
| @@ -1,5 +1,7 @@ | |||
| # Changelog | |||
|
|
|||
| - *[breaking]* ref: Unify TracesSampler (#444) | |||
There was a problem hiding this comment.
Yep, this is just a reminder for me when doing the release. I’ll try to match the Ruby standard going forward in Go
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In order to implement Dynamic Sampling, I had to make changes to how the
TracesSamplerworks in the SDK.I'm aware that this is a breaking change, but it is required at this point.
For Dynamic Sampling, a
sample_rateitem needs to be present in thetraceenvelope header.With the current implementation, accessing these values was not possible.
TracesSamplerto always return afloat64. This is in line with the implementation of all other SDKs and https://develop.sentry.dev/sdk/performance/#tracessamplersamplemethod quite verbose, ifdebug: true, as this is a constant pain point for developers to figure out what is happening.TracesSampleRate: 0.0/nil, I introduced a newClientOptioncalledEnableTracing, which turns off tracing completely by default. This setting is required, as we need a way to allow developers to turn off sampling that could be started by asentry-traceheader with atruesampling decision. I did consider using a pointer, but this was too clunky.Open questions:
The old and new implementations would also sample spans if theI missed this when making my changes. Not an issue anymore, I kept the old logic in place.TracesSampleroption is set. I hence would vote for only callingsample()on transactions. @AbhiPrasad Do I miss the use-case of sampling spans separately to a transaction?