[connector/spanmetricsv2] Scale spans based on adjusted count #95
Conversation
There was a problem hiding this comment.
The logic here looks good to me.
I asked agent devs if we get r,p value or th by default, but it seems even th is not always propagated by probabilistic samplers. I think there isn't much we can do about this now and the PR makes the situation better, so 👍 from me.
There was a problem hiding this comment.
Looks good overall. If we're planning to upstream this, then I think having the p-value handling in there may be problematic.
The logic should probably be controlled with a config option but that is not done yet.
In case you haven't seen it yet: open-telemetry/opentelemetry-collector-contrib#33632
| // For consistent probablity sampler, calculate the adjusted count based on | ||
| // p-value, negative base-2 logarithm of sampling probability | ||
| var p uint64 | ||
| for _, kv := range otTraceState.ExtraValues() { | ||
| if kv.Key == "p" { | ||
| // If p-value is present then calculate the adjusted count as per | ||
| // the consistent probability sampling specs. | ||
| if kv.Value != "" { | ||
| // p-value is represented as unsigned decimal integers | ||
| // requiring at most 6 bits of information. We parse to | ||
| // 7 bits as 63 holds a special meaning and thus needs | ||
| // to be distinguished w.r.t. other invalid >63 values. | ||
| p, _ = strconv.ParseUint(kv.Value, 10, 7) | ||
| } | ||
| break | ||
| } | ||
| } | ||
| switch { | ||
| case p == 0: | ||
| return 1 | ||
| case p == 63: | ||
| // p-value == 63 represents zero adjusted count | ||
| return 0 | ||
| case p > 63: | ||
| // Invalid value, default to 1 | ||
| return 1 | ||
| default: | ||
| // TODO (lahsivjar): Should we handle fractional adjusted count? | ||
| // One way to do this would be to scale the values in the histograms | ||
| // for some precision. | ||
| return uint64(math.Pow(2, float64(p))) | ||
| } |
There was a problem hiding this comment.
I'm not sure about this...
My understanding is that the old (experimental) p-value/r-value spec is superseded by OTEP 235, which introduces t-value.
If we need to support both, perhaps it should be a separate processor that transforms the old tracestate? We might want to do something similar to handle the Elastic APM tracestate.
There was a problem hiding this comment.
My understanding is that the old (experimental) p-value/r-value spec is superseded by OTEP 235, which introduces t-value.
+1. I didn't know that probabilistic sampling processor already supported the new specs and wanted to continue supporting that processor.
If we need to support both, perhaps it should be a separate processor that transforms the old tracestate? We might want to do something similar to handle the Elastic APM tracestate.
This makes more sense. I have updated the PR to remove the p-value handling in favor of OTEP 235.
Calculates the adjusted count based on OTEP-235. Adjusted count represents the number of spans that a single sampled span represents -- similar to APM's representative count. OTEP-235 is now supported by probabilistic sampler too.
The logic should probably be controlled with a config option but that is not done yet.
Related to: https://github.com/elastic/opentelemetry-dev/issues/378
Also, related to: https://github.com/elastic/opentelemetry-dev/issues/372