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

Change should_sample parameters to be spec compliant #1764

Merged
merged 9 commits into from Apr 15, 2021
Merged

Change should_sample parameters to be spec compliant #1764

merged 9 commits into from Apr 15, 2021
+106 −51

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Apr 12, 2021 •
edited

Fixes #1763

  1. Adds SpanKind to should_sample
  2. should_sample uses trace_state from the passed in Context 's SpanContext 's TraceState instead of the explicitly passed in TraceState.

@lzchen lzchen requested a review from a team as a code owner April 12, 2021 19:34
@lzchen lzchen requested review from owais and codeboten and removed request for a team April 12, 2021 19:34
srikanthccv
srikanthccv reviewed Apr 13, 2021
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Shouldn't we also update the should_sample to take trace_state from parent_context?

@lzchen
Copy link
Contributor Author

lzchen commented Apr 13, 2021

@lonewolf3739
I believe we already do that here

@lzchen lzchen requested a review from srikanthccv April 13, 2021 15:03
@srikanthccv
Copy link
Member

Does it make sense to move that to should_sample and add a note saying that is the recommended way and discourage people from using trace_state param?

@lzchen
Copy link
Contributor Author

lzchen commented Apr 13, 2021

@lonewolf3739

Does it make sense to move that to should_sample and add a note saying that is the recommended way and discourage people from using trace_state param?

This makes sense. I'll make these changes.

@lzchen lzchen requested review from owais and codeboten April 14, 2021 17:45
@lzchen
Copy link
Contributor Author

lzchen commented Apr 14, 2021

@lonewolf3739
Please take a look.

@lzchen lzchen changed the title Add SpanKind to should_sample Change should_sample parameters to be spec compliant Apr 14, 2021
ocelotl
ocelotl approved these changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Approving, there is mostly a minor fix in documentation suggested.

opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/tests/trace/test_sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/tests/trace/test_sampling.py Outdated Show resolved Hide resolved
opentelemetry-sdk/tests/trace/test_sampling.py Outdated Show resolved Hide resolved
@lzchen lzchen merged commit 71e3a7a into open-telemetry:main Apr 15, 2021
@lzchen lzchen deleted the sample branch April 15, 2021 15:00
@adriangb adriangb mentioned this pull request Nov 10, 2023
Closed
@adriangb adriangb mentioned this pull request Apr 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocelotl ocelotl ocelotl approved these changes

@srikanthccv srikanthccv srikanthccv approved these changes

@owais owais Awaiting requested review from owais

@codeboten codeboten Awaiting requested review from codeboten

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discrepancy between spec and sdk implementation of should_sample
5 participants
@lzchen @srikanthccv @owais @codeboten @ocelotl