Split/borrowed labels followups#7584
Draft
carles-grafana wants to merge 5 commits into
Draft
Conversation
Duplicate histogram_buckets still panicked prometheus.NewHistogram in hybrid (both) mode because its monotonicity check rejects equal bounds, and emitted duplicate le series for classic histograms. Compact the sorted buckets in both constructors; in the classic histogram compact after appending +Inf so a user-supplied +Inf does not duplicate the synthetic bucket. Also split the changelog entry into enhancement (borrowed labels) and bug_fix (bucket normalization) per review.
The observe, collect, and stale-removal paths all take the full seriesMtx, so the remaining atomics (count, sum, buckets, lastUpdated) guarded nothing the mutex does not already guard, and made it unclear which synchronization layer is load-bearing. Converting them to plain values also saves len(buckets)+3 allocations per new series. counterSeries and gaugeSeries keep their atomics: their read paths run under RLock.
Replace the three *WithHashAt methods (IncWithHashAt, ObserveWithExemplarTraceIDBytesWithHashAt, SetForTargetInfoWithHashAt) with IncBorrowed/ObserveBorrowed/SetForTargetInfoBorrowed taking a *BorrowedLabels. The labels+hash pair now travels as one value computed by CloseAndBorrowLabels, so the 'hash MUST equal lbls.Hash()' precondition (previously documented three times and verified at runtime by TestRegistry's assertLabelHash, which distorted processor benchmarks) is enforced by construction and the scaffolding is deleted. CloseAndBorrowLabels returns a pointer into the pooled labelBuilder itself, keeping the steady-state borrow path at 0 allocs/op (verified: 253ns/op, 0 B/op) — returning a value and passing its address through the interface methods would have forced a per-span heap escape. Release clears the fields before pooling the builder so a future borrower never races with a stale holder.
spanKindString and statusCodeString hand-roll the proto enum names to avoid Span_SpanKind.String's map lookup on the per-span hot path. Assert they match the generated String() for every value in the proto name maps, and fall back to it for unknown values, so a tempopb update cannot silently diverge. Follow-up to grafana#7498 review.
Post-review follow-ups from grafana#7498. Readability: replace the bare 16 with initialLabelCapacity and explain the sizing/grow behaviour, and document that nativeHistogramSeries.lastUpdated is advanced by updateSeries with the caller-supplied timestamp and read by removeStaleSeries under seriesMtx. Release hardening: Release is now a no-op on the nil returned by a failed CloseAndBorrowLabels, so a defer set up before checking ok (the recommended resource pattern) cannot panic on invalid-UTF-8 span input. Its doc no longer promises unconditional idempotency: Release is call-exactly-once, and the repeated-call no-op only holds until the pooled builder is re-issued. A new concurrent borrow/release churn test pins the clear-before-Put ordering under -race (a reordering regression was verified to fail it). Native histograms now honor the interface contract "empty trace ID = no exemplar": observations with an empty (or all-zero, which hex-encodes to "") trace ID no longer attach a junk traceID="" exemplar, matching classic histograms, which drop empty exemplars at collect time. Bucket normalization: extract a normalizeBuckets helper shared by both histogram constructors, additionally dropping NaN buckets (NaN defeats slices.Compact and panics prometheus.NewHistogram in both mode), and warn from ManagedRegistry.NewHistogram when normalization changes the configured buckets so operators learn about the misconfiguration. Also pin the proto enum cardinalities in the enum-drift test so added values must extend the fast-path switches, and note the multi-variable caveat on the borrowed-path benchmark comparison.
carles-grafana
force-pushed
the
split/borrowed-labels-followups
branch
from
8c7cfac to
f46346e
Compare
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 this PR does:
Which issue(s) this PR fixes:
Fixes #
Checklist
.chloggen/(runmake chlog-new, ormake chlog-new FILENAME=<name>to override the default branch-name file; see.chloggen/README.md)