Skip to content

Conversation

nholbrook
Copy link
Contributor

@nholbrook nholbrook commented Jun 30, 2020

Implements ProbabilitySampler based on the sampler interface in #118.

Relevant snippet from specification:

Probability
The default behavior should be to trust the parent SampledFlag. However there should be configuration to change this.
The default behavior is to apply the sampling probability only for Spans that are root spans (no parent) and Spans with remote parent. However there should be configuration to change this to "root spans only", or "all spans".
Description MUST be ProbabilitySampler{0.000100}.
TODO: Add details about how the probability sampler is implemented as a function of the TraceID. TODO: Split out the parent handling.

@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from 0359e4c to 1097723 Compare June 30, 2020 16:17
@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #136 into master will increase coverage by 0.40%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   94.18%   94.59%   +0.40%     
==========================================
  Files          80       83       +3     
  Lines        1893     2034     +141     
==========================================
+ Hits         1783     1924     +141     
  Misses        110      110              
Impacted Files Coverage Δ
...ude/opentelemetry/sdk/trace/samplers/probability.h 100.00% <100.00%> (ø)
sdk/src/trace/samplers/probability.cc 100.00% <100.00%> (ø)
sdk/test/trace/probability_sampler_test.cc 100.00% <100.00%> (ø)

Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

I think it would make sense to simplify this for the first go, and then discuss what of the SHOULD parts of the spec we want to add:

  • We can get rid of SamplingBehavior and just implement the default DETACHED_SPANS_ONLY logic.
  • We can get rid of defer_parent and just implement the default defer_parent = true.

If deemed necessary, we can add those parts in separate PRs (no of the other SDKs I looked at have this implemented now). This way we can reduce complexity.

You can refer to some other implementations:
Go
Java
Python

@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from 84f5801 to a03c841 Compare July 6, 2020 13:55
@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from a03c841 to b10f67d Compare July 8, 2020 13:51
@nholbrook nholbrook marked this pull request as ready for review July 8, 2020 16:30
@nholbrook nholbrook requested a review from a team July 8, 2020 16:30
@nholbrook
Copy link
Contributor Author

The current implementation of this Sampler handles invalid parameter values (i.e. any probability out of the bounds [0, 1]) silently. The intended functionality is to raise an exception to alert the user, however, this is currently not allowed by CI tests (see #156). This PR can be merged now and the error handing can be added in a later PR, or this will have to wait for #156 to be resolved.

@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from e6629b6 to d930347 Compare July 8, 2020 20:08
Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

This looks good, I added some thoughts.

@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from c4dd785 to a1470b0 Compare July 14, 2020 13:38
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang
Copy link
Member

reyang commented Jul 14, 2020

@nholbrook would you please rebase? Thanks.

@nholbrook nholbrook force-pushed the nholbrook/probability-sampler branch from a1470b0 to a788576 Compare July 14, 2020 23:20
@nholbrook
Copy link
Contributor Author

@reyang Rebased

@reyang reyang merged commit ab2a5c1 into open-telemetry:master Jul 14, 2020
@reyang reyang mentioned this pull request Aug 25, 2020
@neopaf
Copy link

neopaf commented Feb 11, 2025

Friends, please shed some light on necessity of CalculateThreshold

It moves us from double to int64 space, which is sometimes good.

But is it really necessary in this case?

What is wrong with converting trace_id to ratio and using it directly, without double-to-int magic?

//  if (CalculateThresholdFromBuffer(trace_id) <= threshold_)
  if (CalculateRatioFromBuffer(trace_id) <= ratio_)

GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025
…s-create-or-update-comment-digest

Update peter-evans/create-or-update-comment digest to 4ed2ee4
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.

5 participants