Skip to content

Revert "Add InstrumentationLibrary to Sampler.ShouldSample (#1850)"#1942

Merged
jsuereth merged 2 commits intoopen-telemetry:mainfrom
iNikem:revert-instrumentation-library
Sep 22, 2021
Merged

Revert "Add InstrumentationLibrary to Sampler.ShouldSample (#1850)"#1942
jsuereth merged 2 commits intoopen-telemetry:mainfrom
iNikem:revert-instrumentation-library

Conversation

@iNikem
Copy link
Copy Markdown
Contributor

@iNikem iNikem commented Sep 20, 2021 •
edited
Loading

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/or Tracers with the correct Samplers. His opinion was that we should use the latter: all decisions based on Resources and InstrumentationLibraries can be done before providing Tracers to the users. Thus those Tracers can already use Samplers chosen/narrowed down based on that information. Thus they don't need to accept it in ShouldSample method.

Several attendees, myself including, agreed that this is a reasonable idea. It was thus decided to revert introducing InstrumentationLibrary to ShouldSample method and get back to the drawing board to choose between two approaches:
either pass both Resources and InstrumentationLibrary to ShouldSample in one spec release OR don't pass neither of them and allow SDKs to configure specific Sampler for a specific Tracer.

@iNikem iNikem requested review from a team September 20, 2021 16:46
Oberon00
Oberon00 previously requested changes Sep 20, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Oberon00 Oberon00 left a comment •
edited
Loading

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)

@yurishkuro
Copy link
Copy Markdown
Member

yurishkuro commented Sep 20, 2021 •
edited
Loading

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?

@Oberon00 Oberon00 added area:sampling Related to trace sampling area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Sep 20, 2021
@iNikem
Copy link
Copy Markdown
Contributor Author

iNikem commented Sep 20, 2021

Sorry, updated PR's description

@iNikem iNikem dismissed Oberon00’s stale review September 20, 2021 17:31

Concern addressed

@Oberon00
Copy link
Copy Markdown
Member

Oberon00 commented Sep 21, 2021

Please ensure a follow-up issue for this new configuration design is created before merging.

@jsuereth
Copy link
Copy Markdown
Contributor

jsuereth commented Sep 21, 2021

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

@Oberon00
Copy link
Copy Markdown
Member

Oberon00 commented Sep 21, 2021 •
edited
Loading

Quoting the description:

get back to the drawing board to choose between two approaches:
either pass both Resources and InstrumentationLibrary to ShouldSample in one spec release OR don't pass neither of them and allow SDKs to configure specific Sampler for a specific Tracer.

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.

@iNikem
Copy link
Copy Markdown
Contributor Author

iNikem commented Sep 22, 2021

Added #1588 (comment)

@Oberon00 Oberon00 mentioned this pull request Sep 22, 2021
Closed
@carlosalberto
Copy link
Copy Markdown
Contributor

carlosalberto commented Sep 22, 2021

@Oberon00 Wanted to double confirm you are fine with Nikita's comment, and we can merge this one?

@Oberon00
Copy link
Copy Markdown
Member

Oberon00 commented Sep 22, 2021

I need to correct my previous comment: I meant to suggest "Closes #1658", #1588 is obviously not resolved.

But yes, I'm OK with merging this PR, although with a somewhat uneasy feeling, as I fear this will keep #1588 in the backlog for quite some time.

@jsuereth jsuereth merged commit 6d44e2e into open-telemetry:main Sep 22, 2021
@iNikem iNikem deleted the revert-instrumentation-library branch September 22, 2021 15:42
@srikanthccv srikanthccv mentioned this pull request Sep 24, 2021
Closed
@cijothomas cijothomas mentioned this pull request Feb 21, 2024
Closed
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@iNikem @bogdandrutu
Revert "Add InstrumentationLibrary to Sampler.ShouldSample (open-tele…
3b003ee 
…metry#1850)" (open-telemetry#1942)

This reverts commit f936f2e.

Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlosalberto carlosalberto carlosalberto approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@jmacd jmacd jmacd approved these changes

@Oberon00 Oberon00 Oberon00 left review comments

+1 more reviewer

@yurishkuro yurishkuro yurishkuro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jsuereth jsuereth

Labels

area:sampling Related to trace sampling area:sdk Related to the SDK spec:trace Related to the specification/trace directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@iNikem @yurishkuro @Oberon00 @jsuereth @carlosalberto @bogdandrutu @jmacd