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

Sampler.ShouldSample parent parameter should be Context #1024

Closed
Oberon00 opened this issue Sep 28, 2020 · 2 comments · Fixed by #881
Closed

Sampler.ShouldSample parent parameter should be Context #1024

Oberon00 opened this issue Sep 28, 2020 · 2 comments · Fixed by #881
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK priority:p2 Medium priority level 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

Comments

@Oberon00
Copy link
Member

Oberon00 commented Sep 28, 2020 •
edited
Loading

This is a tracking issue for PR #881: Sampler.ShouldSample should be changed to receive the full parent Context instead of the parent SpanContext, as there is no reason to only provide the SpanContext, and it is one of the few remaining places in the spec where we deal with a bare SpanContext.
@Oberon00 Oberon00 added spec:trace Related to the specification/trace directory area:sampling Related to trace sampling area:sdk Related to the SDK labels Sep 28, 2020
@Oberon00
Copy link
Member Author

Since spec is frozen, this probably has to be after-ga (even though I had originally tagged the PR required-for-ga), except if we say it is related enough to #1019.

@Oberon00 Oberon00 mentioned this issue Sep 28, 2020
Merged
@Oberon00 Oberon00 changed the title Update Sampler.ShouldSample parent parameter to be Context. Sampler.ShouldSample parent parameter should be Context. Sep 28, 2020
@Oberon00 Oberon00 changed the title Sampler.ShouldSample parent parameter should be Context. Sampler.ShouldSample parent parameter should be Context Sep 28, 2020
@bogdandrutu
Copy link
Member

The only problem with this is if we want to extend in the future to support delayed calls to Sampler because that requires holding a reference to the parent Context. But that itself is a new feature which requires more changes, so because we do not intend to support that I am fine with having this now

@andrewhsu andrewhsu added the release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs label Sep 29, 2020
@andrewhsu andrewhsu added the priority:p2 Medium priority level label Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK priority:p2 Medium priority level 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
3 participants
@andrewhsu @Oberon00 @bogdandrutu