Probabilistic sampler processor based on draft t-value/r-value encoding #24811
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tor-contrib into jmacd/tvaluesampler
github-actions
bot
added
the
processor/probabilisticsampler
|
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. |
|
@jmacd , is this ready for a first review? |
PeterF778
reviewed
Aug 4, 2023
PeterF778
reviewed
Aug 4, 2023
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Reviewer notes: I am returning this PR to draft mode. I will copy the |
jmacd
added a commit
to jmacd/opentelemetry-collector-contrib
that referenced
this pull request
Dec 8, 2023
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
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>
5 tasks
This was referenced Mar 6, 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.