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

feat(core): rename ProbabilitySampler to TraceIdRatioBasedSampler #1562

Merged
merged 4 commits into from
Oct 3, 2020

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Oct 1, 2020

TraceIdRatioBasedSampler are determinist samplers based on trace ids.

Which problem is this PR solving?

Short description of the changes

  • rename ProbabilitySampler to TraceIdRatioBasedSampler
  • TraceIdRatioBasedSampler samples span based on trace ids deterministically. A TraceIdRatioBased sampler with a given sampling rate also sample all traces that any TraceIdRatioBased sampler with a lower sampling rate would sample.

Note: per the time the PR is filed, the implementation detail of the sampler regarding the trace ids on spec is not yet determined. Though there will be no API changes regarding the new spec on this part, so it is safe to land the API with this primitive reminder based implemention.

This is a breaking change.

TraceIdRatioBasedSamplers are determinist samplers based on trace ids.
@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #1562 into master will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1562      +/-   ##
==========================================
+ Coverage   92.74%   92.93%   +0.18%     
==========================================
  Files         146      156      +10     
  Lines        4593     4870     +277     
  Branches      943      988      +45     
==========================================
+ Hits         4260     4526     +266     
- Misses        333      344      +11     
Impacted Files Coverage Δ
packages/opentelemetry-tracing/src/utility.ts 100.00% <ø> (ø)
...core/src/trace/sampler/TraceIdRatioBasedSampler.ts 100.00% <100.00%> (ø)
...y-plugin-grpc-js/src/server/serverStreamAndBidi.ts 100.00% <0.00%> (ø)
...plugin-grpc-js/src/client/loadPackageDefinition.ts 100.00% <0.00%> (ø)
...s/opentelemetry-plugin-grpc-js/src/client/utils.ts 93.25% <0.00%> (ø)
...s/opentelemetry-plugin-grpc-js/src/server/index.ts 100.00% <0.00%> (ø)
...ackages/opentelemetry-plugin-grpc-js/src/grpcJs.ts 100.00% <0.00%> (ø)
...telemetry-plugin-grpc-js/src/server/patchServer.ts 90.00% <0.00%> (ø)
packages/opentelemetry-plugin-grpc-js/src/utils.ts 92.30% <0.00%> (ø)
... and 3 more

@obecny obecny merged commit fa9af4a into open-telemetry:master Oct 3, 2020
@legendecas legendecas deleted the trace-id-ratio-based branch October 4, 2020 15:25
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…te (open-telemetry#1562)

* fix(redis-4): omit credentials from db.connection_string span attribute

* Reconstruct non-sensitive db.connection_string using built-in URL.href

* style(redis-4): Fix code style according to community standards

* fix(redis-4): Log error on connection string sanitization failure

---------

Co-authored-by: Amir Blum <amirgiraffe@gmail.com>
Co-authored-by: Haddas Bronfman <85441461+haddasbronfman@users.noreply.github.com>
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 and implement ratio sampling
4 participants