feat(sdk): collect exemplars for explicit-bucket histograms - #3624
Open
ormeilu wants to merge 2 commits into
Open
feat(sdk): collect exemplars for explicit-bucket histograms#3624ormeilu wants to merge 2 commits into
ormeilu wants to merge 2 commits into
Conversation
Implements exemplar collection for the explicit-bucket histogram aggregation, behind the new `spec_unstable_metrics_exemplars` feature. The `Exemplar<T>` data structure and the `exemplars` fields on data points already existed; nothing populated them. A measurement recorded inside a sampled span is now retained alongside the histogram data point together with its trace id and span id, so a backend can link from a bucket to a representative trace. Eligibility is decided by the new public `ExemplarFilter` (`AlwaysOn`, `AlwaysOff`, `TraceBased`), configured with `SdkMeterProvider::builder().with_exemplar_filter(..)` and defaulting to `TraceBased` per the specification. Sampling uses the spec's `AlignedHistogramBucketExemplarReservoir`, which keeps at most one exemplar per bucket; the bucket index it keys on is the one the histogram already computes while precomputing its value, so no extra work is done to find it. Cost is contained by design: the filter is resolved once per instrument rather than per measurement, `AlwaysOff` returns before touching thread-local storage, and the reservoir lives inside the mutex the histogram aggregator already holds, so no new synchronization is added. With the feature disabled every exemplar type is a zero-sized stand-in with empty inlined bodies, leaving the measurement path unchanged.
Carrying `Option<ExemplarOffer>` by value through `ValueMap::measure` widened the histogram's `PreComputedValue` from 16 to ~72 bytes, and that move was paid on every measurement — including the overwhelmingly common one where no exemplar is offered at all. It cost 9.2% on `Histogram_Record` with the feature enabled. Boxing the offer makes the ineligible case a null pointer. The allocation now happens only for measurements that are actually going to be sampled, which allocate an `Exemplar` regardless. Histogram_Record, against main as baseline: feature off 159.32 ns -0.48% (no change, p = 0.41) feature on 162.25 ns +1.50% (was +9.24% unboxed)
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
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.
Closes #3369
What this does
Exemplar<T>and theexemplarsfields on data points already existed in the SDK, and the read accessors were already public — but nothing ever populated them. Every emit site hardcodedexemplars: vec![]. This adds the collection half for the explicit-bucket histogram.A measurement recorded inside a sampled span is now retained alongside the histogram data point together with its trace id and span id, so a backend can link from a latency bucket straight to a representative trace instead of leaving you to hunt by time range.
Everything is behind a new
spec_unstable_metrics_exemplarsfeature, following the precedent ofspec_unstable_metrics_views. With the feature off, the measurement path is unchanged.Spec conformance
Against the metrics SDK spec, Exemplars section:
ExemplarFilter— all three variants (AlwaysOn,AlwaysOff,TraceBased), defaulting toTraceBased.TraceBasedchecks the sampled flag, not merely the presence of a span.AlignedHistogramBucketExemplarReservoir— at most one exemplar per bucket, uniformly weighted (reservoir sampling with k=1, so every measurement a bucket has seen is equally likely to be the survivor). This is the spec's default reservoir for an explicit-bucket histogram with more than one bucket.Reservoirs are drained on every collection, in both delta and cumulative temporality — an exemplar describes the interval it was sampled in, so holding one across cycles would keep re-exporting a stale trace id.
Why this is a contained change
Three things in the existing design made this smaller than it looks, and they're worth naming because they're the reason the diff doesn't sprawl:
ValueMap::measure, so there were no call sites to chase.PreComputedValueis(T, usize)— value and bucket index. The aligned reservoir keys on exactly that index, so it is handed the key rather than recomputing it.Mutex<Buckets<T>>the aggregator already locks for the counter update, andclone_and_resetalready implements the drain-and-reset cycle a reservoir needs.Reading ambient trace context on a per-record path is also established practice in this SDK rather than something new — the logs pipeline does it in
logs/logger.rs. And #1076 ("Remove Context from sync instruments") anticipated this explicitly in its own description: "The SDK could infer the current implicit context OR we could introduce a newctr.AddWithContext()." This takes the implicit-context path.Performance
This is the part that matters for review, so numbers up front.
cargo bench --bench metrics_histogram, baseline ismainat 8eeafe0.Histogram_Recordmain(baseline)TraceBased, no active spanCost per filter, isolated (single static attribute, so a cheaper workload than the row above — compare these three to each other, not to the table):
AlwaysOffAlwaysOffTraceBased(no active span)AlwaysOnThree deliberate choices keep this flat:
ExemplarSampleris built at instrument creation.AlwaysOffreturns before touching thread-local storage. It pays a predictable branch on an enum discriminant and nothing else.Option<ExemplarOffer>by value widenedPreComputedValuefrom 16 to ~72 bytes and cost 9.2% — paid on every measurement, including the common one where nothing is offered. Boxing makes the ineligible case a null pointer; the allocation happens only for measurements actually being sampled, which allocate anExemplaranyway. That's the second commit, kept separate so the before/after is visible.AlwaysOnis genuinely more expensive — it builds an offer and reads the clock on every measurement by definition. That is inherent to what the filter asks for, and it is not the default.With the feature disabled, every exemplar type is a zero-sized stand-in with empty inlined bodies, so the optimizer removes the machinery entirely.
Scope, and what I deliberately left out
Per CONTRIBUTING's guidance on PR size and on starting with a smaller surface, this covers the explicit-bucket histogram only. The scaffolding (
ExemplarFilter, the sampler, the plumbing throughAggregateBuilder) is general; wiring the remaining aggregations is mechanical follow-up:Increment<T>andAssign<T>are lock-free atomics. A reservoir needs a mutex, so unlike the histogram this is not free, and it deserves its own PR with its own benchmark rather than being smuggled in here.SimpleFixedSizeExemplarReservoirat sizemin(20, max_buckets). Straightforward; omitted to keep this reviewable.PrecomputedSum, observable gauges) — the spec scopes trace id and span id to synchronous instruments, and there is no meaningful ambient context inside a callback. These continue to emit empty exemplar lists.I'm happy to fold any of these in if you'd rather land it as one piece — say the word and I'll extend this PR.
Diff size
736 insertions, but roughly 330 of those are tests (unit tests for filter and reservoir behaviour, two end-to-end tests through the meter provider) and 35 are the new benchmark. Production code is ~370 lines.
Checks
cargo fmt --allcleancargo clippy -p opentelemetry_sdk --all-targets --all-features -- -Dwarningscleancargo test -p opentelemetry_sdk --all-features --lib— 404 passed, 0 failed--no-default-features,metrics,spec_unstable_metrics_exemplars(no SDKtracefeature),exemplars + views + bound_instruments,--all-features, and--workspace --all-featuresallowed-external-types.tomlneeds no change —ExemplarFilterexposes no external typesTests added
Filter behaviour for all three variants; trace and span id match the active sampled span; nothing recorded under an unsampled span or with no span; at most one exemplar per bucket across all four buckets; exemplars do not leak across collection cycles in delta or cumulative temporality; and two end-to-end tests proving
with_exemplar_filteractually reaches the histogram aggregation.