Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update Sampler.ShouldSample parent parameter to be Context #881

Merged
merged 10 commits into from
Oct 20, 2020
Merged

Update Sampler.ShouldSample parent parameter to be Context #881

merged 10 commits into from
Oct 20, 2020

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Aug 26, 2020 •
edited
Loading

Fixes #1024

Changes

  • Change Sampler.ShouldSample to receive a full parent Context

CC @alolita (you are responsible for the sampling issues currently)

@Oberon00 Oberon00 requested review from a team August 26, 2020 08:32
Oberon00
Oberon00 commented Aug 26, 2020
View reviewed changes
specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
@Oberon00 Oberon00 added area:sampling Related to trace sampling area:sdk Related to the SDK release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory labels Aug 26, 2020
@Oberon00

This comment has been minimized.

Sign in to view
@Oberon00 Oberon00 changed the title Update Sampler.ShouldSample parent parameter to be Context, clean up. Update Sampler.ShouldSample parent parameter to be Context. Aug 26, 2020
@carlosalberto

This comment has been minimized.

Sign in to view
@lzchen lzchen mentioned this pull request Aug 26, 2020
Closed
@bogdandrutu

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions github-actions bot added the Stale label Sep 9, 2020
@Oberon00 Oberon00 marked this pull request as draft September 9, 2020 08:30
@carlosalberto
Copy link
Contributor

@bogdandrutu out of curiosity, of specifying Context as the single source of parenthood goes in, this would become required for GA?

@github-actions github-actions bot removed the Stale label Sep 17, 2020
@carlosalberto
Copy link
Contributor

@bogdandrutu @Oberon00 You guys think is is needed after the related PR was merged?

cc @alolita

@Oberon00 Oberon00 marked this pull request as ready for review September 24, 2020 08:12
@Oberon00
Copy link
Member Author

@carlosalberto I think "needed" is the wrong word, just the SpanContext should work as well as before it is not needed more than before but I think it now makes even more sense. Note that among other things, this gives access to the full parent span in many cases, instead of only the SpanContext.

@Oberon00 Oberon00 removed the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Sep 28, 2020
@Oberon00 Oberon00 mentioned this pull request Sep 28, 2020
Closed
@arminru arminru requested a review from a team October 6, 2020 12:27
@Oberon00 Oberon00 added the release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs label Oct 6, 2020
@Oberon00
Copy link
Member Author

I think this is really a straightforward, simple to implement (maybe even simplifying) and beneficial change since #875 and also #994 was merged some time ago. @open-telemetry/specs-approvers please review!

@arminru
Copy link
Member

arminru commented Oct 13, 2020

Please add an entry in the spec compliance matrix (preferably marking it as "[SDK]" or something in case we'll split the table later on).

@Oberon00
Copy link
Member Author

It feels like the changelog and compliance matrix should somehow be merged...

I'll add the line in the table.

@Oberon00
Copy link
Member Author

Done in f081f86.
I did not add the [SDK] prefix since it's in a Sampling section which is all-SDK anyway.

@carlosalberto
Copy link
Contributor

@open-telemetry/specs-approvers Please review. Although not something required-for-GA, it's still allowed to go in and looks straight enough ;)

@Oberon00
Copy link
Member Author

This PR has been semantically unchanged since 25 days ago and it has two approvers from different companies, so can we just merge it? @open-telemetry/technical-committee

Otherwise I will bring it up in the SIG spec meeting tomorrow, unless it can get more reviews until then 😃 @open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers

@carlosalberto
Copy link
Contributor

@Oberon00 Lets mention it tomorrow, please (although I'm strongly inclined to merge it already, as it's a natural progression of allowing Context to be the only parenthood source).

This was referenced Oct 20, 2020
Closed
Merged
bogdandrutu
bogdandrutu approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discard all my previous comments, I think this is a good change

@bogdandrutu bogdandrutu merged commit cbc8936 into open-telemetry:master Oct 20, 2020
@jkwatson jkwatson mentioned this pull request Oct 20, 2020
Merged
@jtescher jtescher mentioned this pull request Oct 20, 2020
Closed
@lzchen lzchen mentioned this pull request Oct 21, 2020
Closed
@tsloughter tsloughter mentioned this pull request Oct 30, 2020
Closed
@jtescher jtescher mentioned this pull request Nov 23, 2020
Merged
@Oberon00 Oberon00 deleted the samplerparent branch December 18, 2020 10:59
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@Oberon00 @arminru
Update Sampler.ShouldSample parent parameter to be Context (open-tele…
3840367 
…metry#881)

* Change Sampler.ShouldSample to get full parent Context.

* Link to Context.

* SpanContext is never stored in Context anymore

* Add CHANGELOG.

* Wording.

* Add compliance matrix entry.

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cijothomas cijothomas cijothomas left review comments

@arminru arminru arminru approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

@carlosalberto carlosalberto carlosalberto approved these changes

Assignees

@carlosalberto carlosalberto

Labels
area:sampling Related to trace sampling area:sdk Related to the SDK release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sampler.ShouldSample parent parameter should be Context
5 participants
@Oberon00 @carlosalberto @bogdandrutu @arminru @cijothomas