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

Change name of ProbabilitySampler to TraceIdRatioBased #1115

Merged
merged 5 commits into from
Sep 3, 2020

Conversation

evantorrie
Copy link
Contributor

fix #1083

  • Modify behavior to ignore parent span
  • Add test for inclusivity property on TraceIdRatioBased sampler
  • Modify tests in trace_test.go to reflect change in parent span behavior

* Modify behavior to ignore parent span
* Add test for inclusivity property on TraceIdRatioBased sampler
* Modify tests in `trace_test.go` to reflect change in parent
  span behavior
@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

Merging #1115 into master will decrease coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1115     +/-   ##
========================================
- Coverage    77.1%   77.1%   -0.1%     
========================================
  Files         133     133             
  Lines        7109    7106      -3     
========================================
- Hits         5488    5483      -5     
  Misses       1371    1371             
- Partials      250     252      +2     
Impacted Files Coverage Δ
sdk/trace/sampling.go 83.7% <100.0%> (+4.2%) ⬆️
api/trace/api.go 54.0% <0.0%> (-9.7%) ⬇️
api/trace/tracetest/provider.go 90.4% <0.0%> (-9.6%) ⬇️
sdk/trace/provider.go 85.8% <0.0%> (+0.3%) ⬆️
sdk/trace/batch_span_processor.go 81.4% <0.0%> (+1.0%) ⬆️

@evantorrie
Copy link
Contributor Author

Original issue #1083 says to change the name to TraceIDRatioBasedSampler (as does the title of open-telemetry/opentelemetry-specification#611), but the actual wording change in the specification says to change ProbabilitySampler to TraceIDRatioBased.

I guess it is more congruent with AlwaysOn and AlwaysOff which do not have a Sampler suffix, as well as the upcoming changes to ParentBased from ParentOrElse #1084

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Tyler Yahn <[email protected]>
Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

LGTM

@MrAlias MrAlias merged commit 4d83d5b into open-telemetry:master Sep 3, 2020
@evantorrie evantorrie deleted the traceid-ratio branch September 10, 2020 15:48
evantorrie added a commit to evantorrie/opentelemetry-go that referenced this pull request Sep 10, 2020
…y#1115)

* Change name of ProbabilitySampler to TraceIdRatioBased

* Modify behavior to ignore parent span
* Add test for inclusivity property on TraceIdRatioBased sampler
* Modify tests in `trace_test.go` to reflect change in parent
  span behavior

* Add to CHANGELOG

* Satisfy golint

* Update CHANGELOG.md

Co-authored-by: Tyler Yahn <[email protected]>

Co-authored-by: Tyler Yahn <[email protected]>
KasonBraley added a commit to KasonBraley/opentelemetry-go-contrib that referenced this pull request Feb 16, 2023
Update the examples to use the current API for configuring production sampling.
These names were changed a couple of years ago.

open-telemetry/opentelemetry-go#1115
open-telemetry/opentelemetry-go#1153
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename ProbabilitySampler to TraceIdRatioBasedSampler
4 participants