Conversation
… trackCache Implements the approved RFC (vtex/stable-storefronts-specs#8): DiagnosticsMetrics only exposes push-based instruments (recordLatency, incrementCounter, setGauge), so metrics.trackCache() and metrics.addOnFlushMetric() — both pull-based — have had no replacement. Apps migrating off MetricsAccumulator keep those two calls untouched (see vtex/render-server#861), which blocks ever removing the legacy API from apps that use them. Adds three public methods, reaching the OTel SDK through metricsClient.getProvider() — already part of DiagnosticsMetrics' declared type, and the same access node-vtex-api already uses for HostMetricsInstrumentation in service/telemetry/client.ts. No new dependency. - registerObservableGauge(name, observe, options?) / registerObservableCounter(...): generic pull-based instruments, for anything read on a schedule rather than pushed per request (queue depth, pool size, addOnFlushMetric replacements). Re-registering a name replaces the previous callback on the same instrument, matching the precedent in vtex/faststore-cloud's lib/diagnostics.ts. - trackCache(name, cacheInstance): direct replacement for the legacy trackCache, accepting the same LRUCache/DiskCache/LRUDiskCache/ MultilayeredCache instances already in use. Emits io_app_cache_operations_total, io_app_cache_items_current, io_app_cache_capacity and io_app_cache_disposed_total. The one subtlety: these caches' getStats() resets hits/total/disposedItems on every read (so the legacy flush-based model wouldn't double-count between flushes). The RFC's approved decision was corte seco — no dual-write path, no non-resetting read added to the cache classes — so trackCache reads each cache exactly once per collection cycle, via a single addBatchObservableCallback covering all four instruments, and folds each delta into a running cumulative total before observing it (an ObservableCounter must report the cumulative value; the SDK derives the delta itself). Reading the same cache from both the legacy trackCache and this one would split its counts between them — replace, don't add alongside. Also fixed while building this: pending registrations. DiagnosticsMetrics' underlying client initializes asynchronously, but apps call trackCache/ registerObservableGauge/registerObservableCounter synchronously at module load — frequently before that initialization finishes. Registrations made before the client is ready are queued and replayed once it is; this is the one existing method (initMetricClient) this change touches, and it's a no-op for any app that never calls the new APIs — the existing 34 tests still pass without needing to know it exists, since their mock client has no getProvider() at all and it's simply never called. Everything else about this change is additive: new imports, a new exported TrackedCache interface, new private fields initialized in the constructor alongside the existing ones, and new methods appended to the class. No existing method's behavior changes. hitRate is intentionally not republished — it's derivable from io_app_cache_operations_total, and publishing a pre-computed ratio prevents correct aggregation across instances. A Grafana/alert search across ~1,565 dashboards and ~1,769 alert rules (done during the RFC) found no consumer of hitRate/itemCount/disposedItems to preserve. Verified: tsc clean, tslint at baseline (0 errors), 51/51 tests in DiagnosticsMetrics.test.ts (17 new), 223/223 across the full repo suite. trackCache's tests run against a real MeterProvider + MetricReader rather than a mock, specifically to catch the single-read-per-cycle and delta-to-cumulative accounting a mock could get "passing" while still wrong. Docs: added a Cache Metrics (Observable) section to METRICS_CATALOG.md and a Pattern 5 to METRICS_OVERVIEW.md, cross-referenced with the existing Pattern 4 (which is the different, already-supported push-based idiom — hand-rolled incrementCounter/setGauge calls at the point a cache is read, not a registered callback). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![arnica-github-connector[bot]](https://avatars.githubusercontent.com/in/144692?s=60&v=4){kind=small}
![dk-pr-review[bot]](https://avatars.githubusercontent.com/in/4406719?s=60&v=4){kind=small}
There was a problem hiding this comment.
DK Review — Deferred (Sonar gate)
⏭️ LLM review not run: this PR's Sonar quality gate is currently failing.
DK Review holds the AI review until Sonar passes, so review budget is not spent on a PR that already needs changes. Fix the issues Sonar reported and push again — the full review runs automatically once Sonar is green.
⚠️ Governance warnings: Large PR detected (4 files) — review coverage may be incomplete
This PR is not blocked by DK Review; a human engineer can still approve and merge manually.
DK Review v1.0.0
Two things this branch was getting called out for: 1. registerObservableGauge and registerObservableCounter, plus their pending- registration handling, were four near-identical methods and four separate Maps. ObservableGauge and ObservableCounter are the same OTel type (Observable) — createObservableGauge/createObservableCounter only differ in which one they call. Collapsed into one registerObservable/ syncObservables pair keyed by 'gauge' | 'counter'; the two public methods are now one line each. Fixed a bug this surfaced: syncObservables() must check whether there's anything registered for that kind before calling getObservableMeter() — calling it unconditionally (as the merged flushPendingObservables did at first) reintroduces the exact "calls getProvider() even when unused" bug already fixed for the cache path, just on the gauge/counter path instead. 2. JSDoc had grown a multi-paragraph block with an @example per method. Cut to what the existing methods in this file already do: one to a few lines, only the part that isn't obvious from the signature. Also: fixed an unsafe format string flagged on the PR — string-interpolating a variable into console.error's first argument. Passed it as a separate argument instead. Net effect: DiagnosticsMetrics.ts's diff against master goes from +379 to +276 lines; the test file from +337 to +285, after also merging the gauge/counter tests that were exercising the same code path twice. Verified: tsc clean, tslint at baseline, 48/48 in DiagnosticsMetrics.test.ts (down from 51 — 3 tests merged, not dropped: coverage is the same code paths, fewer near-duplicate assertions), 220/220 across the full repo suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
DK Review — Deferred (Sonar gate)
⏭️ LLM review not run: this PR's Sonar quality gate is currently failing.
DK Review holds the AI review until Sonar passes, so review budget is not spent on a PR that already needs changes. Fix the issues Sonar reported and push again — the full review runs automatically once Sonar is green.
⚠️ Governance warnings: Large PR detected (4 files) — review coverage may be incomplete
This PR is not blocked by DK Review; a human engineer can still approve and merge manually.
DK Review v1.0.0
…further Two things from review: 1. TrackedCache was a byte-for-byte copy of MetricsAccumulator's GetStats interface. Exported GetStats instead and made TrackedCache a type alias for it, removing the duplicate declaration. Checked for other reuse opportunities across the repo — grepped for any existing instrument- registry/getOrCreate utility and for any other code already using getProvider()/createObservableGauge/createObservableCounter/ addBatchObservableCallback outside this file. Found none; this is genuinely new capability, not a rewrite of something that existed. 2. Every private method had gained its own JSDoc block. The file's own existing private methods (getBaseAttributes, createLatencyHistogram, mergeAttributes) carry zero to one inline comment, not a doc block — matched that. Public methods keep a short comment, not the @param/@example treatment the three original public methods have; that fuller style makes sense for the three flagship methods, less so repeated across eight more. Also fixed a comment that overclaimed precedent: it said this reaches the MeterProvider "the same access node-vtex-api uses for HostMetricsInstrumentation", but that call site uses .provider() (a different, wider type), not .getProvider(). Same underlying object at runtime, different method — worth being accurate about rather than implying an existing pattern that isn't quite there. Caught along the way: an editing artifact left an unterminated /** in getObservableMeter's comment (a leftover open token from an earlier trim that never got its matching close deleted). Compiled and tested clean regardless — worth flagging since "tsc passed" doesn't rule out this class of mistake if the malformed comment happens to swallow only comment-like content, but it's exactly the kind of thing worth double-checking by hand after several consecutive edits, which is how this was caught. DiagnosticsMetrics.ts's diff against master: 379 -> 276 -> 227 lines across the three rounds of feedback on this branch. Verified: tsc clean, tslint at baseline, 48/48 in DiagnosticsMetrics.test.ts, 220/220 across the full repo suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
DK Review — Deferred (Sonar gate)
⏭️ LLM review not run: this PR's Sonar quality gate is currently failing.
DK Review holds the AI review until Sonar passes, so review budget is not spent on a PR that already needs changes. Fix the issues Sonar reported and push again — the full review runs automatically once Sonar is green.
⚠️ Governance warnings: Large PR detected (5 files) — review coverage may be incomplete
This PR is not blocked by DK Review; a human engineer can still approve and merge manually.
DK Review v1.0.0
getStats() reports a per-flush window and consumes it on read — a legacy of MetricsAccumulator publishing metrics as log lines. That makes the read mutating, so two readers of the same cache split its counts between them. Internal counters are now plain monotonic totals and getStats() computes its own delta against a snapshot of what it last reported, so its contract is unchanged: same windowed values, same hitRate. getCumulativeStats() reports the lifetime total and never resets, letting an observable reader and the legacy flush read the same cache independently. Also adds the first tests for these four classes, covering both contracts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…gistration trackCache() now reads getCumulativeStats() instead of getStats(). This drops the running-total bookkeeping that existed only to undo the reset-on-read, and removes the footgun it left behind: migrating a cache no longer requires *replacing* the legacy call, so the two can run side by side while the migration is validated. Three fixes to the observable instruments: - A name already registered as the other kind is refused with an error log. The SDK accepts two same-named streams of different types silently and the collector then rejects them. - Attributes reported by an observable callback are held to the same MAX_CUSTOM_ATTRIBUTES limit as the push-based methods. - The operations counter is skipped for an object with no hit/total counters, instead of emitting two zero-valued series. Reverts the now-unneeded export of MetricsAccumulator's GetStats. Tests: the production delta temporality, a cache registered before the client is ready, a real cache shared with the legacy flush, and the inert-if-unused guarantee (spied before construction, so it covers initialization too). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pattern 5 said to replace the legacy trackCache call rather than add alongside it. That is no longer true, and running both is now the recommended way to validate a migration. Pattern 6 maps addOnFlushMetric onto registerObservableGauge/Counter — the other legacy API with no documented replacement. The translation is not mechanical: a flush metric returns one object with arbitrarily many fields, an instrument reports one value, so the pattern covers both shapes. Also notes that io_app_cache_capacity is in the cache's own units, and declares @opentelemetry/sdk-metrics and context-async-hooks, which the tests import but which resolved only as hoisted transitive deps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
DK Review — Deferred (Sonar gate)
⏭️ LLM review not run: this PR's Sonar quality gate is currently failing.
DK Review holds the AI review until Sonar passes, so review budget is not spent on a PR that already needs changes. Fix the issues Sonar reported and push again — the full review runs automatically once Sonar is green.
⚠️ Governance warnings: Large PR detected (11 files) — review coverage may be incomplete
This PR is not blocked by DK Review; a human engineer can still approve and merge manually.
DK Review v1.0.0
Sonar S2933 on the three fields this PR adds. The two pre-existing fields right above them (counters, gauges) have the same issue and are marked too, so the block stays consistent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…servable-cache-metrics
There was a problem hiding this comment.
DK Review — Deferred (Sonar gate)
⏭️ LLM review not run: this PR's Sonar quality gate is currently failing.
DK Review holds the AI review until Sonar passes, so review budget is not spent on a PR that already needs changes. Fix the issues Sonar reported and push again — the full review runs automatically once Sonar is green.
⚠️ Governance warnings: Large PR detected (11 files) — review coverage may be incomplete
This PR is not blocked by DK Review; a human engineer can still approve and merge manually.
DK Review v1.0.0
Pattern 5 told the reader to keep the legacy call "during validation" and compare the two metrics. There is nothing to compare: since #676 (v7.4.2) nothing consumes what the legacy flush returns — statusTrack() keeps running only for the reset side effects. What the side-effect-free read actually buys is that a half-migrated app still reports correct numbers, which matters across 23 call sites in three apps. Under a read that consumed the counters, registering a cache in both places would have leaked an arbitrary fraction of its counts into a flush that discards them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
0 New Issues
2 Fixed Issues
0 Accepted Issues
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.
Implements the approved RFC: vtex/stable-storefronts-specs#8, with one decision revisited — see Departure from the RFC below.
What is the purpose of this pull request?
Adds three public methods to
DiagnosticsMetrics:registerObservableGauge(name, observe, options?)/registerObservableCounter(name, observe, options?)— generic pull-based instruments, for values read on a schedule rather than pushed per request (queue depth, pool size, etc). Re-registering a name replaces the previous callback on the same instrument, matching the precedent in vtex/faststore-cloud'slib/diagnostics.ts.trackCache(name, cacheInstance)— the direct replacement for the legacyMetricsAccumulator.trackCache(). Accepts the sameLRUCache/DiskCache/LRUDiskCache/MultilayeredCacheinstances already in use. Emitsio_app_cache_operations_total,io_app_cache_items_current,io_app_cache_capacity,io_app_cache_disposed_total.Both reach the OTel SDK through
metricsClient.getProvider()— already part ofDiagnosticsMetrics' declared type, and the same access node-vtex-api already uses forHostMetricsInstrumentationinservice/telemetry/client.ts. No new dependency. The meter name matches the one the existing push instruments already use, so the scope stays consistent.And adds
getCumulativeStats()to the four cache classes — see below for why.What problem is this solving?
DiagnosticsMetricsonly exposes push-based instruments (recordLatency,incrementCounter,setGauge).metrics.trackCache()andmetrics.addOnFlushMetric()are pull-based and have had no replacement, so apps migrating offMetricsAccumulatorkeep both calls untouched (see vtex/render-server#861) — which blocks ever removing the legacy metrics API from an app that uses either.Departure from the RFC: corte seco
The RFC's open question was what to do about the cache classes'
getStats()consuming the counters it reads — a legacy ofMetricsAccumulatorpublishing metrics as log lines in windows. Its approved answer was corte seco: don't touch the cache classes, and accept that reading a cache from two places splits its counts.This PR does not do that, and the reason got sharper once #676 came into view.
#676 (merged 2026-08-05, released in 7.4.2) removed the status log write.
statusTrack()still runs and still callsgetStats()on every registered cache, resetting its counters — but the return value is now discarded. The comment it left behind:So under corte seco, an app that registered a cache in both places would leak an arbitrary fraction of its counts into a reader that throws them away — with no second metric anywhere to reveal the discrepancy, because the legacy output reaches no backend. Not "each sees about half", as the RFC put it: one plausible-looking, permanently wrong number.
That is the argument. Two consequences:
For the same reason, this PR does not claim you can validate the new metric against the legacy one — there is nothing to compare against. What you get is that leaving the legacy call in place is harmless.
What's here instead: the four cache classes' internal counters are now plain monotonic totals, and
getStats()computes its own delta against a snapshot of what it last reported. Its external contract is unchanged — same windowed values, samehitRate, pinned by tests.getCumulativeStats()reports the process-lifetime total and has no side effects, so the observable reader and the legacy flush read the same cache independently.Cost:
+85/-56across 6 files — but-11net inDiagnosticsMetrics.ts, because the running-total bookkeeping that existed only to undo the reset (cacheCumulativeand the whole delta→cumulative folding block) is gone. Migrating a cache is now additive:A side effect worth naming: since #676 the legacy cache metric reaches nothing, so this PR restores cache observability rather than preserving it. The RFC's problem statement assumed the former; corrected in its v1.3.
This also gives the four cache classes their first tests — they had none.
hitRateis intentionally not republished — derivable fromio_app_cache_operations_total, and publishing a pre-computed ratio would prevent correct aggregation across instances. A Grafana search across ~1,565 dashboards and ~1,769 alert rules (done while writing the RFC) found no consumer ofhitRate/itemCount/disposedItemsto preserve.Also in here
Pending registrations.
DiagnosticsMetrics' underlying client initializes asynchronously, but apps calltrackCache/registerObservableGauge/registerObservableCountersynchronously at module load — frequently before that initialization finishes. A call made before the client is ready is now queued and replayed once it is. This is the one existing method this PR touches (initMetricClient), and it's a no-op for any app that never calls the new APIs — the existing tests' mock client has nogetProvider()at all, and it is never called by them.Three guards on the observable instruments:
dup_metric:GAUGE+dup_metric:SUM, no error) and the collector then rejects them.MAX_CUSTOM_ATTRIBUTESlimit as the push-based methods. The callback callsresult.observedirectly, so this needs a wrapper on the result.How should this be manually tested?
yarn test src/metrics/DiagnosticsMetrics.test.ts src/caches/cacheStats.test.tsThe cache tests run against real cache instances;
trackCache's tests run against a realMeterProvider+MetricReaderrather than a mock, specifically to catch the single-read-per-cycle and cumulative-counter accounting a mock could get "passing" while still wrong. Notable cases:deltatemporality — the one actually configured inservice/telemetry/client.ts, not justcumulative.LRUCacheread by the legacy flush and the observable at once, confirming neither steals the other's counts.trackCachecalled before the client is ready.Verified:
tsc --noEmittslintDiagnosticsMetrics.tsTypes of changes
docs/METRICS_CATALOG.mdgains a Cache Metrics (Observable) section, including the caveat thatio_app_cache_capacityis in the cache's own units (maxis an item count only when the LRU wasn't built with alengthfunction).docs/METRICS_OVERVIEW.mdgains Pattern 5 (trackCache) and Pattern 6 (addOnFlushMetric→registerObservableGauge/Counter) — the RFC names both legacy APIs as blocking STR-1209, and the second had no documented path. Cross-referenced with the existing Pattern 4, the different already-supported push-based idiom.Follow-up, deliberately not here
src/HttpClient/middlewares/inflight.tsusesaddOnFlushMetricfor node-vtex-api's own inflight-map metric, so the legacy method can't be deleted fromMetricsAccumulatoruntil that caller moves toregisterObservableGauge. It also has a latent bug —inflight.entries.lengthis the method's arity (0), not the map size, so that metric has always reported zero. Separate PR.🤖 Generated with Claude Code