Repository navigation
production: First to report served one chain's numbers on every chain tab - #3363
Merged
Merged
Conversation
… same everywhere (#3362) Lucas spotted it on production: bench 001's "First to report" view showed Codex 68.8% and Serialized 29.5% identically on Base, BNB Chain, Solana and Robinhood Chain, while the head-lag view beside it moved per chain as it should. The shares are a per-chain race, so one set of numbers on four chains is not a coincidence, it is an unfiltered query. It was. Two paths build a panel's PromQL and only one of them scoped it: values + 24h series specToBenchmark -> applyDimensionsToSpec -> injectLabels 7d + 30d series /api/series/[slug]?panel=... -> hand-rolled The second exists because slimBenchmarkForCache and the snapshot both strip seriesByProvider7d/30d on purpose, so the chart refetches them on tab click. That route loads the raw spec off disk, never passes it through applyDimensionsToSpec, and injected only the provider selector: avg by (aggregator) (head_lag_first_share_pct{aggregator="codex"}) averaged over every chain and every region. The blob branch above it is correctly keyed by filterSig, but it can never hit for a panel because the series it looks for are the ones that were stripped, so every request fell through to the unscoped build. Bench 001 declares the selector as `{}`, which is what made it invisible in review: it looks filtered and matches everything. Both paths now call one exported panelMetricQuery, so they cannot drift again, and ignore_dimensions is honoured in both rather than in one (bench 282's blank-on-71-of-75-views trap). The route's own cache key moves to v10 because entries written before this hold a cross-chain average under a per-chain key. Ten tests on the shared function. Verified they fail: putting the dropped dimensions back turns 5 of them red, so this is a guard and not a guess. Not a regression from today's Prometheus work. head_lag_first_share_pct comes from the monitor job, which was kept, and no head_lag metric was touched.
Flotapponnier
changed the title
…ther tab (#3364) This is the bug Lucas actually reported, and #3362 was not it. Bench 001's First to report showed the same shares on Base, BNB, Solana and Robinhood while head lag beside it moved per chain. Florent narrowed it to "only 7D and 30D", which is the whole diagnosis: 1H, 6H and 24H render from the embedded per-filter series, and 7D, 30D, 90D and 1Y are lazy-fetched. The lazy caches were keyed by panel id alone: if (lazyPanel7d[activePanelId] && lazyPanel30d[activePanelId]) return; The effects DO list chainProp in their deps and DO re-run on a tab change, and the fetch URL has carried the chain since the main series was fixed for this same reason. It never got there: the first chain opened filled the entry, and every later chain hit that guard and returned before fetching. What it published, measured against the live series with EU-WEST selected: base Serialized 74.7 % the chart named Codex bnb Mobula 64.8 % the chart named Codex robinhood Serialized 93.3 % the chart named Codex solana Codex 73.4 % right, by luck So three chains out of four named the wrong leader, which is worse than the "same numbers everywhere" it looked like. Keyed on panel + chain + region + tier now, through one panelSeriesCacheKey helper, reads and writes together. The 90d/1y writes are included: they had been left on the old key by the first pass, which would have made that cache never hit and refetch on every render. Seven tests on the key, including the two orderings a naive concatenation would collide. Note #3362 is still a real fix and stays: /api/series rebuilds a panel's series from the raw spec whenever the blob misses, and that path dropped the dimensions. It is latent rather than what Lucas saw, because the blob does carry the per-filter panel series and was answering correctly throughout. The PR body claimed otherwise and has been corrected.
Flotapponnier
changed the title
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.
Two commits, both cherry-picked from
dev(#3362 and #3364), shipped together so production takes one deploy rather than two.The reported bug, and its real cause (#3364)
Bench 001's First to report showed the same shares on Base, BNB Chain, Solana and Robinhood Chain, while Head lag beside it moved per chain. Reported by Lucas on production.
The clue that settled it: it only happened on 7D and 30D. 1H, 6H and 24H render from the embedded per-filter series that ships with the page; 7D, 30D, 90D and 1Y are lazy-fetched, and only those were wrong.
The lazy panel caches were keyed by panel id alone:
The effects do list
chainPropin their dependencies and re-run on a tab change, and the fetch URL has carried the chain ever since the main series was fixed for this same reason. It never got that far: the first chain opened filled the entry, and every later chain hit that guard and returned before fetching.What that published, measured against the live series with EU-WEST selected:
Three chains out of four named the wrong leader, which is worse than the "same numbers everywhere" it looked like.
Fixed by keying on panel + chain + region + tier through one
panelSeriesCacheKeyhelper, reads and writes together. The 90d/1y writes are included deliberately: a first pass rekeyed their reads and left their writes on the old key, which would have made that cache never hit and refetch on every render.The latent bug found on the way (#3362)
/api/series/[slug]?panel=...has two ways to answer. The blob branch is keyed byfilterSigand is correct, and it is what was answering throughout, which is how the server looked innocent: nobody was asking it a second time. The fallback below it rebuilds from the raw spec on disk, never passes it throughapplyDimensionsToSpec, and injected only the provider selector:That runs whenever the blob misses for a (slug, filter) pair. The unscoped form returns Codex 43.0 %, and the mean of Codex's twelve chain-by-region cells is 516.5 / 12 = 43.04 %, which confirms the mechanism exactly.
Fixed with one exported
panelMetricQuery(panel, labels)called by both the loader and the route so they cannot drift again.ignore_dimensionsis now honoured in both rather than one.Route cache key to v10: any entry written by the unscoped path holds a cross-chain average under a per-chain key.
Verification
Seventeen tests across the two helpers, and both sets were made to fail first: reinstating the dropped dimensions turns 5 of the server tests red, and the key tests cover the two orderings a naive concatenation would collide.
Gates on this branch:
pnpm test754 pass 0 fail,tsc --noEmitclean,pnpm validate254 specs valid.Not a regression from today's Prometheus cleanup:
head_lag_first_share_pctcomes from themonitorjob, which was kept, and nohead_lagmetric was touched.After merge: confirm the four chain tabs serve four distinct 7d series on the deployed site.