Uh oh!
There was an error while loading. Please reload this page.
feat(opentelemetry): Expose sampling helper - #12674
Merged
Merged
Conversation
Contributor
size-limit report 📦
|
tnolet
commented
Jul 2, 2024
@mydea do you have any idea when this patch PR might land in the main branch and be published? |
mydea
commented
Jul 2, 2024
MemberAuthor
Once this is reviewed and merged, it will go into the next release - probably later this week or early next week it should be out! |
| SentrySampler, | ||
| sentrySamplerNoDecision, | ||
| sentrySamplerNotSampled, | ||
| sentrySamplerSampled, |
Contributor
There was a problem hiding this comment.
m: Couldn't we expose a single method that takes in SamplingDecision from @opentelemetry/sdk-trace-base or similar?
sentrySampler({
decision, // SamplingDecision enum
context,
spanAttributes,
})
It feels like a lot of overhead for us to export three different methods, and to me it feels better to expose the underlying enum being used.
MemberAuthor
There was a problem hiding this comment.
that sounds like a good idea to me! 👍 will adjust this.
When users want to use a custom sampler, they can use these new helpers to still have sentry working nicely with whatever they decide to do in there. For e.g. trace propagation etc. to work correctly with Sentry, we need to attach some things to trace state etc. These helpers encapsulate this for the user, while still allowing them to decide however they want if the span should be sampled or not.
mydea
commented
Jul 2, 2024
MemberAuthor
Updated this to instead only export returnwrapSamplingDecision({decision: SamplingDecision.NOT_RECORD, context, spanAttributes }); |
AbhiPrasad
approved these changes
Jul 2, 2024
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
When users want to use a custom sampler, they can use these new helpers to still have sentry working nicely with whatever they decide to do in there.
For e.g. trace propagation etc. to work correctly with Sentry, we need to attach some things to trace state etc. These helpers encapsulate this for the user, while still allowing them to decide however they want if the span should be sampled or not.
This was brought up here: #12191 (reply in thread)