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

Probabilistic sampler processor based on draft t-value/r-value encoding #24811

Closed
wants to merge 42 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Aug 2, 2023

Description:
Draft for open-telemetry/opentelemetry-specification#3602.
Previously #22058, see also open-telemetry/oteps#235

Reviewers: thank you for your patience. While this has was first started in May 2023 and has progressed slowly, it's finally ready and the Sampling SIG has converged on this design.

This PR adds two consistent sampler modes to the existing probabilistic sampler, "equalizing" and "proportional". The consistent sampling mechanism used here is based on 56 bits of randomness which is extracted from the tracestate or the TraceID. Because this is a tail sampler, there is no real benefit to synthesizing an r-value, therefore when rv: is not set in the tracestate, this implementation unconditionally uses the TraceID bits.

When open-telemetry/opentelemetry-proto#503 is included in the next release of OTLP and pdata is extended with the corresponding field, this processor will be updated to use the randomness bit before deciding what to do about TraceIDs that do not have it set. We imagine adding a configuration parameter to indicate how to treat this scenario, but since this is a tail processor, there is little we can do that would be consistent, so using the TraceID bits as they are is probably best, despite the missing confirmation of their randomness.

Note there is a question of how to treat spans that arrive with an empty tracestate, indicating what is likely to be 100% sampling but could potentially be otherwise. There are no OpenTelemetry specifications for what we should do in this situation, and we leave this decision open.

Link to tracking Issue:
open-telemetry/opentelemetry-specification#1413

Testing: Complete.

Documentation: Added new documentation.

@github-actions github-actions bot added the processor/probabilisticsampler Probabilistic Sampler processor label Aug 2, 2023
@github-actions github-actions bot requested a review from jpkrohling August 2, 2023 23:35
@jmacd
Copy link
Contributor Author

jmacd commented Aug 3, 2023

Based on the feedback received in Sampling SIG today, I refactored the implementation to avoid the use of floating point numbers except when explicitly converting thresholds to probabilities and back to thresholds. I have also added a second implementation of the based on byte arrays instead of unsigned numbers, for comparison. The unsigned implementation is trickier to read and write, but faster.

goos: darwin
goarch: arm64
pkg: github.com/open-telemetry/opentelemetry-collector-contrib/pkg/sampling
BenchmarkThresholdCompareAsUint64
BenchmarkThresholdCompareAsUint64-10    	1000000000	         0.7034 ns/op	       0 B/op	       0 allocs/op
BenchmarkThresholdCompareAsBytes
BenchmarkThresholdCompareAsBytes-10     	191687650	         5.990 ns/op	       0 B/op	       0 allocs/op

@jpkrohling
Copy link
Member

@jmacd , is this ready for a first review?

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Aug 19, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2023

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Sep 3, 2023
@jmacd jmacd reopened this Dec 8, 2023
@jmacd jmacd marked this pull request as ready for review December 8, 2023 21:53
@jmacd jmacd requested a review from a team December 8, 2023 21:53
@jmacd
Copy link
Contributor Author

jmacd commented Dec 8, 2023

Reviewer notes: I am returning this PR to draft mode. I will copy the pkg/sampling package out of this PR and form a new PR with it. After that stuff merges (if we may), then I'll re-open this one.

@jmacd jmacd marked this pull request as draft December 8, 2023 22:07
jmacd added a commit to jmacd/opentelemetry-collector-contrib that referenced this pull request Dec 8, 2023
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 23, 2023
Copy link
Contributor

github-actions bot commented Jan 7, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jan 7, 2024
jpkrohling added a commit that referenced this pull request Jan 31, 2024
…29720)

**Description:** This is the `pkg/sampling` portion of of
#24811.

**Link to tracking Issue:** 
#29738

open-telemetry/opentelemetry-specification#1413

**Testing:** Complete.

**Documentation:** New README added.

---------

Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
Co-authored-by: Kent Quirk <kentquirk@gmail.com>
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Feb 1, 2024
…pen-telemetry#29720)

**Description:** This is the `pkg/sampling` portion of of
open-telemetry#24811.

**Link to tracking Issue:** 
open-telemetry#29738

open-telemetry/opentelemetry-specification#1413

**Testing:** Complete.

**Documentation:** New README added.

---------

Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
Co-authored-by: Kent Quirk <kentquirk@gmail.com>
@jmacd
Copy link
Contributor Author

jmacd commented Mar 21, 2024

Replaced by #31894.

jpkrohling added a commit that referenced this pull request Jun 13, 2024
…rt OTEP 235) (#31894)

**Description:** Creates new sampler modes named "equalizing" and
"proportional". Preserves existing functionality under the mode named
"hash_seed".

Fixes #31918

This is the final step in a sequence, the whole of this work was
factored into 3+ PRs, including the new `pkg/sampling` and the previous
step, #31946. The two new Sampler modes enable mixing OTel sampling SDKs
with Collectors in a consistent way.

The existing hash_seed mode is also a consistent sampling mode, which
makes it possible to have a 1:1 mapping between its decisions and the
OTEP 235 randomness and threshold values. Specifically, the 14-bit hash
value and sampling probability are mapped into 56-bit R-value and
T-value encodings, so that all sampling decisions in all modes include
threshold information.

This implements the semantic conventions of
open-telemetry/semantic-conventions#793, namely
the `sampling.randomness` and `sampling.threshold` attributes used for
logs where there is no tracestate.

The default sampling mode remains HashSeed. We consider a future change
of default to Proportional to be desirable, because:

1. Sampling probability is the same, only the hashing algorithm changes
2. Proportional respects and preserves information about earlier
sampling decisions, which HashSeed can't do, so it has greater
interoperability with OTel SDKs which may also adopt OTEP 235 samplers.

**Link to tracking Issue:** 

Draft for
open-telemetry/opentelemetry-specification#3602.
Previously
#24811,
see also open-telemetry/oteps#235
Part of #29738

**Testing:** New testing has been added.

**Documentation:** ✅

---------

Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/probabilisticsampler Probabilistic Sampler processor Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants