-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Span Metrics connector support for OTEP 235 probability sampling #33632
Comments
Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
@portertech @Frapschen please review |
@jmacd did you have plans to work on this in the near future? We are implementing OTEP-235 in order to get adjusted-count spanmetrics. So, if it would help move the development along, I can put my hand up to start work on this. |
We (Elastic) are working on porting some functionality from Elastic APM to OpenTelemetry Collector, which includes functionality that you're describing here, in a new connector: https://github.com/elastic/opentelemetry-collector-components/tree/main/connector/signaltometricsconnector. We're planning to offer this to the contrib repo in the not too distant future. (CC @lahsivjar) The approach we took in the past (i.e. in the existing Elastic APM code) was to scale fractional counts up when recording, and scale back down when emitting metrics. That has the downside of limiting the range of counts that can be accumulated, but in practice it's not a problem at high enough frequency. Another option would be to use probabilistic counting, which we're planning to use in the connector I mentioned above: elastic/opentelemetry-collector-components#170 @jmacd did you consider these alternatives? |
I'm wondering why we need to have a In the case where sampling probability is Is this for a case where the customer samples in the SDK, but still expects to see all spans (sampled or not) in the connector for metrics computation? Note that the SDK currently does not allow this. However, an additional hypothetical There are a few nuances on how adjustment works with head-based sampling. The spec recommends using However, This is probably why in the previous experimental version of "tracestate probabilistic sampling":
So it seems like in order to get span metrics out of the intermediate service, one has to configure a non-root sampler of The interoperability guidance describes this is possible as long as
This effectively requires the same probabilistic sampling across the whole service chain. In practice, this will present challenges if the customer has different setups elsewhere as a legacy case, during a migration, or wants to adjust sampling rates dynamically or just some general tweaking, ... The requirement of requiring same |
@yuanyuanzhao3 @PeterF778 @oertl please consider: In last week's SIG meeting, we discussed how to resolve the comment above. There are two quoted statements: A. For non-root spans, composite samplers SHOULD NOT condition the choice of delegated-to sampler based on the parent’s sampled flag or OpenTelemetry tracestate. B. The OpenTelemetry built-in ParentBased sampler is interoperable with the ConsistentProbabilityBased sampler, provided that the delegated-to sampler does not change the decision that determined its selection. I am exploring the ways in which these statements intersect with each other. There are two failure modes we are potentially concerned about:
The reason for this line of questioning is that I want to preserve the existing ParentBased API specification, which allows registering up to 5 delegated samplers for the various conditions. In the experimental draft, the ConsistentParentBased sampler registers only a root sampler, because parent-based decisions are consistent. The feature I want to preserve is the "4-way" delegation for child spans, which are:
I am wondering if statement (A) is overly restrictive when the samplers being delegated to are also consistent. Can we rephrase (A) to add: "For non-root spans, composite samplers MAY condition the choice of delegated-to Samplers on sampled-flag/tracestate when the delegated-to sampler is consistent." I believe that it is safe to make a delegation choice when all the involved samplers are consistent samplers in the sense that failure mode 1, incorrect counting, does not happen. I believe this is true because it implies statement (B). If the delegated-to samplers are consistent, they cannot change the decision that determined its selection. Therefore, I think we can upgrade the existing ParentBased sampler to support consistency without much pain. The changes I'd propose are:
|
If I understand your proposal correctly, it will not work, i.e. the counts will not be correct. |
I agree with @PeterF778.
This will break unbiased counting. The reason is that the child's sampling EDIT: corrected child's sampling decision by child's sampling threshold |
I wasn't clear enough -- I expect the delegation to be managed as we have described in PR 4166.
I mean to specify that the ParentBased sampler would require consistent samplers to follow that rule. The delegated-to sampler cannot change the decision without raising the threshold, and it cannot lower the threshold. For your example, then, we should see a 20% population with adjusted count 5, no threshold change from a 50% sampler. |
I also think that allowing conditioning the sampler choice won't work. I
also thought about some composite adjusted count by combining the adjusted
count on arrival and the sampling happen in "this" sampler, but that won't
work as the population seen by "this" sampler is not guaranteed to be the
same as its parents.
…On Wed, Nov 6, 2024 at 1:01 AM Joshua MacDonald ***@***.***> wrote:
The result will be that the child span will be sampled with P=0.2, yet the
adjusted count will be 2.
I wasn't clear enough -- I expect the delegation to be managed as we have
described in PR 4166
<https://github.com/open-telemetry/opentelemetry-specification/pull/4166/files>
.
In this case the sampler MUST output the span with a th equal to max(input
th, chosen th). In other words, th MUST NOT be decreased (as it is not
possible to retroactively adjust an earlier stage's sampling probability),
and it MUST be increased if a lower sampling probability was used.
I mean to specify that the ParentBased sampler would require consistent
samplers to follow that rule. The delegated-to sampler cannot change the
decision and cannot lower the threshold. For your example, then, we should
see a 20% population with adjusted count 5, no threshold change from a 50%
sampler.
—
Reply to this email directly, view it on GitHub
<#33632 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJIUW4WGVOHIJUYRUS5O5G3Z7GWEVAVCNFSM6AAAAABJQMMP5WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJYG44DMOJTGE>
.
You are receiving this because you were mentioned.Message ID:
<open-telemetry/opentelemetry-collector-contrib/issues/33632/2458786931@
github.com>
|
I agree that it won't work. Following the example, let's change samplers A and B to |
I think it will be helpful to consider a general case where there are
What Will come back flesh out more here ... |
Component(s)
connector/spanmetrics
Is your feature request related to a problem? Please describe.
OTEP 235 describes how to encode sampling probability, and now probabilistic sampler processor supports it.
I propose to two new boolean flags to the
Config
of this component:sampling_adjustment
(default: false) When disabled, each span counts 1. When enabled and sampling has been recorded, each span counts as the inverse of its sampling probability.fractional_counting
(default: false) When disabled, spans are counted as integer data points. When enabled, spans are counted as floating point number data points. _Note this only applies to Sum points, not to Histogram point count fields, because OpenTelemetry does not (currently) support floating-point count histograms.When the sampling adjustment feature is enabled and the fractional counting feature is disabled, there is a potential for errors to be introduced stemming from either inadequate precision or from the use of non-integer-reciprocal sampling probabilities.
As an example of the first case:
The sampler is configured with 33.33% sampling, which is sufficiently close to 1-in-3 that integer counts will have very small error using the threshold calculated by
pkg/sampling
. However, the sampler is also configured withsampling_precision: 1
which forces the effective probability down in this case. Note the rejection thresholdot=th:a
equals 10/16 = 37.5%, and the rejection thresholdot=th:b
equals 11/16 = 31.25%. The sampler will outputot=th:b
in this case, and the effective adjustment equals exactly1/(1 - 11/16)
= 3.2, which rounds down to 3 for a error of 6.7%. The user should raise sampling precision to lower the systematic error.As an example of the second case:
The sampler is configured for 75% sampling. This is exactly expressed using powers-of-two, and the adjustment in this case is 1.333. No amount of precision will help in this case. The user should choose sampling probabilities that equate with integer counts. This rules out sampling percentages above 50%.
Describe the solution you'd like
When a sampling adjustment is used without fractional counting, a warning will be issued for spans with sampling probability with an unacceptable margin of error.
Describe alternatives you've considered
When a sampling adjustment is used without fractional counting, a floating-point valued metric named
M_residue
will be incremented (for metric namedM
) with the residual error. This amount can be monitored and used to correct the integer-valued metric.Additional context
open-telemetry/semantic-conventions#793
The text was updated successfully, but these errors were encountered: