-
Notifications
You must be signed in to change notification settings - Fork 422
refactor: Break samplers into classes and store them on agent.sampler.*
#3527
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
Conversation
agent.sampler.*agent.sampler.*
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3527 +/- ##
==========================================
+ Coverage 81.48% 89.49% +8.01%
==========================================
Files 411 425 +14
Lines 54553 55903 +1350
Branches 1 1
==========================================
+ Hits 44455 50033 +5578
+ Misses 10098 5870 -4228
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lib/samplers/README.md
Outdated
| * `agent.sampler.partial_granularity.remote_parent_sampled` | ||
| * `agent.sampler.partial_granularity.remote_parent_not_sampled` | ||
|
|
||
| Will be deprecated, `full_granularity` takes precedence: |
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.
This hasn't been decided yet. The spec says to use both but agent.sampler.full_granularity.* takes precedence over agent.sampler.*.
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.
When I mean "deprecated", I'm referring to the agent.sampler.root, agent.sampler.remoteParentSampled, and agent.sampler.remoteParentNotSampled fields on the agent, not the config. In the future, it will be agent.sampler.fullGranularity.root, etc., which assumes the precedence logic of sampler.fullGranularity > sampler handled by the config initialization.
lib/samplers/README.md
Outdated
| ```javascript | ||
| ... | ||
| // Decide sampling from w3c data | ||
| let full_sampler = null |
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.
Should we make these camel case since that's the preferred way?
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.
This is more pseudo-codey than anything else, but I can make that change
| }) | ||
| this.config.on('sampling_target', function updateSamplingTarget(target) { | ||
| self.transactionSampler.samplingTarget = target | ||
| self.sampler.root.samplingTarget = target |
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.
So the transactionSampler is always the root sampler?
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.
Yes, it was.
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.
Though this updateSamplingTarget function does reveal another thing we will have to resolve with the sharing of AdaptiveSampler states, but that's out of scope for this PR.
14a07ab to
7d0d13b
Compare
Description
Fixes some sampling-related bugs by better encapsulating
remote_parent_sampledandremote_parent_not_sampledsampling decisions withagent.sampler.remoteParentSampledandagent.sampler.remoteParentNotSampled. Also, replacesagent.transactionSamplerwithagent.sampler.root, so quite a few tests were modified to account for this.Introduces the concept of a
Samplerinterface with exposesapplySamplingDecision, which updates a givenTransaction'spriorityandsampledflags.How to Test
Related Issues
Precursor for core tracing and
TraceIdRatioBasedSamplerwork.