feat(observability): attach trace exemplars to the duration histograms (MAPCO-11496) - #24
Open
NivGreenstein wants to merge 27 commits into
Open
NivGreenstein wants to merge 27 commits into
NivGreenstein wants to merge 27 commits into
Conversation
newHttpHandler registered its four families with MustRegister, which panics on a duplicate. The registry is process-wide, so a second observer — a second SetObservability call, or a test that builds one of its own — registered the same families again and took the process down. newCache has always gone through registerOrReuse for exactly this reason, and its comment describes this case; this constructor was simply missed. Nothing reached it before because only one observer is built per process in practice and only one test in server/ constructed one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A latency spike in Grafana is now one click from the trace that caused it. The cache, per-tier and HTTP duration observations carry the active trace and span as a Prometheus exemplar, so a slow bucket names the request that landed in it. Three decisions worth the reader's time. Sampling is consulted, which is the opposite of what the log handler does with the same span context (MAPCO-11494). A trace id on a log line still groups a request's lines whether or not Tempo received the trace; an exemplar has no such consolation use, and prometheus keeps one exemplar per bucket, overwritten by the next observation to land there. Unfiltered, at the default sample_ratio of 0.01, the stored exemplar would be a dead link roughly 99 times in 100. The span id rides along with the trace id. The tracing wrappers are installed outside the metric ones at both seams, which two earlier tickets established deliberately and neither could yet make observable — with the span carried, a slow bucket on the per-tier histogram names the tier read that was slow rather than the request as a whole. The label names are the log record's own constants: both surfaces are wired up separately in Grafana and both fail silently when the name is wrong. The metrics route now negotiates OpenMetrics, without which none of the above is scrapeable: OpenMetrics is the only exposition format that encodes exemplars and the classic text format drops them silently. That has a cost, and it is a breaking change for dashboards — under OpenMetrics a bucket boundary that looks like an integer gains a trailing ".0", so le="1" becomes le="1.0" and the series identity changes. Affected: 1, 2.5 and 5 on the cache families, 1, 5 and 10 on the HTTP one. Anything pinning an exact le has to be checked. Handler also takes a pointer receiver like every sibling, clearing a vet copylocks finding on the line being rewritten. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wrapping order at both instrumentation seams — tracing outside metrics — was established by MAPCO-11497 with a comment at each site saying trace exemplars would need it. Nothing depended on it until now, and inverting either one leaves the span tree unchanged, so the existing span assertions would keep passing. Both new tests fail on that inversion, and say which way round it went wrong: the per-tier exemplar reports the cache-wide span instead of the tier's, and the request exemplar disappears altogether because the metrics middleware observes a request whose context has no span yet. atlas.TestCheckCacheTypes gained a comment rather than a change. It asserts the exact set of registered cache types, so it is sensitive to the fake types every other test file in the package registers — and it passes only because those files all sort after it. This work hit that: a file named cache_exemplar_test.go failed it, and the same file renamed did not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tracing/README.md gains "Correlating metrics with traces" beside the logs section it mirrors, and says where the two deliberately differ: sampling is consulted for exemplars and not for log records, because an exemplar is only ever a link and prometheus keeps one per bucket — so an unfiltered one would point at a dropped trace 99 times out of 100. The observer's README carries the operator half: which families carry an exemplar and which span each names, and the three things outside this repo that each fail quietly on their own — the scrape format, exemplar storage on the server, and the Grafana datasource link. Both record the two limits. Exemplars are not pushed, so a push_url deployment gets none; and negotiating OpenMetrics respells integer-looking le boundaries with a trailing ".0", which changes those series' identity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The observer README is now the source for the docs site metrics sections, and docs/tracing.md gained a metrics half. Neither had a row, so a change to exemplar behaviour had no documented docs-site counterpart to update. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three test files had grown the same walker — gather, match the family, match the labels, find the first bucket with an exemplar, flatten its labels. internal/ttools is where this package's own precedent puts it: MetricFamilyNames is there for the same reason, and says so. The two atlas tests become one table, since they differed only in the family and the span expected. Only the per-tier row can catch the inversion, so only it checks for it. The le respelling was documented wrong in four places, and wrong in the direction that understates an upgrade break. expfmt appends ".0" only when the shortest 'g' rendering contains neither "." nor "e", so 2.5 was never affected — and the *response-size* families are, because the format is negotiated per scrape rather than per family. TestRespelledBucketBoundaries derives the list from the bucket sets so the docs cannot drift from them again. Also: metricsHandler moves next to Handler, its only caller, rather than sitting in the exemplar file; the server test matches the handler label exactly instead of on a "/11/" substring; and http.go points at registerOrReuse's doc rather than restating it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hed write It failed 4 runs in 6 on origin/development, reporting shigola_cache_hits_total as a family tracing had published. The cause is in its own warm-up: it made two requests against an empty cache expecting the first request's write to make the second a hit, but every cache write goes through the bounded pool off the response path, so the second request races that write. When it lost, both requests missed, the hit families were never published before the snapshot, and the traced request got the blame. The warm-up is now one request against a durable tier that was seeded directly — the hot tier still misses, so the hit and miss families are both published, with nothing asynchronous in the path. 8 runs in 8 green. Seeding needs the cache key, which is easy to get subtly wrong: the tile row and column are the other way round from X and Y, and a wrong key would seed a tile nothing reads and reintroduce the race silently. So the key is asserted against what a tier is actually asked for, and swapping X and Y fails the test by name rather than by flaking. Pre-existing and unrelated to exemplars; fixed here because it makes this branch's CI red for reasons that are not this branch's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Report for CI Build 81Coverage increased (+1.3%) to 56.993%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Three docs said a push_url deployment sends "the classic text format to a Pushgateway, which has no notion of exemplars". Both halves are wrong as stated: push.New defaults to expfmt.FmtProtoDelim and nothing here overrides it, and protobuf does carry exemplars — (*histogram).Write fills in dto.Bucket.Exemplar. They now say what is actually verifiable from this tree: the exemplars go out on the wire, what the Pushgateway does with them is its own business and untested here, and push_url is documented for ephemeral jobs rather than the serving path. A comment and a README also pointed at atlas.TestTierExemplarNamesTheTierSpan, which 26bae12 renamed. Rationale comments are load-bearing here, so a pointer to a test that does not exist is worse than none. ttools.ExemplarLabels now takes the newest exemplar by timestamp rather than the first bucket carrying one. That was a genuine flake, not a tidy-up: prometheus keeps one exemplar per bucket, a family on the process-wide registry outlives the test that observed into it, and the two rows of the atlas table both observe the whole-cache family — so under -race, where the spread between two observations is wide enough to put them in different buckets, a row could read its sibling's trace. It failed 1 run in 6; 10 in 10 after, and both ordering mutation checks still fail as they should. Also from the review: the untraced HTTP path had no assertion, though the two paths attach their exemplar through entirely different machinery, so the cache half proved nothing about it; the respelled-le table becomes one tcase map in the repo's usual shape rather than two parallel maps; the atlas table dispatches on a field instead of comparing a family name inside the shared closure; and spanForTier and rootSpan move to internal/faketracer, which already owns the span lookups. Left alone deliberately: the root-span walk in tracing_test.go builds three tallies in one pass, and pulling one out would fragment it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment on metricsHandler enumerated the boundaries in prose alongside the two READMEs and the test that derives them — and the previous version of that same comment is where the wrong "2.5" claim survived longest. It now says what the change is and points at the test and the README for which boundaries it touches. ttools.Histogram becomes HistogramSample: it reads as a constructor but returns one label-matched sample. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng the rule The test reimplemented expfmt's unexported writeOpenMetricsFloat and said it "reproduces" it — which put the docs one vendored change away from being wrong with the test still green, and the copy had silently dropped that function's NaN and ±Inf cases. It now registers each bucket set, scrapes it under both exposition formats, and reports the boundaries the two spell differently. Nothing here knows the rule, so a change to it fails by naming the boundary that moved. The four rows are unchanged, which is independent confirmation that the hand-derived table was right. go.mod: client_model moves to the direct require block. internal/ttools now imports it, making this the only first-party importer, and `go mod tidy` in a container agrees — that one line, nothing else, and tidy is a no-op after. Also: the two "outside a trace" tests had identical bodies, now one helper asserting both halves of the criterion — nothing attached, and the observation still recorded. And ttools' comment claimed a gatherer parameter existed because such tests "usually want a registry of its own", while two of three callers pass the default one; it now says why both are needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tants TierSpan's comment claimed two callers "atlas asserting a tier exemplar, atlas asserting the tier span tree". It has one: tracing_test.go wants every tier name at once for a set comparison, not one span by name, so it cannot use this and never will. RootSpan's invoked "the propagation tests", which find their roots inside a loop that also tallies names and trace ids, and which a previous review round talked me out of splitting. Both now say one caller, and why they sit here anyway. The atlas and server tests read exemplar["trace_id"] as a literal while the whole rationale for exemplar.go's constants is that one spelling serves both correlation surfaces. They now go through log.TraceIDKey and log.SpanIDKey — the same constants the exemplar labels are built from, so a rename moves the assertion with the code. tracing/README.md carried a second in-repo copy of the respelled-le table. It now points at the test that derives it and the observer README that states it, keeping the two counterintuitive parts as prose. The "63 of 128 runes" figure it quotes is asserted rather than logged, so the number cannot go stale. getLabels reads as a function; it is readLabels. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hasLabels was extracted into ttools and left one caller short: atlas's own counter() still inlined the same nested label match. It is now ttools.HasLabels, and counter is twenty lines shorter. The four duration-exemplar tests were a 2x2 — cache and HTTP, each traced and untraced — written as four functions in a package whose documented pattern is a table keyed by name. They are one table now, which also made it obvious that only the trace id was being asserted; both rows check the span too, and each row fails on the mutation it should. Handler took its registerer from the package default while the observer holds exactly that field. It reads its own now, so an observer built against another registry serves that one, with the nil guard its siblings all have — the pointer receiver taken to clear a vet finding is also a receiver that can be nil. scrapeLabels returns le values rather than labels; it is scrapeLE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three comments in server/tracing_test.go named seedDurableTier and seededTwoTierCache. Neither exists: the second was written and inlined again in the same change, and the comments were left behind. This is the second time this branch has done it, and 89d5334's own message is the argument against it — "a pointer to a test that does not exist is worse than none". 9b617be claimed "an observer built against another registry serves that one". It did not: the registerer followed obs.registry while the gatherer stayed the package default, so such an observer would have registered its scrape counter into one registry and served another's metrics — worse than not following it at all. The gatherer now comes off the registerer, which is a *prometheus.Registry in every case this has, and that type is both. The exemplarTraceIDKey/exemplarSpanIDKey aliases were a middle man: this package's tests asserted through them while atlas and server used log.TraceIDKey directly, so "one spelling serves both correlation surfaces" was half true. The aliases are gone and the comment explaining the coupling now sits at the use site. ttools gains AssertExemplar, which is the trace-and-span compare pair that was written out in three files. Its failures name the family, and all three packages still fail when the span id is dropped. CONTRIBUTING.md said "`master` is always the most recent state of the code, and pull requests are opened against it". Since 2026-09-07 master is the rewound branch and development is the trunk, so it was telling contributors to open pull requests against a branch with no history — and it says so three lines above the mapping table this branch already edits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docs framed the OpenMetrics break as le-only, over shigola's four families. It is a property of the encoder, not of histograms: expfmt writes summary quantile labels through the same float formatter. So go_gc_duration_seconds — a Go runtime metric this package never touches and every Go process publishes — has quantile="0" respelled to "0.0" and quantile="1" to "1.0". A dashboard pinned to either was affected and the docs did not say so. TestRespelledQuantileBoundaries scrapes a real Go collector under both formats rather than reasoning about the encoder, the way the bucket list is already derived. Both READMEs and docs/tracing.md now say the break reaches past le and past this project. Handler nil-guards obs.registry as well as obs: New always sets it, but the value receiver this replaced could not have been nil either way, so the guard now covers both ways the zero value can arrive. CONTRIBUTING.md's trunk section is taken back off this branch. Correcting it was right — it told contributors to open pull requests against the rewound master — but it is its own piece of work, and the docs repo had already had exactly this done to it here (d8a8d5c). Only the docs-mapping rows, which this ticket does create, stay. The correction follows as its own branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
observeDuration is wired into Get, Set and Purge, and the docs name all three spans, but every exemplar test observed a read. The table now has a write and a purge row. The write matters most, and it had no coverage at all: a detached write runs on the pool goroutine after its response is gone, and keeps its trace only because WritePool derives the context with WithoutCancel. TestDetachedWriteExemplar- NamesItsRequest asserts that, and it took two wrong shapes to get right — worth recording, because both looked correct and proved nothing. Asserting the whole-cache family passes with WithoutCancel removed: the detachment decorator sits inside the observability wrapper, so that observation is made synchronously on the response path where the live context is still in hand. Then asserting the tier exemplar against the tier span's own trace also passes, because a write that lost its context still gets a span — the root of a new orphaned trace, which an exemplar naming it still matches. The assertion has to be against the *request's* trace, taken from the whole-cache span. It now fails on the mutation, naming both traces. TierSpan takes the span name, so it can find a write as well as a read. Also from the review: one differ shared by both respelling tests rather than the comparison written twice, and the exposition formats are named constants instead of a bare bool. TestRespelledQuantileBoundaries uses a summary carrying the Go collector's objectives rather than the collector itself, whose constructor is deprecated in the vendored client and whose replacement is in a package this tree does not vendor — registering it to prove a fact about the encoder would have meant vendor churn on a ticket whose criteria turn on vendor/ being untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…istry Handler's own comment rules out serving a mismatched pair — "registering its scrape counter into one registry while gathering another's metrics would be worse than not following it at all" — and then the code did exactly that whenever the Gatherer assertion failed: the registerer followed obs.registry while the gatherer fell back to the package default. Either half alone is wrong, so fall back to both defaults or to neither. The assertion holds for every registry New can produce, so nothing observable changes; what changes is that the branch which does run no longer contradicts the paragraph above it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s helper TestExemplarReachesTheExposition called metricsHandler directly, which is the unexported helper rather than the entry point server.go mounts. The encoding is chosen in Handler, so the test asserted the exemplar reached an exposition nothing serves: replacing Handler's body with a plain promhttp.Handler() — the default, which leaves EnableOpenMetrics off — dropped every exemplar on the wire with the whole suite still green. It now goes through Handler and fails that mutation on the Content-Type, which is also the first thing that makes the observer's registry field load-bearing: it is what lets the production entry point be exercised against this test's own registry instead of the process-wide default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both overstate their evidence, which is the failure mode this branch has already had to fix twice in comments. observability/prometheus/README.md said TestRespelledQuantileBoundaries "scrapes a real Go collector". It does not, deliberately: the collector's constructor is deprecated in the vendored client and its replacement is not vendored, so the test carries the collector's own objectives on a summary instead. Say that, and why. tracing/README.md said the two ordering tests "say which way round it went wrong". Only the atlas one does — a tier exemplar can go wrong exactly one way, so it names the inversion. The server one reports that no bucket carries an exemplar at all, because a request observed outside its own span has nothing to point at. Both fail; they do not fail alike. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It was private to package prometheus, which quietly bounded what it could check to the bucket sets declared there — while the docs it backs describe every family the process publishes. provider/postgis declares its own duration buckets, so they were invisible to a test whose comment said it derived the list from the bucket sets. Shared here so the next family declared outside the observer can be pinned without copying the differ, which is how the docs got a hand-written list in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… table Serving OpenMetrics respells 1, 5 and 20 on shigola_mvt_provider_sql_query_seconds and shigola_provider_sql_query_seconds, changing the identity of every le series a dashboard pins on them. Neither family appeared in the documented list, which therefore understated the upgrade break for anyone running the PostGIS provider. Pinned rather than asserted: the boundaries move from a local inside Collectors to a package-level var so TestRespelledQueryBuckets can derive the row the same way the other four are derived. No database — what is under test is how the encoder writes the numbers, which matters because every other test in that package is gated behind RUN_POSTGIS_TESTS. tracing/README.md also claimed only the size histograms and counters go without an exemplar. These two are duration histograms, are observed inside their own query span, and still carry nothing: exemplarFrom lives in the Prometheus observer, and a provider importing it would defeat the noPrometheusObserver build tag. Recorded with the reason, not as an oversight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…efault registry The branch that serves the default registry because the configured one cannot gather did it without a word. It is unreachable for anything New produces, so it guards against a panic rather than a real case — but silently serving a different registry's metrics than the caller configured is the same class of failure the exemplar label constants are commented against, and it presents as an inexplicably empty /metrics. Also moves ctx to the first parameter of the two closure builders in the exemplar table, per the context package's own convention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mapping table is what makes a source change reach its docs page, so a page deriving from a source with no row is a page that goes stale silently. docs/http-endpoints.md now describes the metrics route's exposition format, which observability/prometheus.metricsHandler owns, and had no row at all. docs/logging.md gained a paragraph deriving from tracing/README.md's metrics-correlation section, while its row named only the logs one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tating the push path Two corrections to the same section. The break is written up under trace exemplars in both files, which reads as though it follows from them. It does not: metricsHandler sets EnableOpenMetrics on every metrics route and never consults the tracing config, so an operator running the observer with tracing off — the default — gets the renamed series and no exemplars at all. tracing/README.md also carried the pushed-metrics mechanism near-verbatim from the observer's README. That paragraph is the one whose earlier prose copy asserted the opposite of what the code does and had to be corrected in three places at once, so it now lives in one place and is linked from the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…comparison The unchanged list names the boundaries that surprise by not moving, and its loop ran after AssertRespelled — which fails fatally on a length mismatch. A documented-unchanged boundary that starts moving *is* a length mismatch, so the specific message could only ever appear if some other boundary stopped moving in the same change. Reordered, so it says which boundary. Also drops the six per-row metric prefixes in TestDurationExemplars. They date from before each row got its own registry and now imply an isolation requirement that is not there; the comment says where the requirement is real, which is the atlas and server tests on the default registry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Correcting claims this branch added two commits ago. The family is constructed and returned as a collector, and nothing in the tree calls Observe on it: its layer_name label belongs to the feature-returning postgis provider removed in MAPCO-11487, and mvt_postgis records against shigola_mvt_provider_sql_query_seconds instead. A HistogramVec with no observed child emits no family, so it never reaches a scrape. So three statements were wrong. It was listed in the respelled-le table, which told operators to re-check series it has none of. It was named as a duration histogram "observed inside its own query span", which it is not. And the docs-site page said its latency was readable as that span's duration, when there is no such span. The mvt family's row is correct and stays — it is observed, and 1, 5 and 20 do move. The dead one is now annotated where it is documented rather than removed: deleting a registered collector is a separate change from describing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot a regexp TierSpan's comment said "one caller today"; it gained a second when the detached-write test was written and the comment did not follow. AssertExemplar said "three tests in three packages" — three packages is right, four call sites is the count, because atlas uses it twice. Respelled took a *regexp.Regexp and the package exported two compiled patterns to feed it. Both callers want one label's values, so it takes the label name and builds the pattern itself: the package stops promising it can differ on an arbitrary expression, and the call sites read Respelled(t, reg, "quantile"). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit dropped shigola_provider_sql_query_seconds from the respelled table and left the sentence under it plural, still saying the provider *families* carry no exemplars and their le labels move regardless. One of them has no le labels, because it publishes no series. HistogramSample's comment claimed three tests in three packages, the same stale count corrected on AssertExemplar one commit ago. It has one external caller and one in-file one; the rationale now says what it actually shares. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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 MAPCO-11496.
A latency spike in Grafana is now one click from the trace that caused it. The cache, per-tier and HTTP duration observations carry the active trace and span as a Prometheus exemplar.
The vendored client already had everything needed —
ExemplarObserveron histograms,promhttp.WithExemplarFromContextfor the handler — so no dependency moved.Three decisions worth a reviewer's attention
Sampling is consulted, unlike the log records. MAPCO-11494 deliberately logs ids for valid-but-unsampled traces. This does the opposite, and the asymmetry is the point: an exemplar is only ever a link, and Prometheus keeps one exemplar per bucket, overwritten by the next observation to land there. So the stored exemplar is almost always the most recent observation — and at the default
sample_ratio = 0.01the most recent observation is almost never sampled. Attaching them unfiltered would make clicking a bucket open nothing roughly 99 times out of 100. A log line's trace id keeps its consolation use when the trace was dropped (it still groups that request's lines); an exemplar has none.This is a defensible reading of "attach the active trace ID … when one exists" rather than the literal one, and it is what the ticket's Expected Result actually requires.
The span id rides along with the trace id. Both prior tickets put the tracing wrapper outside the metric wrapper at both seams, each with a comment saying trace exemplars would need it — and with trace_id alone that ordering would make no observable difference at all, since the request span and the tier span share a trace. Carrying the span makes it load-bearing: a slow bucket on the per-tier histogram names the tier read that was slow, on the one family whose whole purpose is telling tiers apart. 63 of the 128 runes the client allows.
The label names are
internal/log.TraceIDKey/SpanIDKey, the log records' own constants. Logs-to-traces and metrics-to-traces are configured separately in Grafana and both fail silently on a wrong name; one pair of constants stops a rename from fixing one surface and breaking the other.The metrics route now negotiates OpenMetrics, and without this the whole change would have had no observable effect: OpenMetrics is the only exposition format that encodes exemplars, and the classic text format drops them without a word.
promhttp.Handler()'s defaultHandlerOptsleaves it off.The cost of that, and it is a breaking change
Under OpenMetrics a boundary that renders as a whole number is written with a trailing
.0, and a label value is part of a series' identity — sole="1"becomesle="1.0", a different series.shigola_cache_duration_seconds,..._tier_...1,5shigola_api_duration_seconds1,5,10shigola_cache_response_size_bytes,..._tier_...1024,5120,25600,102400,256000,512000shigola_api_response_size_bytes512000Two things there are worth reading twice, and I got both wrong on the first pass.
2.5is not affected —expfmtappends.0only when the shortest'g'rendering contains neither.nore, and2.5already has one; the megabyte boundaries are likewise untouched, rendering as1.048576e+06. And the response-size families are affected, despite carrying no exemplars, because the format is negotiated once per scrape rather than per family.TestRespelledBucketBoundariesnow derives this list from the bucket sets, so the docs cannot drift from them again. Anything pinning an exactleneeds checking. The alternative was recording exemplars nobody could scrape.Two latent defects this surfaced, both fixed separately
newHttpHandlerpanicked on a second observer (085570a). It registered its four families withMustRegister, which panics on a duplicate; the registry is process-wide, so a secondSetObservability— or a test building its own observer — was fatal.newCachehas always usedregisterOrReuseand its comment describes this exact case; this constructor was simply missed. The new server test is what reached it. Note the tradeoff this inherits:AlreadyRegisteredErroris raised on descriptor equality, and buckets are not part of a descriptor, so two same-named histograms with different buckets are now silently reused rather than panicking. That is the choiceregisterOrReusealready documents for the cache families; this makes HTTP consistent with it rather than introducing it.TestTracedRequestPublishesNoNewMetricsfailed 4 runs in 6 onorigin/development(19f1044), reportingshigola_cache_hits_totalas a family tracing had published. Its warm-up made two requests against an empty cache expecting the first's write to make the second a hit — but writes are detached through the bounded pool, so the second request raced that write. It now makes one request against a directly seeded durable tier: the hot tier still misses, so hit and miss families are both published, with nothing asynchronous in the path. 8 runs in 8 green, and the seeded key is asserted against the key a tier is actually asked for, since the tile row and column are the other way round from X and Y and a wrong key would reintroduce the race silently.Verified
-raceat-count=1, 29 packages, clean.CGO_ENABLED=0build clean.gofmt -sclean outsidevendor/.go vetclean on every touched package — one fewer finding than before, sinceHandlerwas passing a lock by value.atlas.instrumentTiersfailsTestExemplarNamesTheSpanThatMeasuredIt/the_tier_readwithtier exemplar names the cache-wide span; the metric wrapper is outside the tracing one— and only that row, since a whole-cache exemplar naming the cache span is correct. Invertingserver.NewRouterfailsTestRequestExemplarNamesTheRequestSpanwithno bucket … carries an exemplar. Neither inversion changes the span tree, so the pre-existing span assertions keep passing — these are the only tests that hold the order.TestExemplarReachesTheExpositionscrapes the real handler with Prometheus's ownAcceptheader and finds the exemplar in the bytes. That header's version list matters: the vendoredexpfmtnegotiates OpenMetrics0.0.1only, so a scraper offering1.0.0alone silently gets the classic format. Prometheus offers both.TestExemplarFitsTheRuneLimitmakes a realObserveWithExemplarcall rather than counting runes by hand, because the limit counts names and values together and the failure mode is a panic.Left out, deliberately
push.Newdefaults toexpfmt.FmtProtoDelimand nothing overrides it, and protobuf does carry exemplars —(*histogram).Writefills indto.Bucket.Exemplar. So they go out on the wire; whether a Pushgateway stores and re-exposes them is its business and is not tested here.push_urlis documented for ephemeral jobs anyway, not the serving path. (My first draft of these docs claimed push used the classic text format and dropped exemplars — wrong on the mechanism and asserting a conclusion this tree cannot check. Corrected in 89d5334.)--enable-feature=exemplar-storage(Mimir its equivalent), and the Grafana Prometheus datasource needs an exemplar link ontrace_idtargeting Tempo. Both documented with the config; worth a check by whoever owns those.One flake of my own, found and fixed
ttools.ExemplarLabelsoriginally returned the exemplar on the first bucket carrying one. Prometheus keeps one exemplar per bucket, and a family on the process-wide registry outlives the test that observed into it — so with both rows of the atlas table observing the whole-cache family, a row could read its sibling's trace whenever the two observations landed in different buckets. Under-racethat spread is wide enough: it failed 1 run in 6. It now takes the newest by timestamp; 10 in 10 after, and both ordering mutation checks still bite.Also found on the way
atlas.TestCheckCacheTypesasserts the exact set of registered cache types, so it is sensitive to the fake types every other test file in the package registers — and it passes only because those files all sort after it. A new file namedcache_exemplar_test.gofailed it; the same file renamed did not. Left as a comment on that test rather than a change to it.Docs: MapColonies/shigola-docs#16
🤖 Generated with Claude Code