Add InstrumentationLibrary to Sampler.ShouldSample#1850
Add InstrumentationLibrary to Sampler.ShouldSample#1850carlosalberto merged 4 commits intoopen-telemetry:mainfrom
Conversation
anuraaga
left a comment
There was a problem hiding this comment.
This change LGTM. I would suggest not rushing it into this week's release if possible, mostly since if this goes in, I don't see any reason we can't add Resource in the same way
It would be good to aim to make such changes in the same release to reduce churn especially when taking into account backwards compatibility.
I am afraid it will not be that easy with Resources, as demonstrated by discussions in #1658 |
|
@iNikem We are ready to roll, just add a CHANGELOG entry ;) |
There was a problem hiding this comment.
It just occurred to me that the complicated design that @bogdandrutu suggested on #1658 is only 50% due to Resources. The InstrumentationLibrary would already not be provided through a simple argument but instead we should have
Have a "SamplerProvider" with a "getSampler(InstrumentationLibrary).
So while I think that this PR is a step in the right direction, I see no reason not to add a Resource as well, and I prefer adding these at once.
EDIT: Just to clarify: I'm against the more complicated design suggested in the linked thread and think the simple "design" in this PR or my #1658 are perfectly adequate. I just think that there is no reason not to add Resources as well.
|
If you have an argument for why Resources cannot be passed to ShouldSample directly while InstrumentationLibrary can, please say so and I will gladly re-approve. |
|
@Oberon00 As you saw yourself in #1658, there was a lot pushback for passing |
Changelog updated |
I got similar pushback for InstrumentationLibrary. Maybe it's more of a question who is currently on vacation. InstrumentationLibrary is static information per Tracer too, whereas Resource is static per TracerProvider. |
I don't really want to block it. I brought up my arguments.
|
@Oberon00 Consider having a PR to add |
|
Merging - thanks @Oberon00 for unblocking this (and hopefully we can get the remaining portion this month!) |
…metry#1850)" This reverts commit f936f2e.
…metry#1850)" (open-telemetry#1942) This reverts commit f936f2e. Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com>
…pen-telemetry#1850) Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com> Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
Partially addresses #1588. Picks up part of the previous PR #1658
Changes
Add
InstrumentationLibrarytoSampler.ShouldSampleAlternatives
First,
InstrumentationLibrarycould be added as an optional, not required, argument toShouldSample. This change may be easier to implement in backward compatible manner. But I don't think it will make a significant difference.Second, as was proposed for
Resourcein #1658, we may avoid passingInstrumentationLibraryto eachShouldSamplecall by associating it withSampler. That would mean having a separateSamplerinstance for everyInstrumentationLibraryeither by having those instances associated withTraceror by lettingTracerProviderto have a registry/lookup table betweenInstrumentationLibraryandSampler. Unfortunately, current state of the spec makes this solution unfeasible for the following reasons.SDK spec says (emphasis is mine):
But the more important obstacle is the possibility to update the configuration of the
TracerProvider, including updating the sampler. In this case all already existingTracershave to start using the newSampler. This makes the option of associatingSamplerwith theTracertotally unfeasible. The option of havingSamplerregistry inTracerProviderwould work but would requireTracerto lookup itsSamplereach time it has to make sampling decision. This will lead to worse performance that passingInstrumentationLibrarytoSampler.ShouldSample.