perf(server): coalesce metrics queries and handle empty series - #1710
OskarEichler wants to merge 3 commits into
Conversation
Greptile SummaryThis PR replaces the dynamic Prometheus proxy with a typed promise cache that coalesces concurrent manager-metrics queries, removes failed entries for retry, and preserves five-minute result reuse.
Confidence Score: 4/5The PR appears safe to merge, with the non-blocking concern that its new cache lifecycle is not protected by repository regression tests. The typed wrapper satisfies the complete PrometheusClient interface, concurrent calls share promises, rejected entries are retryable, and the null guards safely preserve default bandwidth values; only durable test coverage for the cache lifecycle is missing. Files Needing Attention: src/shadowbox/server/manager_metrics.ts
|
| Filename | Overview |
|---|---|
| src/shadowbox/server/manager_metrics.ts | Adds typed pending-query caching and defensive handling for empty or unusual Prometheus series; behavior appears sound, but the cache lifecycle lacks committed regression tests. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
R[Manager metrics request] --> P[Prune expired entries]
P --> L{Cache entry exists?}
L -->|Yes| A[Await shared promise]
L -->|No| Q[Start Prometheus query]
Q --> C[Cache pending promise]
C --> A
A --> S{Query succeeds?}
S -->|Yes| T[Refresh timestamp and return result]
S -->|No| E[Evict matching entry and rethrow]
Reviews (1): Last reviewed commit: "perf(server): coalesce metrics queries a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private async cachedQuery( | ||
| cacheId: string, | ||
| query: () => Promise<QueryResultData> | ||
| ): Promise<QueryResultData> { | ||
| const cached = this.prometheusCache.get(cacheId); | ||
| if (cached) { | ||
| return cached.result; | ||
| } | ||
|
|
||
| return result; | ||
| }; | ||
| }, | ||
| }); | ||
| // Cache the pending query too, so simultaneous dashboard requests share work. | ||
| const entry = {timestamp: Date.now(), result: query()}; | ||
| this.prometheusCache.set(cacheId, entry); | ||
| try { | ||
| const result = await entry.result; | ||
| entry.timestamp = Date.now(); | ||
| return result; | ||
| } catch (error) { | ||
| // A failed query must be retried; don't evict a newer entry with the same key. | ||
| if (this.prometheusCache.get(cacheId) === entry) { | ||
| this.prometheusCache.delete(cacheId); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
The new pending-promise reuse and rejection-eviction behavior has no repository regression coverage, so later changes can silently restore duplicate Prometheus queries, retain rejected promises, or break expiration semantics.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Added coverage for pending-query sharing, successful-result reuse, rejection eviction, and retry. The focused manager metrics suite passes: 5 specs, 0 failures. |
1 participant
Why
Concurrent dashboard reads issue the same Prometheus queries independently. Empty bandwidth results and negative-infinity peak samples can also dereference absent values.
Changes
Verification
After splitting, all 65 existing manager-metrics, shared-metrics, access-key, and file-operation specs passed again on this isolated branch under Node 22.23.2.
Existing manager-metrics specs passed.
Controlled source checks verified ten concurrent reads sharing seven queries instead of seventy, failed-query retry, empty results, and unusual peak samples.
Targeted semantic TypeScript and formatting checks passed.
Patch isolation and
git diff --checkpassed. Recombining all focused patches reproduces the reviewed source changes exactly.No test/spec files were added or modified; controlled probe drivers are kept outside the repository.
Compatibility and remaining validation
Full npm installation/build checks remain incomplete: npm 12 rejected existing lockfile Git dependencies (
EALLOWGIT). Isolated registry-only tooling was used without disabling that safeguard.Scope
This is one focused part of the reliability review, based directly on upstream
master; it does not include the other review patches. No installed client, live VPN/DNS setting, or production service was changed by this patch.Second review — 2026-08-27
Reuse the already-extracted bandwidth range instead of looping once and breaking. Use Date.now() directly for timestamps. Retain pending-query coalescing, failed-entry eviction and cache expiry behavior.
Controlled checks passed for seven shared queries across simultaneous dashboard reads, empty series, expiry, and failure retry. All 65 selected existing server specs and targeted TypeScript checks passed on this branch.
All touched files and nearby callers were reviewed again. Each focused patch was also checked in combination with the other seven patches for this repository. External probe drivers remain outside the repositories; no test/spec files were added or modified. Existing full-build and platform-validation limitations above still apply.