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.

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

Fix Sampler.ShouldSample parameter descriptions. #884

Merged

Conversation

Oberon00
Copy link
Member

Fixes #883.

Only non-normative changes, so no CHANGELOG entry.

@Oberon00 Oberon00 added bug Something isn't working area:sdk Related to the SDK area:sampling Related to trace sampling spec:trace Related to the specification/trace directory release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p3 Lowest priority level labels Aug 26, 2020
@Oberon00 Oberon00 requested review from a team August 26, 2020 09:04
@Oberon00 Oberon00 changed the title Fix Sampler.ShouldSample paramter descriptions. Fix Sampler.ShouldSample parameter descriptions. Aug 26, 2020
@carlosalberto
Copy link
Contributor

Overall fine. Left a pair of small, non-blocking comments.

@SergeyKanzhelev
Copy link
Member

can you please rebase so this can be merged?

image

@arminru
Copy link
Member

arminru commented Aug 26, 2020

@SergeyKanzhelev I thought this setting was changed in the repo following #512 (comment)? If there are no conflicts, merging should work fine even if another unrelated PR was merged after the last merge/rebase.

@SergeyKanzhelev
Copy link
Member

@SergeyKanzhelev I thought this setting was changed in the repo following #512 (comment)? If there are no conflicts, merging should work fine even if another unrelated PR was merged after the last merge/rebase.

This is why I posted the screenshot. It didn't even gave me a button "Update branch". Perhaps a GitHub glitch. Merging now as it is widely approved and uncontroversial.

@SergeyKanzhelev SergeyKanzhelev merged commit 85b9c26 into open-telemetry:master Aug 26, 2020
@arminru arminru deleted the samplercleanup branch August 27, 2020 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK bug Something isn't working priority:p3 Lowest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sampler.ShouldSample has wrong parameter descriptions.
7 participants