Add InstrumentationLibrary to Sampler.ShouldSample

[iNikem]

Partially addresses #1588. Picks up part of the previous PR #1658

Changes

Add InstrumentationLibrary to Sampler.ShouldSample

Alternatives

First, InstrumentationLibrary could be added as an optional, not required, argument to ShouldSample. 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 Resource in #1658, we may avoid passing InstrumentationLibrary to each ShouldSample call by associating it with Sampler. That would mean having a separate Sampler instance for every InstrumentationLibrary either by having those instances associated with Tracer or by letting TracerProvider to have a registry/lookup table between InstrumentationLibrary and Sampler. Unfortunately, current state of the spec makes this solution unfeasible for the following reasons.

SDK spec says (emphasis is mine):

The SDK defines the interface Sampler as well as a set of built-in samplers and associates a Sampler with each TracerProvider

But the more important obstacle is the possibility to update the configuration of the TracerProvider, including updating the sampler. In this case all already existing Tracers have to start using the new Sampler. This makes the option of associating Sampler with the Tracer totally unfeasible. The option of having Sampler registry in TracerProvider would work but would require Tracer to lookup its Sampler each time it has to make sampling decision. This will lead to worse performance that passing InstrumentationLibrary to Sampler.ShouldSample.

[anuraaga]

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

#1588 (comment)

It would be good to aim to make such changes in the same release to reduce churn especially when taking into account backwards compatibility.

[iNikem]

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

#1588 (comment)

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

[carlosalberto]

@iNikem We are ready to roll, just add a CHANGELOG entry ;)

[Oberon00]

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).

#1658 (comment)

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.

[Oberon00]

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.

[iNikem]

@Oberon00 As you saw yourself in #1658, there was a lot pushback for passing Resource into ShouldSample, presumably because it was deemed a static value. As I wrote in #1588 (comment), I am not ready for the second round of exactly the same discussions. This PR already got an adequate number of approvals and not a single objection so far - stark contrast with #1658. I don't want to risk that.

[iNikem]

@iNikem We are ready to roll, just add a CHANGELOG entry ;)

Changelog updated

[Oberon00]

As you saw yourself in #1658, there was a lot pushback for passing Resource into ShouldSample

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.

[carlosalberto]

@Oberon00 Consider having a PR to add Resource as we have the entire month to try to have both added for the September 1.17.0 release.

[carlosalberto]

Merging - thanks @Oberon00 for unblocking this (and hopefully we can get the remaining portion this month!)