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

Span Metrics connector support for OTEP 235 probability sampling #33632

Open
jmacd opened this issue Jun 18, 2024 · 6 comments
Open

Span Metrics connector support for OTEP 235 probability sampling #33632

jmacd opened this issue Jun 18, 2024 · 6 comments

Comments

@jmacd
Copy link
Contributor

jmacd commented Jun 18, 2024

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 with sampling_precision: 1 which forces the effective probability down in this case. Note the rejection threshold ot=th:a equals 10/16 = 37.5%, and the rejection threshold ot=th:b equals 11/16 = 31.25%. The sampler will output ot=th:b in this case, and the effective adjustment equals exactly 1/(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 named M) with the residual error. This amount can be monitored and used to correct the integer-valued metric.

Additional context

open-telemetry/semantic-conventions#793

@jmacd jmacd added enhancement New feature or request needs triage New item requiring triage labels Jun 18, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

Copy link
Contributor

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 @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@atoulme
Copy link
Contributor

atoulme commented Oct 12, 2024

@portertech @Frapschen please review

@jamesmoessis
Copy link
Contributor

@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.

@axw
Copy link
Contributor

axw commented Oct 16, 2024

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.

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?

@yuanyuanzhao3
Copy link

yuanyuanzhao3 commented Oct 24, 2024

I'm wondering why we need to have a sampling_adjustment flag, where the value can be false. It is not needed for cases where sampling probability is 100%.

In the case where sampling probability is <100%, disabling this flag will result in in-accurate metrics.

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 EXPORT sampling decision can do this. It comes with a cost, but also has a benefit of simplicity and accuracy. It also provides an independent dimension of data (that is not dependent on the sampling mechanism).

There are a few nuances on how adjustment works with head-based sampling.

The spec recommends using ParentBased sampler, which by default invokes AlwaysOn when parent sampling decision is true and AlwaysOff otherwise, for non-root spans. This applies to the case at a service in the middle of a chain.

However, AlwaysOn will output a tracestate=th:0. But no matter what value we set sampling_adjustment flag to, the generated metric will not have adequate precision.

This is probably why in the previous experimental version of "tracestate probabilistic sampling":

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.

So it seems like in order to get span metrics out of the intermediate service, one has to configure a non-root sampler of ConsistentProbabilityBased.

The interoperability guidance describes this is possible as long as

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.

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 ConsistenProbabilityBased sampling here is probably not really head-based sampling any more, since respecting the head-based decision is now just a coincidence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants