Support OTel TraceState when determining the span multiplier

[csmarchbanks]

What this PR does:
OTel TraceState is moving along with support in the probabilistic sampler as well as some languages (mainly Java with Go coming soon). If the ot key inside the W3C tracestate contains a threshold (th), then we know exactly what the sampling rate is, even if the trace has passed through other processors before reaching Tempo.

This PR adds a new configuration variable that will first check the tracestate for the ot key and try to parse the threshold. If that does not succeed it will fall back to looking for a configured attribute.

More details can be found in the OpenTelemetry documentation: https://opentelemetry.io/docs/specs/otel/trace/tracestate-probability-sampling/.

Which issue(s) this PR fixes:
Fixes #2684, which is already closed but could be reopened...

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[cla-assistant]

All committers have signed the CLA.

[github-actions]

💻 Deploy preview deleted (Support OTel TraceState when determining the span multiplier).

[javiermolinar]

@codex review

[javiermolinar]

Have you considered using

sampling.NewW3CTraceState(traceState)

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/a423a8a644e0d71ac1a43f8da9858ca5965ae296/pkg/sampling/w3ctracestate.go#L106

This implementation can produce false positives (and negatives)

ie:
myot=...

[csmarchbanks]

Good catch. My initial commit has the sampling.NewW3CTraceState(traceState) implementation, but it is significantly slower than manual parsing, according to the benchmark about twice as slow as manual parsing, and 10x slower than the attribute lookup. I have updated the manual parsing to handle the vendor ending with ot case (including a couple tests). Since this will be ran on every span I thought the performance improvement would be worthwhile. That said, I can go back to sampling.NewW3CTraceState(traceState) if the slow performance is ok.

[javiermolinar]

This should be an user-configurable override as the SpanMultiplierKey so we can keep it disabled by default and enabled per tenant

tempo/modules/overrides/user_configurable_overrides.go

Lines 348 to 353 in b378c2e

	func (o *userConfigurableOverridesManager) MetricsGeneratorProcessorServiceGraphsSpanMultiplierKey(userID string) string {
	if spanMultiplierKey, ok := o.getTenantLimits(userID).GetMetricsGenerator().GetProcessor().GetServiceGraphs().GetSpanMultiplierKey(); ok {
	return spanMultiplierKey
	}
	return o.Interface.MetricsGeneratorProcessorServiceGraphsSpanMultiplierKey(userID)
	}

[csmarchbanks]

👍 added another commit that implements this.

[javiermolinar]

@csmarchbanks there is a changelog conflict preventing the gate checks to start

[csmarchbanks]

Rebased!

[javiermolinar]

This has the same issue as before:

not=th:8,ot=th:c This will find not first and returns a multiplier of 8

Wdyt about something simpler like splitting first by comma to get all the = pairs and then look for the key ot:

  func getSpanMultiplierFromTraceState(span *v1.Span) float64 {
     traceState := span.GetTraceState()
     if traceState == "" {
        return 0
     }

     for _, member := range strings.Split(traceState, ",") {
        member = strings.Trim(member, " \t")

        key, value, ok := strings.Cut(member, "=")
        if !ok || key != "ot" {
           continue
        }

        otts, err := sampling.NewOpenTelemetryTraceState(value)
        if err != nil {
           return 0
        }
        return otts.AdjustedCount()
     }

     return 0
  }

[csmarchbanks]

That's what the if a bit below is for, it checks that the index is either at the beginning of the string or right after a comma. I added a test case for your example and it appropriately returns a count of 4. That said, my solution does fail if there is whitespace between commas...

I did try the split option a bit ago as well, on my computer it is about halfway between the manual parsing and using the upstream function in terms of time, but also adds an allocation.

I have pushed a commit with one more attempt. This time it jumps to the next value of a comma and ensures that after trimming space that the strings starts with ot=. So basically the same as your solution but avoiding the extra allocation caused by strings.Split. Hopefully that is a bit more understandable! I also added a fuzz test to capture that any time we should parse ot= according to the upstream implementation that our solution also works.

Benchmarks of using upstream, split, and the manual solution:

goos: linux
goarch: amd64
pkg: github.com/grafana/tempo/modules/generator/processor/util
cpu: AMD Ryzen 9 9950X 16-Core Processor
                                             │ /tmp/parse-full-trace-state.txt │            /tmp/split.txt            │       /tmp/manual-parsing.txt       │
                                             │             sec/op              │    sec/op     vs base                │   sec/op     vs base                │
GetSpanMultiplier/with_otel_tracestate-32                          56.60n ± 0%    40.21n ± 1%  -28.96% (p=0.000 n=10)   27.05n ± 1%  -52.22% (p=0.000 n=10)
GetSpanMultiplier/without_otel_tracestate-32                      47.260n ± 1%   20.110n ± 2%  -57.45% (p=0.000 n=10)   6.131n ± 1%  -87.03% (p=0.000 n=10)
GetSpanMultiplier/otel_racestate_disabled-32                       2.342n ± 0%    2.174n ± 1%   -7.19% (p=0.000 n=10)   2.338n ± 1%        ~ (p=0.066 n=10)
geomean                                                            18.44n         12.07n       -34.54%                  7.292n       -60.45%

                                             │ /tmp/parse-full-trace-state.txt │            /tmp/split.txt            │         /tmp/manual-parsing.txt         │
                                             │              B/op               │    B/op     vs base                  │    B/op     vs base                     │
GetSpanMultiplier/with_otel_tracestate-32                          0.00 ± 0%     16.00 ± 0%        ? (p=0.000 n=10)      0.00 ± 0%         ~ (p=1.000 n=10) ¹
GetSpanMultiplier/without_otel_tracestate-32                      32.00 ± 0%     16.00 ± 0%  -50.00% (p=0.000 n=10)      0.00 ± 0%  -100.00% (p=0.000 n=10)
GetSpanMultiplier/otel_racestate_disabled-32                      0.000 ± 0%     0.000 ± 0%        ~ (p=1.000 n=10) ¹   0.000 ± 0%         ~ (p=1.000 n=10) ¹
geomean                                                                      ²               ?                      ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

                                             │ /tmp/parse-full-trace-state.txt │         /tmp/split.txt         │         /tmp/manual-parsing.txt         │
                                             │            allocs/op            │ allocs/op   vs base            │ allocs/op   vs base                     │
GetSpanMultiplier/with_otel_tracestate-32                         0.000 ± 0%     1.000 ± 0%  ? (p=0.000 n=10)     0.000 ± 0%         ~ (p=1.000 n=10) ¹
GetSpanMultiplier/without_otel_tracestate-32                      1.000 ± 0%     1.000 ± 0%  ~ (p=1.000 n=10) ¹   0.000 ± 0%  -100.00% (p=0.000 n=10)
GetSpanMultiplier/otel_racestate_disabled-32                      0.000 ± 0%     0.000 ± 0%  ~ (p=1.000 n=10) ¹   0.000 ± 0%         ~ (p=1.000 n=10) ¹
geomean                                                                      ²               ?                ²               ?                       ² ³
¹ all samples are equal
² summaries must be >0 to compute geomean
³ ratios must be >0 to compute geomean

[javiermolinar]

Yeah, I see its much slower. To be honest the metrics generator its not in the hot path so that's not that much of a problem but I appreciate your efforts to make this faster. :)