-
Notifications
You must be signed in to change notification settings - Fork 164
Probability sampling: Encode Span's head-adjusted count #170
Conversation
@oertl @yurishkuro @paulosman this document is unchanged from the most recent discussion in #148. I thought that opening a new PR would improve our chances of getting this specification merged. This specification includes a lot of background to establish that there are many ways to sample and count spans, but all of them involve recording an adjusted count. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of merging the narrative, but I do not full agree with the proposed spec changes.
Thank you. I can at least provide a vendor perspective, as a consumer of sampled telemetry events. Adding an adjusted count solves a real problem for us: At the moment we depend on the return value from ShouldSample including attributes that are added to the span. We have custom plugins that add a sample rate to this list of attributes, which is then read at ingest and used to make estimates about the population. If no adjusted count is included, we assume a value of 1. This works but requires customers to use custom sampling plugins that provide no real value above the ability to communicate the adjusted count (the plugins use a deterministic algorithm that is similar to the For tail sampling, we accept OTLP data to our sampling proxy and then convert the data to our HTTP API which includes an adjusted count. We'd prefer to be able to do end-to-end OTLP, which the spec change described in this OTEP would allow us to do. So 👍 from me. Thanks again for writing this up. |
@open-telemetry/specs-trace-approvers Please take a look. I've completed a prototype of this proposal in https://github.com/open-telemetry/opentelemetry-go/compare/main...jmacd:jmacd/propagate?expand=1. The text in this PR matches the prototype. |
There is an error condition implied by #168 that would occur when the random variable This illustrates the benefits of deriving As specified, @oertl, the |
Suggestion: we may want to consider adding top-level Span fields for count and sampler name, given how important these concepts are and the space savings we'd achieve by recording values only. |
I revised this OTEP based on the Thursday Sampling SIG discussion, in which it was proposed that we introduce just a single integer field to encode head sampling probability, ignoring (for the time being) the topic of tail sampling. The proposed field would take 65 possible values, as follows:
and the
|
@jmacd, maybe it is better to sacrify the sampling rate 1/2^62 (which will never be used in practice I guess) in order to fit the state into 6 bits? |
I agree, and have applied this change in both OTEPs. |
@yurishkuro I responded to your commend above. This PR is reduced to proposing to a single
This matches OTEP #168, which is also ready for another look, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm (minor nit on v=63), but I think this OTEP is not actionable in the current form, it needs to propose changes to the Sampler SPI to return inclusion probability (or something) to the Tracer.
@jmacd, @yurishkuro I have implemented an abstract Sampler base class prototype in Java, see here. Any derived implementation will meet the requirements for consistent sampling. It takes care of generating the geometric random value and propagates it together with the exponent of the sampling probability. There are currently two prototype implementations: ConsistentParentRateSampler and ConsistentFixedRateSampler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This OTEP looks GTM, any small changes could be worked out during the spec process.
The only remaining question here, possibly, is whether the Span field name should be |
…ry#170) * draft from OTEP 148 * renumber * typo in header * typos * formatting * clean TOC * TOC edit * Clarify the counting algorithm * typos * grammar * grammar * two paragraphs * Summarize from the prototype * Remove exported count from proposed spec language * statement about not dropping sampler attributes * from Thursday's SIG, limit proposal to head sampling probability * log_head_adjusted_count * Use 6 bits * update the proto text * add detail on SamplerResult * remove metrics examples, add to span-to-metrics examples * whitespace * lint
…ry#170) * draft from OTEP 148 * renumber * typo in header * typos * formatting * clean TOC * TOC edit * Clarify the counting algorithm * typos * grammar * grammar * two paragraphs * Summarize from the prototype * Remove exported count from proposed spec language * statement about not dropping sampler attributes * from Thursday's SIG, limit proposal to head sampling probability * log_head_adjusted_count * Use 6 bits * update the proto text * add detail on SamplerResult * remove metrics examples, add to span-to-metrics examples * whitespace * lint
…ry#170) * draft from OTEP 148 * renumber * typo in header * typos * formatting * clean TOC * TOC edit * Clarify the counting algorithm * typos * grammar * grammar * two paragraphs * Summarize from the prototype * Remove exported count from proposed spec language * statement about not dropping sampler attributes * from Thursday's SIG, limit proposal to head sampling probability * log_head_adjusted_count * Use 6 bits * update the proto text * add detail on SamplerResult * remove metrics examples, add to span-to-metrics examples * whitespace * lint
…ry/oteps#170) * draft from OTEP 148 * renumber * typo in header * typos * formatting * clean TOC * TOC edit * Clarify the counting algorithm * typos * grammar * grammar * two paragraphs * Summarize from the prototype * Remove exported count from proposed spec language * statement about not dropping sampler attributes * from Thursday's SIG, limit proposal to head sampling probability * log_head_adjusted_count * Use 6 bits * update the proto text * add detail on SamplerResult * remove metrics examples, add to span-to-metrics examples * whitespace * lint
This text is reproduced from #148, a review that became too long after several revisions to the document.
This proposes specification text for a
sampler.name
andsampler.adjusted_count
attribute, used to convey sampling probability in recordedSpanData
messages.A companion OTEP #168 discusses how to propagate sampling probability when using the
Parent
sampler.