fix(loader): label loader_cache with the block key, not the resolve chain - #1220
Open
nicacioliveira wants to merge 1 commit into
Open
fix(loader): label loader_cache with the block key, not the resolve chain#1220nicacioliveira wants to merge 1 commit into
nicacioliveira wants to merge 1 commit into
Conversation
…hain
`loader_cache` and `resolver_latency` were labelled with `ctx.resolverId`, which
carries the full resolve chain:
Categories@sections.variants.1.value.5.sections.0.section.page
SearchResults Global@sections.0.section.page
Categories@sections.variants.1.value.2.jsonLD
Every section position, of every variant, of every page becomes its own time
series. That is an identifier, not a dimension.
Measured on the production ClickHouse:
distinct `loader` values, 3h window
fila-store 3,684
todolivrooficial 3,478
montecarlo 2,209
farmrio 1,469
happybooksoficial 1,028
fleet total 21,849
Against the budget documented in our own o11y guide — >1,000 per site is
forbidden as an alert dimension and >100 fleet-wide as a cross-fleet
aggregation — five sites are over and the fleet figure is 218x over. The cost
is real: `loader_cache` is 11,485,954 of 14,998,196 rows in otel_metrics_sum
over 3h, i.e. 76.6% of the whole table, and that table holds 48.4 GB.
For contrast, `cache_hit` covers 194 tenants in 9 series, and the
@decocms/start runtime does the equivalent job with 13 distinct
`deco.cache.profile` values fleet-wide.
The fix uses the block key that `adapt` already receives — the loader's module
path, e.g. `vtex/loaders/legacy/productListingPage.ts` — which is bounded by
the number of loader modules in the app. It is threaded into `wrapLoader` as a
new parameter; `wrapLoader` is module-private with a single call site.
What is lost: the metric can no longer distinguish two instances of the same
loader sitting in different sections. That trade is deliberate. The chain is
still carried on error logs, where high cardinality is acceptable because they
are read by point lookup rather than aggregated — and the o11y guide draws
exactly that line: high-cardinality attributes are fine for filtering and point
lookups, forbidden as aggregation keys.
`ctx.resolverId` is kept as a fallback so behaviour is unchanged if a caller
ever passes an empty key.
Verified: deno check blocks/loader.ts clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthrough
ChangesLoader metric labels
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@blocks/loader.ts`:
- Around line 218-232: Separate the metric label from the cache identity in the
loader flow: keep the bounded block key for the metric’s loader value, but use
ctx.resolverId for ctx.vary?.push and the resolver query parameter when
constructing the cache URL. Update the relevant cache-key handling around
ctx.vary and cache URL generation without changing the existing fallback
behavior for metrics.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+218
to
+232
| // Metric label. Deliberately the block key and NOT `ctx.resolverId`: | ||
| // resolverId carries the full resolve chain, e.g. | ||
| // `Categories@sections.variants.1.value.5.sections.0.section.page`, so | ||
| // every section position of every variant of every page becomes its own | ||
| // time series. Measured in production that reached 3,684 distinct values | ||
| // on a single site and 21,849 across the fleet, against a documented | ||
| // budget of 1,000 per site and 100 fleet-wide — and `loader_cache` alone | ||
| // became 76.6% of all rows in otel_metrics_sum. | ||
| // | ||
| // The block key is bounded by the number of loader modules in the app, | ||
| // which is what the @decocms/start runtime already uses for the | ||
| // equivalent metric (13 distinct values fleet-wide). The chain is not | ||
| // lost: it still travels on error logs, where high cardinality is fine | ||
| // because they are read by point lookup rather than aggregated. | ||
| const loader = blockKey || ctx.resolverId || "unknown"; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the resolver chain for cache identity.
Line 232 changes loader, but this value is also used by ctx.vary?.push and as the resolver query parameter in the cache URL at Lines 282-305. Two instances of the same loader can therefore collide when their cacheKeyValue values match.
Keep the block key for metric labels, but retain ctx.resolverId for cache identity.
Suggested fix
const loader = blockKey || ctx.resolverId || "unknown";
+ const cacheResolver = ctx.resolverId || "unknown";
...
- ctx.vary?.push(loader, cacheKeyValue);
+ ctx.vary?.push(cacheResolver, cacheKeyValue);
...
- resolver: loader,
+ resolver: cacheResolver,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@blocks/loader.ts` around lines 218 - 232, Separate the metric label from the
cache identity in the loader flow: keep the bounded block key for the metric’s
loader value, but use ctx.resolverId for ctx.vary?.push and the resolver query
parameter when constructing the cache URL. Update the relevant cache-key
handling around ctx.vary and cache URL generation without changing the existing
fallback behavior for metrics.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="blocks/loader.ts">
<violation number="1" location="blocks/loader.ts:232">
P1: Cached loader calls now collide across distinct resolve chains because `loader` is used as the cache identity as well as the metric label, so a loader with the optional/default cache key can serve one section or page's result to another. Keep a separate block-key label for metrics and retain `ctx.resolverId` for the cache URL and vary key.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // equivalent metric (13 distinct values fleet-wide). The chain is not | ||
| // lost: it still travels on error logs, where high cardinality is fine | ||
| // because they are read by point lookup rather than aggregated. | ||
| const loader = blockKey || ctx.resolverId || "unknown"; |
Contributor
There was a problem hiding this comment.
P1: Cached loader calls now collide across distinct resolve chains because loader is used as the cache identity as well as the metric label, so a loader with the optional/default cache key can serve one section or page's result to another. Keep a separate block-key label for metrics and retain ctx.resolverId for the cache URL and vary key.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At blocks/loader.ts, line 232:
<comment>Cached loader calls now collide across distinct resolve chains because `loader` is used as the cache identity as well as the metric label, so a loader with the optional/default cache key can serve one section or page's result to another. Keep a separate block-key label for metrics and retain `ctx.resolverId` for the cache URL and vary key.</comment>
<file context>
@@ -208,7 +215,21 @@ const wrapLoader = (
+ // equivalent metric (13 distinct values fleet-wide). The chain is not
+ // lost: it still travels on error logs, where high cardinality is fine
+ // because they are read by point lookup rather than aggregated.
+ const loader = blockKey || ctx.resolverId || "unknown";
const start = performance.now();
let status: "bypass" | "miss" | "stale" | "hit" | undefined;
</file context>
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.
Closes #1219.
Problem
loader_cacheandresolver_latencyare labelled withctx.resolverId, which carries the full resolve chain:Every section position, of every variant, of every page becomes its own time series. That is an identifier, not a dimension.
Measured
Distinct
loadervalues on the production ClickHouse, 3h window:Our own o11y guide forbids >1,000 distinct values per site as an alert dimension and >100 fleet-wide as a cross-fleet aggregation. Five sites are over; the fleet figure is 218× over.
The cost is not theoretical:
loader_cacheis 11,485,954 of 14,998,196 rows inotel_metrics_sumover 3h — 76.6% of the whole table — and that table holds 48.4 GB.For contrast,
cache_hitcovers 194 tenants in 9 series, and the@decocms/startruntime does the equivalent job with 13 distinctdeco.cache.profilevalues fleet-wide (vtex/productDetailsPage,vtex/relatedProducts,site/categoryTree).Fix
Use the block key that
adaptalready receives — the loader's module path, e.g.vtex/loaders/legacy/productListingPage.ts— bounded by the number of loader modules in the app. Threaded intowrapLoaderas a new parameter;wrapLoaderis module-private with a single call site, so nothing external changes.ctx.resolverIdis kept as a fallback, so behaviour is unchanged if a caller ever passes an empty key.The trade-off, stated plainly
This loses the ability to distinguish two instances of the same loader in different sections. If you are debugging "which section is slow", the metric will no longer answer that — it will tell you which loader is slow.
That is deliberate, and it is the line the o11y guide already draws: high-cardinality attributes are fine for filtering and point lookups, forbidden as aggregation keys. The chain still travels on error logs, which are read by point lookup rather than aggregated.
If the per-section breakdown turns out to be load-bearing for someone, the alternative is to keep the chain as a span attribute and use the key for the metric — happy to switch to that shape instead.
Note for reviewers
This changes an existing label's values. Any dashboard or alert currently grouping by
loaderwill see its series collapse from thousands to dozens after deploy — better, but not a silent change. Historical data keeps the old labels, so charts spanning the deploy will show both.Verification
deno check blocks/loader.ts— clean. There is no existing test file forblocks/loader.ts; the change is a label substitution with a fallback, exercised by every loader call.🤖 Generated with Claude Code
Summary by cubic
Label
loader_cacheandresolver_latencywith the loader’s block key (module path) instead of the resolve chain to cut metric cardinality and keep dashboards within our o11y budget. Metrics now group by loader, not per-section instance.Bug Fixes
blockKeyas theloaderlabel; fall back toctx.resolverIdif empty.blockKeyparam towrapLoader; no external API changes.@decocms/startbehavior and reduces series count inotel_metrics_sum.Migration
loaderwill collapse from thousands of series to dozens.Written for commit 67ea0b3. Summary will update on new commits.
Summary by CodeRabbit