metrics-generator: cap per-series span-metrics CPU with stratified sampling - #7888
Draft
csmarchbanks wants to merge 10 commits into
Draft
csmarchbanks wants to merge 10 commits into
csmarchbanks wants to merge 10 commits into
Conversation
Profiling the span-metrics hot path shows 76% of a push goes to
aggregateMetricsForSpan and 58% to CloseAndBorrowLabels alone (sort,
sanitize, per-label limit, hash, UTF-8 validation), while resolving the
attribute values that decide the series costs only 4%. That gap is what
this exploits: compute a series key from the raw values, decide whether
the span is worth aggregating, and skip the expensive 70% if it isn't.
max_spans_per_series_per_second bounds how many spans per metric series
run the full path each second. Series under the rate are untouched.
Series over it have a uniform sample of their spans aggregated and the
result scaled back up, so no spans are dropped and every estimate stays
unbiased.
The sampling is stratified rather than independent-per-span: a series'
arrivals are cut into equal blocks and exactly one span per block, chosen
uniformly at random, is kept. Every completed block consumes blockSize
spans and contributes exactly blockSize, so calls_total and
latency_count can only be off by the one block in flight -- 1% error at
100 sampled spans, where independent sampling would need 66,000. The
random position within the block is what keeps the estimates unbiased
when arrival order correlates with latency. Bucket shape, and therefore
latency_sum and the quantiles, is multinomial under any scheme, so those
carry a sampling error that shrinks with the budget; the accuracy study
test derives the sample counts each query needs and the docs table
reports them.
Sampling is disabled by default. BenchmarkSpanMetricsSampling, against a
real ManagedRegistry:
off 1/10 kept 1/100 kept
plain 261 ns/span 79 ns/span 51 ns/span
prod 1372 ns/span 270 ns/span 143 ns/span
Assisted-by: Claude Opus 5
The docs blamed the histogram buckets for the sample counts quantiles need, and buckets are the smaller half. Estimating a quantile from m samples pins its rank to about sqrt(phi(1-phi)/m), and converting a rank error to a value error multiplies by the inverse density at the quantile -- thin on a long tail, so one point of rank is 7% of the value at p90 and 48% at p99. A perfect estimator reading every span still needs ~13,000 samples for 5% at p90. Reading the quantile off bucket counts adds to that only when the quantile sits near a bucket boundary, which is exactly what happens at p90 for a sigma=1.29 lognormal (2% into its bucket) and accounts for the whole 13,000 -> 42,000 jump. Mid-bucket it costs nothing: the same shape's p50 needs slightly fewer samples than a perfect estimator, because coarse buckets trade variance for bias like any binned estimator. So native histograms are the fix for the bucket half -- factor-1.1 buckets bring p90 back to ~13,000 -- and nothing fixes the floor except a narrower latency distribution. Both are now called out in the docs, and studyQuantileDecomposition derives the table. Assisted-by: Claude Opus 5
Two corrections to the sampling accuracy guidance. The study labelled a factor-1.1 classic layout "native-like", but a native histogram does not just have finer buckets -- it interpolates exponentially inside them. Read through promql.HistogramQuantile at schema 3, which is what Tempo's default native_histogram_bucket_factor of 1.1 resolves to, the quantization bias is under 0.1% at p50, p90 and p99. The classic default carries 1.5% to 35% depending on where the quantile lands, and no sample budget removes it. If you sample and care about quantiles, native histograms are the mode to be in. The tables also reported only the 99th percentile of the error, which reads as the expected error and is roughly 4x it. They now give the typical window alongside the tail, which is what sizing a budget actually wants. At m=10,000 a schema 3 histogram lands within 1.1% of its own answer at p50, 1.5% at p90 and 3.2% at p99 typically, and the sampled bucket distribution is within a total variation distance of 0.034 of the true one -- a well filled histogram. So m=10,000, a budget of 34 spans/s/series against a 5-minute lookback, is a sensible default target rather than the 100,000 the old table implied. studyQuantileAccuracy derives all of it, including the quantization bias and the shape fidelity. Assisted-by: Claude Opus 5
The sample counts for histogram accuracy were all coming out of simulation, so they only covered the shapes and schemas that happened to be simulated. They have closed forms, and studyBucketFill now checks them against simulation across schemas 2-4 and two latency shapes; every one agrees to within a couple of percent. Writing w = ln(2)/2^schema for the bucket width in log space and sigma for the spread of log-latency, a bucket holding p of the mass collects m*p samples and carries 1/sqrt(m*p) relative error, the busiest holds 0.399*w/sigma, and the whole shape's total variation distance from the truth is (2/pi)^(1/4)*sqrt(sigma/(w*m)). A bucket drops out once its expected count falls under 1, putting the edge of the populated range at sqrt(2*ln(w*m/(sigma*sqrt(2*pi)))) standard deviations -- 3.3 sd, or p99.96, at m=10,000 for a long-tailed service, which is why the p99 quantile degrades sharply below a few thousand samples. The budget is linear in sigma and in 2^schema, so a long tail costs proportionally more and each step of histogram resolution doubles the data needed to fill it. Both are now in the docs along with the formulas, so a budget can be computed for a given service rather than read off a table for someone else's. Assisted-by: Claude Opus 5
…lands on studyTempoHistogramValues drives a real prometheus.Histogram configured the way registry.nativeHistogram configures one, and converts it the way the collect path does, so the answer is in milliseconds from the histogram Tempo would really build rather than in percentages from an idealised one. For a 50ms/1s service at 1000 spans/s over a 5-minute window, true p99 1000ms: unsampled native 1000.0 +/- 8.4 ms 5-95% 989 - 1016 sampled native 1000.0 +/- 45.8 ms 5-95% 935 - 1086 unsampled classic 1014.7 +/- 3.4 ms 5-95% 1009 - 1020 sampled classic 1014.4 +/- 54.1 ms 5-95% 984 - 1159 Doing it against the real histogram surfaced something the idealised model had assumed away. native_histogram_bucket_factor is a request, not a guarantee: max_bucket_number is 100 by default, and a series wanting more buckets than that gets its bucket width doubled instead, landing a schema coarser. The sigma=1.29 shape asks for schema 3 and settles at schema 2 once it has more than a few thousand spans in the window, while a sigma=0.85 shape holds schema 3 up to a million. The earlier docs claimed 9%-wide buckets at the default factor; for a wide service it is 19%. It costs almost nothing in accuracy, since the error at a high percentile is dominated by rank uncertainty rather than bucket width, but the docs should not claim a resolution the series does not get. Assisted-by: Claude Opus 5
…plicas The budget was per replica and counted over an internal 10s window, which was wrong in both directions. Wrong unit: metrics are emitted per collection interval and queried over several of them, so a per-second budget over a window unrelated to either made the samples a query actually sees hard to reason about. The knob is now max_spans_per_series_per_interval, the sampler's window is the registry's collection interval, and a query covering W seconds sees budget * W / interval sampled spans per series. Series under the budget are still never sampled, so this only bites the heaviest ones. Wrong scope: every generator emits its own copy of a series tagged with __metrics_gen_instance, and a query sums them, so giving each replica the full budget handed the query one budget per replica -- 30 generators meant 30x the intended samples and 30x the CPU the sampling was supposed to save. The budget is now fleet-wide. Each instance takes the share matching the share of the tenant's spans it receives, derived from its Kafka partition assignment, and those shares sum to 1 so the local budgets sum to the target. Where the split is unknowable -- single binary, or no partition assignment to read -- an instance keeps the whole budget, which under-samples: CPU rather than accuracy, which is the right way to fail. New in spanmetrics.New as a variadic Option so no existing caller changes. TestSeriesSamplerSplitsBudgetAcrossReplicas checks ten instances sharing a stream keep one budget between them and still recover the fleet's span count, and TestSeriesSamplerIgnoresUnusableShare pins the safe fallback. Assisted-by: Claude Opus 5
The budget is counted per collection interval, so the number that hits a given sample count depends on what that interval is, and the guidance assumed Tempo's 15s default. Grafana Cloud's overrides set it through two helpers in deployment_tools: withMetricsGeneratorLegacyDPM uses 15s, and withMetricsGenerator and withAppO11yMetricsGenerator both use 60s, with no caller overriding either. Roughly three quarters of tenants are on 60s, where a 5-minute query covers 5 intervals rather than 20 and the budget for the same accuracy is 4x larger. Give the rearranged formula and both intervals rather than one worked example against the default. Assisted-by: Claude Opus 5
Two review findings. make generate-manifest writes two files from the same marshalled default config, docs/sources/tempo/configuration/manifest.md and modules/frontend/docs/config-reference.md, and fails if either differs from git. Only the first was updated, so CI would have failed on the second. The new option was also documented inside the Overrides section of configuration/_index.md, where it does not exist -- it is static generator config, not a per-tenant limit. Runtime overrides unmarshal strictly, so an operator copying it out of that section would have hit a hard config load error rather than a silently ignored key. Removed; the copy under Metrics-generator, which is the real home, stays. Assisted-by: Claude Opus 5
Findings from a pre-push review pass, two of them real bugs. A series whose block was sized by a burst and whose rate then collapsed could report nothing for as long as it took that oversized block to fill -- thousands of windows for a big enough burst, far longer than the registry's 15m stale duration, so the series would age out and come back as a counter reset and a spike. rollWindow staged the smaller size but only applied it at a block boundary that would not arrive. A shrinking size now takes effect at the roll, abandoning the block in flight: that is zero-mean and at most one block of count error, and only in a window where the rate fell. TestSeriesSamplerRecoversFromABurst fails without the change. A new series was inserted with lastSeenMs still zero and then triggered the eviction sweep, which deleted it on the spot. trafficShare divided by PartitionsCount, which counts pending and inactive partitions. Those take no writes, so a scale-down inflated the denominator and over-sampled for its duration. Uses ActivePartitionsCount now. SendInterval read only the tenant override, while the registry falls back to the static registry config. A deployment setting collection_interval in YAML with no override would budget per 15s while sending every 60s, spending it four times per send. And the caveat that matters most for anyone turning this on: registry.nativeHistogram applies the multiplier by calling Observe that many times, so the observe work is not saved and only the label building in front of it is. The benchmark now has a native row, which puts the saving at 1.8x against 9.6x for a production-shaped classic config, with allocations barely moving. The docs say so, and note the unmeasured risk that the multiplier loop now runs inside the per-metric mutex. Nothing here is a correctness problem, but "enable native histograms if you sample" needed the other half of the trade. Also adds a concurrency test, since sample() claims to be safe under contention and nothing exercised it, and trims the uniformity test from 18M sampler calls to 3M. Assisted-by: Claude Opus 5
![semgrep-code-grafana[bot]](https://avatars.githubusercontent.com/u/7195757?s=60&v=4){kind=small}
| package spanmetrics | ||
|
|
||
| import ( | ||
| "math/rand/v2" |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
Do not use math/rand. Use crypto/rand instead.
Why this might be safe to ignore:
The rule correctly matched the math/rand/v2 import, but this code uses randomness only to sample telemetry spans and does not generate secrets, tokens, or security-sensitive values. crypto/rand would not provide a meaningful security benefit for this statistical sampling use case.
To resolve this comment:
💡 Follow autofix suggestion
Suggested change
| "math/rand/v2" | |
| "crypto/rand" |
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by math-random-used.
We're currently testing semgrep's diff-aware PR comment feature on a subset of our repos-- if you run into issues or find this spammy, please reach out to @danny.cooper in slack and give feedback.
For backwards compatability with gosec, its best to use polyglot suppression comments of the following format for false positives:
// #nosec <gosec rule ID> nosemgrep: <semgrep rule ID>
You can view more details about this finding in the Semgrep AppSec Platform.
check-fmt runs gofumpt and goimports, not plain gofmt, and it runs before golangci-lint in the same CI job -- so the formatting failure was also hiding five lint findings. Fixed together: - gofumpt wanted blank lines between the one-line override methods in the sampling benchmark. - gosec G404 on the per-shard generator seeding, and semgrep's math-random-used on the import. Both are false positives: the draw picks which span in a block to keep, which is a choice about what telemetry to aggregate, not a secret or anything an attacker gains from predicting, and it is on the per-span hot path where a CSPRNG would cost far more than it could protect. gosec flags the use site and semgrep the import, so both are annotated, with the reasoning at the import. - ineffassign on a named return that every path overwrote, and an unconvert on an int-to-int conversion, both in the accuracy study. - unparam on feedSampler and currentBlockSize, whose key argument was always the same series. Replaced with a named constant that says why the choice of key does not matter. Assisted-by: Claude Opus 5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
span_metrics.max_spans_per_series_per_intervalcaps how many spans per metric series run the full aggregation path in each collection interval. Series under the budget are untouched; series over it get a uniform sample of their spans aggregated and the result scaled back up, so no spans are dropped and the metrics stay unbiased. Disabled by default.Why it can be cheap
76% of a push is
aggregateMetricsForSpanand 58% isCloseAndBorrowLabelsalone — sort, sanitize, per-label limit, hash, UTF-8 validation. Resolving the values that decide a span's series is only 4%. So the key is built from raw values, the decision made, and the expensive rest skipped.Design
Per-labelset rather than a global reservoir: accuracy is a per-series property, and a reservoir samples every span at one rate, starving low-share series however large it is.
Within a series it is stratified — arrivals are cut into blocks of
blockSizeand exactly one span per block, at a uniformly random position, is kept and multiplied byblockSize. A completed block consumes and contributes exactlyblockSize, which makes counts near-exact:calls_total/latency_countrate, stratifiedlatency_sumand the quantiles do carry sampling error;TestSamplingAccuracyStudyderives it.m = 10,000holds a native histogram's p50/p90/p99 within 1.1%/1.5%/3.2% typically.The budget is fleet-wide. Every generator emits its own copy of a series tagged
__metrics_gen_instanceand a query sums them, so each instance takes the share matching its share of the tenant's spans, read from its partition assignment. Where that is unknowable the instance keeps the whole budget, which under-samples — CPU rather than accuracy.Sizing
For
m = 10,000over a 5-minute lookback: a budget of 500 at a 15scollection_interval, or 2,000 at 60s.Performance —
BenchmarkSpanMetricsSampling, against a realManagedRegistryThe native row is what I would most like a second opinion on.
registry.nativeHistogram.updateSeriesapplies a multiplier by callingObservethat many times, since client_golang has no weighted observation — so the observe work is not saved, only the label building in front of it. That pulls against the accuracy guidance to prefer native histograms when sampling; both halves are now in the docs. Unmeasured: the loop runs inside the mutex covering every series of that metric, so a sampled series holds it forblockSizeiterations. A weighted native observe would fix both, but needs an upstream change.Not included
No per-tenant override, no metric for how hard sampling is biting, and only the partition-assignment path gets a replica split. The key is built pre-sanitization, so this will not help the high-cardinality span-name case the drain sanitizer exists for, and sampler memory is bounded by distinct keys per eviction window rather than by the active-series limit.
Testing
go test ./modules/generator/...and-race.TestSeriesSampler*cover the block state machine, the count guarantee, pick uniformity, rate adaptation, burst recovery, concurrency, and the fleet-wide split.TestSpanMetricsSamplingPreservesTotalsdrives the whole processor.TestSamplingAccuracyStudy(gated onTEMPO_SAMPLING_STUDY=1, ~15s) derives every number in the docs.🤖 Generated with Claude Code