Missing production metrics for webhook delivery outcomes, retry-queue depth, and price-source failure rates

[prodbycorne]

Overview

The only Prometheus metric wired up anywhere in this codebase is a single WebSocket connection gauge in src/ws/PriceSubscriptionManager.js:

let wsConnectionsGauge = null;
try {
  const prom = require('prom-client');
  wsConnectionsGauge = new prom.Gauge({
    name: 'ws_connections_current',
    help: 'Number of currently active WebSocket connections',
  });
} catch {
  // prom-client not installed; gauge is a no-op.
}

Note prom-client isn't even listed in package.json's dependencies — this gauge silently no-ops unless an operator happens to install it separately, and there is no /metrics endpoint anywhere in src/index.js to actually scrape it even if it were populated. Beyond this one gauge, none of the operationally critical subsystems expose any metrics at all:

• Webhook delivery: no counter for successes/failures/retries/dead-letters, no histogram for delivery latency (webhookDispatcher.js's attempt() already computes enough information — response status, whether it was a network error — to trivially derive these, but nothing records them beyond a logger.info/warn/error line).
• Retry queue depth: deliveryRepository.js's webhooks:retries sorted set size (a direct proxy for "how backed up is webhook delivery right now") is never exposed anywhere.
• Price source health: priceOracle.js's fetchFromAllSources() already knows, per cycle, exactly which of the three sources succeeded/failed/were rate-limited (coingecko.js and coinmarketcap.js both explicitly detect 429s) — none of this is aggregated into a metric an operator could alert on (e.g. "CoinMarketCap has failed for the last 10 consecutive refresh cycles").
• Rate limiting: rateLimit.js sets response headers per-request but never counts how often any given limiter actually rejects requests, making it impossible to tell from metrics alone whether a configured limit is too tight (causing legitimate 429s) or is never being hit at all.

Without any of this, on-call operators have no way to answer basic production questions ("is webhook delivery healthy right now," "is our retry queue growing unboundedly," "which price source keeps failing") without grepping structured logs after the fact.

Requirements

• Add prom-client as an explicit dependency in package.json (not an optional silently-degrading one) and expose a standard /metrics endpoint in src/index.js.
• Add counters/histograms for: webhook delivery attempts by outcome (success/retry/permanent-failure) and by webhook, delivery attempt latency, current retry-queue depth (sampled periodically or updated on scheduleRetry/popDueRetries), price-source fetch outcome by source name (success/failure/rate-limited), and rate-limiter rejection counts by keyPrefix.
• Metrics must have low cardinality where user-controlled values are involved (e.g. do not label a metric directly with an arbitrary user-supplied webhook URL — use webhook_id or an aggregate instead) to avoid a metrics-cardinality blowup that itself becomes an operational hazard.
• Document the new /metrics endpoint and the full list of exposed metrics in the README.

Acceptance Criteria

• GET /metrics returns Prometheus-format output including, at minimum, webhook delivery outcome counters, retry-queue depth, price-source outcome counters, and rate-limit rejection counters.
• prom-client is a real, declared dependency in package.json, not a silently-optional try/catch-guarded one.
• Metrics carry no unbounded-cardinality labels derived directly from user input.
• New tests assert that a successful and a failed webhook delivery each increment the expected counter, and that a price-source failure increments its corresponding counter.
• README documents the metrics endpoint and each exposed metric's meaning.

Additional Notes

More precise references

• src/ws/PriceSubscriptionManager.js:1-16 (approximate, per the issue's own excerpt): confirmed prom-client is required inside a try/catch, meaning the entire gauge silently no-ops if the package isn't installed — and it is confirmed absent from package.json's dependencies per the issue text (not independently re-verified this pass, but consistent with the "silently-optional" framing).
• src/services/webhookDispatcher.js:46-143 (attempt): confirmed this function already computes every value needed for delivery-outcome metrics — responseStatus, networkError, succeeded, attempts, and the final status written via deliveryRepo.update ('success'/'pending'/'failed') — but only logs via logger.info/warn/error, never increments any counter.
• src/repositories/deliveryRepository.js — not re-read in full this pass, but referenced by both webhookDispatcher.js (deliveryRepo.scheduleRetry, line 111) and webhookRetryWorker.js (popDueRetries, line 15) as the sorted-set-backed retry queue; its size (ZCARD webhooks:retries or equivalent) is the natural retry-queue-depth metric source.
• src/services/priceOracle.js:77-92 (fetchFromAllSources): confirmed this is the single choke point where every source's success/failure is already known per cycle (results.push({ source: source.name, price }) on success, logger.warn('Source fetch failed', ...) on failure) — an ideal, minimally-invasive instrumentation point requiring no changes to the three individual source modules.
• src/middleware/rateLimit.js:37-39: confirmed the count > max branch is the single point where a rejection is decided, already carrying keyPrefix in its closure scope (from buildRateLimit's parameters) — a natural, already-available label for a rejection counter with no plumbing needed to know which limiter fired.

Additional edge cases

• Cardinality risk isn't limited to webhook URLs (already flagged in the issue) — webhook_id itself is unbounded-but-growing (one label value per webhook ever created, forever, even after a webhook is deleted, since Prometheus doesn't know a label value has become permanently dead) — for a system that could accumulate thousands of webhooks over its lifetime, a webhook_id-labeled histogram is itself a slow-motion cardinality leak; consider whether delivery metrics should be aggregate-only (no webhook_id label at all, just outcome/source labels) with per-webhook drill-down left to the existing GET /webhooks/:id/deliveries API instead of Prometheus, or whether a bounded cardinality budget (e.g. cap tracked distinct webhook_id label values, falling back to an "other" bucket beyond some N) is worth the complexity.
• The retry-queue-depth gauge needs a sampling strategy: computing ZCARD on every metrics scrape (pull-based Prometheus model) is cheap and simple, but computing it inside webhookRetryWorker.tick() and caching the value for /metrics to read (push-based-internally) avoids adding a Redis round trip to the /metrics request path itself, which matters if /metrics is scraped frequently by Prometheus (default 15-30s) — recommend the latter for consistency with the issue's own suggestion of "sampled periodically or updated on scheduleRetry/popDueRetries."
• prom-client's default metrics (collectDefaultMetrics(), e.g. process CPU/memory/event-loop-lag) are a near-zero-cost addition once the dependency is real rather than optional, and event-loop-lag in particular would have directly surfaced whether e.g. parseCSV's synchronous-ish work (flagged in multer() is configured with no file size limit — unbounded CSV upload enables memory-exhaustion DoS #85) or a large Promise.all in webhookDispatcher.dispatch (flagged in No process-level unhandledRejection/uncaughtException handlers — a single unexpected throw can crash the process #91's cross-references) is ever blocking the event loop — worth including even though not explicitly required by the issue's acceptance criteria, since it's essentially free once /metrics exists.

Implementation sketch

1. package.json: move prom-client to real dependencies (pin a version); remove the try/catch require guard in PriceSubscriptionManager.js, importing it directly like any other dependency.
2. New src/metrics.js module: a single shared prom-client Registry, with named counters/histograms:
   • webhook_deliveries_total{outcome="success|retry|failed"} (Counter)
   • webhook_delivery_duration_seconds (Histogram, no webhook_id label per the cardinality note above — or an explicit, small, allow-listed label set if per-webhook drill-down is deemed worth the risk)
   • webhook_retry_queue_depth (Gauge, updated by webhookRetryWorker.tick() after each popDueRetries call, or a periodic separate sampler)
   • price_source_fetch_total{source="coingecko|coinmarketcap|stellar_dex", outcome="success|failure|rate_limited"} (Counter)
   • rate_limit_rejections_total{key_prefix} (Counter)
   • collectDefaultMetrics({ register }) for process-level metrics.
3. Instrument call sites: webhookDispatcher.js's attempt() increments the delivery counter and observes duration (wrap the postOnce call in a timer) right where succeeded/retryable are already computed; priceOracle.js's fetchFromAllSources increments the source-outcome counter in its existing try/catch, distinguishing rate-limited (needs a way to detect this from the caught error — likely requires source modules to also set a rateLimited flag analogous to nonRetryable from CoinMarketCap's nonRetryable auth-failure flag is set but never consulted — invalid keys are retried every 30s forever #95, or inspecting err.response?.status === 429 directly in the shared catch block); rateLimit.js's count > max branch increments the rejection counter before constructing the AppError.
4. New route: app.get('/metrics', async (req, res) => { res.set('Content-Type', register.contentType); res.end(await register.metrics()); }); in src/index.js, mounted before or independent of the requireApiKey gates (decide whether /metrics itself needs auth — likely yes in production, e.g. gated by a separate METRICS_API_KEY or IP allowlist, since it can leak operational details; the issue doesn't explicitly require this but it's a reasonable follow-up worth flagging in the PR).
5. README: document the endpoint, each metric name/type/labels/meaning, and any auth requirement decided above.

Test/reproduction plan

• GET /metrics returns 200 with Content-Type: text/plain; version=0.0.4 (prom-client's standard exposition format) and contains at least one sample line per required metric name.
• Trigger a successful webhook delivery (mocked axios.post resolving 2xx) via dispatcher.attempt(); assert webhook_deliveries_total{outcome="success"} incremented by exactly 1, and the duration histogram recorded one observation.
• Trigger a failed delivery (mocked axios.post rejecting); assert the outcome="retry" or "failed" counter (depending on attempt count vs maxAttempts) incremented appropriately.
• Simulate a CoinMarketCap 401 via priceOracle.fetchFromAllSources; assert price_source_fetch_total{source="coinmarketcap", outcome="failure"} incremented.
• Fire requests past a rate limiter's max; assert rate_limit_rejections_total{key_prefix="..."} incremented once per rejected request, not once per allowed request.
• Assert no metric sample line in /metrics' output contains a raw webhook URL or any other clearly-unbounded user-controlled string as a label value (a simple regex/grep-style assertion over the scraped text in the test).

Cross-references

• CoinMarketCap's nonRetryable auth-failure flag is set but never consulted — invalid keys are retried every 30s forever #95 (CoinMarketCap circuit breaker) — that issue's own acceptance criteria explicitly ask for circuit state to be "observable... via the metrics work from the companion observability issue," i.e. this one; the price_source_fetch_total counter and a new price_source_circuit_state gauge (open=1/closed=0, or a since timestamp) should be designed together with CoinMarketCap's nonRetryable auth-failure flag is set but never consulted — invalid keys are retried every 30s forever #95, ideally landing in the same PR or immediately sequential ones.
• Health check endpoint reports only Redis status — misses price-source, webhook-worker, and DB connectivity #92 (health check depth) — job-tick-staleness state introduced there is a natural additional metric here (job_last_tick_age_seconds); recommend sharing the underlying lastTickAt tracking variables between the two issues rather than each inventing separate instrumentation.
• Fixed-window rate limiter allows boundary bursts and trusts req.ip without configuring Express trust proxy #90 (rate limiter sliding window) — the rejection-counter work here should be added to whichever rate-limit implementation lands (fixed-window today, sliding-window after Fixed-window rate limiter allows boundary bursts and trusts req.ip without configuring Express trust proxy #90); if Missing production metrics for webhook delivery outcomes, retry-queue depth, and price-source failure rates #93 lands first, keep the instrumentation point (the count > max-equivalent decision) stable across Fixed-window rate limiter allows boundary bursts and trusts req.ip without configuring Express trust proxy #90's refactor so the metric doesn't need rework.
• Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98 (leader election) — once jobs run on a single leader, webhook_retry_queue_depth and the price-source counters should probably be tagged or documented as "leader-only" metrics so operators don't misinterpret a follower's flat/zero metrics as a problem; worth a README note cross-referencing both issues once Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98 lands.