-
Notifications
You must be signed in to change notification settings - Fork 893
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
Revert "Add InstrumentationLibrary to Sampler.ShouldSample (#1850)" #1942
Revert "Add InstrumentationLibrary to Sampler.ShouldSample (#1850)" #1942
Conversation
…metry#1850)" This reverts commit f936f2e.
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.
As discussed during today Maintainers meeting.
Please provide a more detailed justification for those that did not attend (feel free to dismiss my review after that)
+1. Do we need to improve our community guidelines to make it clear that "was discussed in a meeting" is never a justification for any change? |
Sorry, updated PR's description |
Please ensure a follow-up issue for this new configuration design is created before merging. |
@Oberon00 I'm not sure what you mean here. Can you clarify? I'd like to understand what you'd like to see before moving this PR forward. |
Quoting the description:
There should be an issue for that. Probably it will be best to use the existing #1588, copying & pasting the gist to that issuewould make sense. And of course, adding a "Closes #1588" to the description to close the PR for adding resources, which then wouldn't make sense at all. |
Added #1588 (comment) |
@Oberon00 Wanted to double confirm you are fine with Nikita's comment, and we can merge this one? |
…metry#1850)" (open-telemetry#1942) This reverts commit f936f2e. Co-authored-by: Bogdan Drutu <[email protected]>
This reverts commit f936f2e.
During today Maintainers meeting @jmacd expressed his concern about #1658 and the direction that it moves SDK: runtime decisions in
Samplers
as opposed to configuration-time decisions of configuring SDK and/orTracers
with the correctSamplers
. His opinion was that we should use the latter: all decisions based onResources
andInstrumentationLibraries
can be done before providingTracers
to the users. Thus thoseTracers
can already useSamplers
chosen/narrowed down based on that information. Thus they don't need to accept it inShouldSample
method.Several attendees, myself including, agreed that this is a reasonable idea. It was thus decided to revert introducing
InstrumentationLibrary
toShouldSample
method and get back to the drawing board to choose between two approaches:either pass both
Resources
andInstrumentationLibrary
toShouldSample
in one spec release OR don't pass neither of them and allow SDKs to configure specificSampler
for a specificTracer
.