Implement MetricsService with time-series storage, Prometheus exposit…#126
Conversation
…ion, and threshold-based alerting with email notifications and rate limiting.
WalkthroughAdds a MetricsService singleton, request-observing middleware and periodic collectors, Prometheus exposition and dashboard routes, enhanced health endpoints (/dependencies, /db, /system) with per-dependency latencies, typed request config cache changes, and two runtime deps: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as Express App
participant M as metricsMiddleware
participant R as Route Handler
participant S as MetricsService
C->>A: HTTP Request
A->>M: invoke metricsMiddleware
Note right of M #cfe8ff: record start HR time
M->>A: next()
A->>R: handle route
R-->>A: send response
A-->>C: HTTP Response
Note right of M #e8f7e8: on "finish" -> observeRequest
M->>S: observeRequest(req,res,start)
S-->>M: recorded
sequenceDiagram
autonumber
participant C as Client
participant A as Express App
participant Rt as metrics.routes
participant S as MetricsService
C->>A: GET /metrics/prometheus
A->>Rt: metrics route handler
Rt->>S: collectSystemResources()
Rt->>S: checkExternalStatus()
Rt->>S: getPrometheusMetrics()
S-->>Rt: metrics text/plain
Rt-->>C: 200 text/plain (v0.0.4)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 6
🧹 Nitpick comments (12)
src/routes/health.routes.ts (2)
209-214: Extract duplicated database check logic.The database connection check logic is duplicated between
/dband/dependenciesendpoints.Consider extracting this into a shared utility function:
async function checkDatabaseHealth(): Promise<{ isHealthy: boolean; latencyMs: number }> { if (!AppDataSource.isInitialized) { await AppDataSource.initialize(); } const start = Date.now(); await AppDataSource.query("SELECT 1"); return { isHealthy: true, latencyMs: Date.now() - start }; }This would reduce duplication and make the code more maintainable.
231-231: Route handler should not be marked as async without await.The route handler is marked as
asyncbut doesn't useawaitanywhere in the try block.Remove the
asynckeyword since it's not needed:-router.get("/system", async (_req, res) => { +router.get("/system", (_req, res) => {src/middlewares/metrics.middleware.ts (3)
4-14: Capture aborted requests and avoid double-recordingOnly listening to "finish" misses aborted connections; add a "close" handler and guard to prevent double observation.
export function metricsMiddleware(req: Request, res: Response, next: NextFunction) { - const start = process.hrtime(); - res.on("finish", () => { - try { - metricsService.observeRequest(req, res, start); - } catch (err) { - // avoid breaking the request - } - }); + const start = process.hrtime(); + let observed = false; + const observe = () => { + if (observed) return; + observed = true; + try { + metricsService.observeRequest(req, res, start); + } catch { + // avoid breaking the request + } + }; + res.on("finish", observe); + res.on("close", observe); // covers aborted requests next(); }
4-4: Annotate explicit return type for Express v5 handlersExpress v5 types expect route/middleware handlers to return void. Add explicit annotation for clarity.
-export function metricsMiddleware(req: Request, res: Response, next: NextFunction) { +export function metricsMiddleware(req: Request, res: Response, next: NextFunction): void {Based on learnings.
17-24: Run collectors concurrently and fix no-unused-vars in catch
- Run both collectors in parallel to reduce latency.
- Avoid unused catch param to satisfy ESLint.
-export async function collectPeriodicMetrics() { - try { - await metricsService.collectSystemResources(); - await metricsService.checkExternalStatus(); - } catch (err) { - // swallow errors - } -} +export async function collectPeriodicMetrics(): Promise<void> { + try { + await Promise.all([ + metricsService.collectSystemResources(), + metricsService.checkExternalStatus(), + ]); + } catch { + // swallow errors + } +}src/routes/metrics.routes.ts (3)
16-21: Avoid heavy checks on every summary request/metrics/summary calls collectSystemResources() and checkExternalStatus() on each request. The dashboard polls every 5s, which will hit DB/Redis/Horizon aggressively. Prefer returning metricsService.getSnapshot() populated by a background scheduler (collectPeriodicMetrics) with a short TTL cache, and make “forceRefresh=true” optional.
66-73: Prometheus scrape should not block indefinitelyDB/Redis/Horizon checks may hang; add timeouts and consider serving latest snapshot if checks exceed a deadline.
-router.get("/prometheus", async (_req, res) => { - // ensure latest sample collected - await metricsService.collectSystemResources(); - await metricsService.checkExternalStatus(); +router.get("/prometheus", async (_req, res) => { + // ensure latest sample collected, but bound the work + const withTimeout = <T>(p: Promise<T>, ms: number) => + Promise.race([p, new Promise<T>((_, r) => setTimeout(() => r(new Error("timeout")), ms))]); + try { + await withTimeout(metricsService.collectSystemResources(), 1500); + await withTimeout(metricsService.checkExternalStatus(), 1500); + } catch { + // fall back to last snapshot + } const body = metricsService.getPrometheusMetrics(); - res.set("Content-Type", "text/plain; version=0.0.4"); + res.set("Content-Type", "text/plain; version=0.0.4; charset=utf-8"); res.send(body); });
82-167: Minor: dashboard safety and UX nits
- Use textContent consistently (already used for RPS) and prefer encodeURIComponent for all query parts (already done).
- Consider caching-busting headers for the HTML to avoid stale reloads.
- res.setHeader("Content-Type", "text/html"); + res.setHeader("Content-Type", "text/html; charset=utf-8"); + res.setHeader("Cache-Control", "no-store");src/services/MetricsService.ts (4)
125-143: CPU percent is process-average since start; consider windowed samplingprocess.cpuUsage() divided by uptime gives lifetime average, not a current load sample. Track deltas across intervals for a more accurate gauge and alerting.
370-372: Avoid unused catch variableChange to catch { … } or log the error to satisfy ESLint and improve observability.
- } catch (err) { - logger.error("Failed to send alert email", { error: (err as Error)?.message }); - } + } catch (err) { + logger.error("Failed to send alert email", { error: (err as Error)?.message }); + }If you intentionally want to swallow without logging elsewhere, prefer:
- } catch (err) { - // ... - } + } catch { + // intentionally ignored + }
349-381: maybeAlert’svalueparam is unusedRemove the unused parameter or use it in logs; keeps API minimal.
- private async maybeAlert(key: string, value: number, predicate: () => Promise<boolean>, message: string) { + private async maybeAlert(key: string, predicate: () => Promise<boolean>, message: string) { @@ - if (await predicate()) this.triggerAlert(key, message); + if (await predicate()) this.triggerAlert(key, message);And update the only call site accordingly.
118-123: Update maybeAlert call after signature changeFollow-up to previous comment if applied.
- this.maybeAlert("api_latency", durationMs, async () => { + this.maybeAlert("api_latency", async () => { const rawLatency = await configurationService.getConfig("METRICS_API_LATENCY_THRESHOLD_MS", "1000"); const threshold = Number(rawLatency ?? 1000); return durationMs > threshold; }, `High API latency: ${durationMs.toFixed(2)}ms for ${method} ${route}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.json(2 hunks)src/app.ts(4 hunks)src/middlewares/metrics.middleware.ts(1 hunks)src/routes/health.routes.ts(3 hunks)src/routes/metrics.routes.ts(1 hunks)src/services/MetricsService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/middlewares/metrics.middleware.ts (2)
src/utils/subscriptionScheduler.ts (1)
start(14-29)src/services/MetricsService.ts (1)
metricsService(384-384)
src/routes/metrics.routes.ts (1)
src/services/MetricsService.ts (1)
metricsService(384-384)
src/app.ts (1)
src/middlewares/metrics.middleware.ts (1)
metricsMiddleware(4-14)
src/services/MetricsService.ts (3)
src/services/ConfigurationService.ts (1)
configurationService(780-780)src/config/redisConfig.ts (1)
redisClient(78-78)src/utils/mailer.ts (1)
sendEmail(11-31)
src/routes/health.routes.ts (1)
src/config/redisConfig.ts (1)
redisClient(78-78)
🪛 GitHub Check: build-and-test
src/middlewares/metrics.middleware.ts
[warning] 21-21:
'err' is defined but never used
[warning] 9-9:
'err' is defined but never used
src/routes/metrics.routes.ts
[failure] 54-54:
Unexpected any. Specify a different type
src/services/MetricsService.ts
[warning] 151-151:
'err' is defined but never used
[failure] 148-148:
Unexpected any. Specify a different type
[failure] 146-146:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Actions: CI Pipeline
src/middlewares/metrics.middleware.ts
[warning] 9-9: ESLint: 'err' is defined but never used. (no-unused-vars)
[warning] 21-21: ESLint: 'err' is defined but never used. (no-unused-vars)
src/routes/metrics.routes.ts
[error] 54-54: ESLint: Unexpected any. Specify a different type. (no-explicit-any)
🔇 Additional comments (7)
src/routes/health.routes.ts (3)
3-6: LGTM! Imports are appropriate for the enhanced health checks.The added imports support the new functionality:
- Redis and Stellar config for dependency checks
- Node.js built-ins for system resource monitoring
99-99: Database latency measurement implemented correctly.Good addition of latency tracking for the database health check. The implementation is clean and follows a simple pattern.
Also applies to: 107-109
237-239: Critical: CPU percentage calculation is incorrect.The CPU usage calculation has a unit conversion error.
process.cpuUsage()returns values in microseconds, but the calculation incorrectly divides by 1000 to convert to milliseconds, then divides by milliseconds again.Fix the calculation:
- const cpuPercent = - ((cpuUsage.user + cpuUsage.system) / 1000 /* to ms */) / - (uptimeSec * 1000 * cores); + // cpuUsage is in microseconds, convert to seconds + const cpuPercent = + ((cpuUsage.user + cpuUsage.system) / 1_000_000) / + (uptimeSec * cores);The correct formula should:
- Convert CPU usage from microseconds to seconds (divide by 1,000,000)
- Divide by total available CPU time (uptime × cores)
Likely an incorrect or invalid review comment.
src/app.ts (3)
12-12: Metrics imports properly integrated.The new metrics middleware and routes imports follow the existing code structure and naming conventions.
Also applies to: 30-30
90-90: Metrics middleware correctly positioned in the middleware chain.Good placement after rate limiting and request logging - this ensures metrics capture only legitimate requests that pass rate limiting.
174-174: Require authentication for /metrics endpoints or confirm mount-level protection.src/routes/metrics.routes.ts exposes /summary, /historical, /prometheus, and /dashboard and contains no auth imports or router.use middleware; verify src/app.ts (mount at app.use("/metrics", metricsRoutes) — src/app.ts:174) applies authentication/authorization when mounting. If not, protect these endpoints (especially /prometheus) with auth, IP allowlist or mTLS and limit access to internal/prometheus scrapers.
package.json (1)
29-29: Approve dependency additions
The added dependencies align with metrics objectives:
check-disk-space ^3.4.0is fully async and Node.js 16+ compatible.prom-client ^15.1.3is the latest stable version.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/services/MetricsService.ts (2)
14-16: ExportTimeSeriesPointfor downstream typing
Routes consuminggetHistoricalmay want this type; exporting improves DX without behavior change.-type TimeSeriesPoint = { ts: number; value: Numeric }; +export type TimeSeriesPoint = { ts: number; value: Numeric };
264-270: Consider ring buffer to avoid O(n)shift()on large series
Current shifting can be costly under high volume. A fixed-size circular buffer would reduce GC/CPU overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/health.routes.ts(3 hunks)src/routes/metrics.routes.ts(1 hunks)src/services/MetricsService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/metrics.routes.ts
- src/routes/health.routes.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/MetricsService.ts (3)
src/services/ConfigurationService.ts (1)
configurationService(780-780)src/config/redisConfig.ts (1)
redisClient(78-78)src/utils/mailer.ts (1)
sendEmail(11-31)
🪛 GitHub Check: build-and-test
src/services/MetricsService.ts
[failure] 309-309:
Unnecessary escape character: "
[warning] 152-152:
'err' is defined but never used
🪛 GitHub Actions: CI Pipeline
src/services/MetricsService.ts
[warning] 152-152: 'err' is defined but never used (no-unused-vars)
[error] 309-309: Unnecessary escape character: " (no-useless-escape)
🔇 Additional comments (2)
src/services/MetricsService.ts (2)
93-101: Use stable route patterns to prevent label cardinality blow‑ups
Usingreq.originalUrl/req.urlcan include querystrings and IDs. Prefer route pattern (optionally with baseUrl), then fall back toreq.path.- const route = req.route?.path || req.originalUrl || req.url || "unknown"; + // Use stable route pattern (baseUrl + route.path) or fallback to path (no query) + const routePattern = ((req.baseUrl ?? "") + (req.route?.path?.toString?.() ?? "")).toString(); + const route = routePattern || req.path || "unknown";
214-219: No action needed: production Node v24.3.0 includes global fetch support—polyfill isn’t required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/services/MetricsService.ts (7)
114-121: Throughput calculation is O(n) per request; make it O(1) with 60 bucketsMaintaining and shifting a timestamp array each request will hurt under load. Track counts in a 60‑slot ring buffer keyed by current second.
Example (fields to add):
+ private throughputBuckets: number[] = Array(60).fill(0); + private lastBucketSec = 0;Update in observeRequest:
- const now = Date.now(); - this.lastRequestsTimestamps.push(now); - // keep only last 60s - const cutoff = now - 60_000; - while (this.lastRequestsTimestamps.length && this.lastRequestsTimestamps[0] < cutoff) { - this.lastRequestsTimestamps.shift(); - } + const now = Date.now(); + const sec = Math.floor(now / 1000); + if (sec !== this.lastBucketSec) { + // zero buckets between last and current second + const steps = Math.min(60, sec - this.lastBucketSec); + for (let i = 1; i <= steps; i++) this.throughputBuckets[(sec - i) % 60] = 0; + this.lastBucketSec = sec; + } + this.throughputBuckets[sec % 60] += 1;Then in getSnapshot:
- const throughputRps = this.lastRequestsTimestamps.filter((t) => t >= cutoff).length / 60; + const throughputRps = this.throughputBuckets.reduce((a, b) => a + b, 0) / 60;
131-147: CPU percent computed since process start; switch to interval‑based samplingUsing lifetime CPU time skews percent down over long uptimes. Track deltas between samples for accuracy.
Add fields:
+ private lastCpu = process.cpuUsage(); + private lastCpuHr = process.hrtime.bigint();Replace CPU calculation:
- const cpuUsage = process.cpuUsage(); - const cores = os.cpus().length || 1; - const cpuPercent = (((cpuUsage.user + cpuUsage.system) / 1000) / (uptimeSec * 1000 * cores)) * 100; + const diff = process.cpuUsage(this.lastCpu); // microseconds since last sample + const nowHr = process.hrtime.bigint(); + const elapsedUs = Number(nowHr - this.lastCpuHr) / 1000; + const cores = os.cpus().length || 1; + const cpuPercent = ((diff.user + diff.system) / (elapsedUs * cores)) * 100; + this.lastCpu = process.cpuUsage(); + this.lastCpuHr = nowHr;
167-178: Memory alert uses heap ratio; consider RSS vs system memoryHeap ratio can under‑report memory pressure (native memory excluded). Prefer
rss / os.totalmem()for a host‑level signal, or make the basis configurable.- const memPercent = (resources.memory.heapUsed / resources.memory.heapTotal) * 100; + const memPercent = (resources.memory.rss / os.totalmem()) * 100;
190-224: Timeouts are hard‑coded; derive from config and cancel DB if supportedReuse existing thresholds or add config keys for DB/Redis/Stellar timeouts. If your DB driver supports per‑query timeout, prefer that over
Promise.raceto avoid orphan queries.- const deadline = (ms: number) => new Promise<never>((_, reject) => setTimeout(() => reject(new Error("timeout")), ms)); + const deadline = (ms: number) => new Promise<never>((_, reject) => setTimeout(() => reject(new Error("timeout")), ms)); + const dbTimeoutMs = Number(await configurationService.getConfig("METRICS_DB_TIMEOUT_MS", "1500")); + const redisTimeoutMs = Number(await configurationService.getConfig("METRICS_REDIS_TIMEOUT_MS", "800")); + const stellarTimeoutMs = Number(await configurationService.getConfig("METRICS_STELLAR_TIMEOUT_MS", "1500")); @@ - await Promise.race([AppDataSource.query("SELECT 1"), deadline(1500)]); + // Prefer driver-level timeout if available; fallback to race + await Promise.race([AppDataSource.query("SELECT 1"), deadline(dbTimeoutMs)]); @@ - const pingResult = await Promise.race([redisClient.ping(), deadline(800)]); + const pingResult = await Promise.race([redisClient.ping(), deadline(redisTimeoutMs)]); @@ - const t = setTimeout(() => controller.abort(), 1500); + const t = setTimeout(() => controller.abort(), stellarTimeoutMs);
332-344: Expose up/down gauges for external depsAdd Prometheus
*_upgauges (1/0) so alerting doesn’t rely solely on latency.push(`# HELP paystell_db_latency_ms Database query latency in ms`); push(`# TYPE paystell_db_latency_ms gauge`); push(`paystell_db_latency_ms ${snapshot.external.database.latencyMs}`); + push(`# HELP paystell_db_up Database availability (1=up, 0=down)`); + push(`# TYPE paystell_db_up gauge`); + push(`paystell_db_up ${snapshot.external.database.status === "OK" ? 1 : 0}`); @@ push(`# HELP paystell_redis_latency_ms Redis ping latency in ms`); push(`# TYPE paystell_redis_latency_ms gauge`); push(`paystell_redis_latency_ms ${snapshot.external.redis.latencyMs}`); + push(`# HELP paystell_redis_up Redis availability (1=up, 0=down)`); + push(`# TYPE paystell_redis_up gauge`); + push(`paystell_redis_up ${snapshot.external.redis.status === "OK" ? 1 : 0}`); @@ push(`# HELP paystell_stellar_latency_ms Stellar Horizon latency in ms`); push(`# TYPE paystell_stellar_latency_ms gauge`); push(`paystell_stellar_latency_ms ${snapshot.external.stellar.latencyMs}`); + push(`# HELP paystell_stellar_up Stellar availability (1=up, 0=down)`); + push(`# TYPE paystell_stellar_up gauge`); + push(`paystell_stellar_up ${snapshot.external.stellar.status === "OK" ? 1 : 0}`);
401-406: AwaittriggerAlertto preserve ordering and error handlingWithout
await, errors intriggerAlertwon’t be caught here and rate limiting may race.- if (await predicate()) this.triggerAlert(key, message); + if (await predicate()) await this.triggerAlert(key, message);
308-373: Consider using prom-client for exposition to reduce maintenanceYou already added
prom-clientin the PR. Using it to register gauges/counters will avoid manual 0.0.4 text formatting and edge cases long‑term.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/MetricsService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/MetricsService.ts (2)
src/services/ConfigurationService.ts (1)
configurationService(780-780)src/utils/mailer.ts (1)
sendEmail(11-31)
🪛 GitHub Actions: CI Pipeline
src/services/MetricsService.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix code style in this file.
🔇 Additional comments (2)
src/services/MetricsService.ts (2)
102-106: Avoid unbounded label cardinality for route keysStill uses
req.originalUrl/req.urlwhich can include query strings and IDs. Prefer stable route pattern orreq.path(no query), combined withreq.baseUrl. This was flagged previously.Apply:
- const route = req.route?.path || req.originalUrl || req.url || "unknown"; - const method = req.method; - const status = res.statusCode; - const key: RouteKey = `${method} ${route}`; + // Prefer stable route patterns to avoid cardinality blowup + const routePattern = (req.baseUrl ?? "") + (req.route?.path ?? req.path ?? ""); + const route = routePattern || "unknown"; + const method = req.method; + const status = res.statusCode; + const key: RouteKey = `${method} ${route}`;
1-1: Prettier formatting verified
All files conform to Prettier style; CI Prettier warning is resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/services/MetricsService.ts (2)
179-193: Windows mount path should be'C:/'not'C:'.Based on learnings:
check-disk-spaceexpects a root path with trailing slash on Windows (e.g.,C:/). UsingC:may cause the library to fail or return incorrect results.Apply this diff:
- const mount = process.platform === "win32" ? "C:" : "/"; + const mount = process.platform === "win32" ? "C:/" : "/";Additionally, consider making the mount path configurable via environment variable to support different drive letters or mount points in CI/dev environments.
111-129: Route key still includes query strings → unbounded cardinality.Line 114 uses
req.originalUrl || req.urlas fallbacks, which can include query parameters and dynamic IDs (e.g.,/users/123?filter=active). This creates unbounded label cardinality in Prometheus metrics and bloats memory with per-request route keys.Apply this diff to prefer stable route patterns:
- const route = req.route?.path || req.originalUrl || req.url || "unknown"; + // Prefer stable route patterns to avoid cardinality explosion + const route = req.route?.path || req.path || "unknown";Note:
req.pathexcludes query strings, unlikereq.originalUrlandreq.url.
🧹 Nitpick comments (3)
src/routes/health.routes.ts (1)
246-249: CPU calculation reports average since process start, not current usage.The CPU percentage calculation uses cumulative
process.cpuUsage()values divided by total uptime, yielding average CPU usage since the process started rather than current instantaneous usage. This may confuse operators expecting real-time CPU load.Consider documenting this behavior in the API response or Swagger docs, or implement a time-windowed calculation that samples CPU usage over the last N seconds to provide more actionable current usage data. If you prefer the current approach, add a clarifying field like
"cpuPercentSinceStart"instead of"cpuPercent".src/services/MetricsService.ts (1)
158-227: CPU calculation reports average since process start, not current usage.The CPU percentage calculation (lines 163-165) uses cumulative
process.cpuUsage()values divided by total uptime, yielding average CPU usage since process start. This differs from real-time CPU load that operators typically expect for alerting and dashboards.Consider implementing time-windowed CPU sampling (e.g., measure delta over the last 5-10 seconds) or clarify in metrics naming/docs that this is a lifetime average. If you keep the current approach, rename the metric to
cpu_percent_lifetime_avgfor clarity.package.json (1)
29-29: Specify Node.js engines field in package.json
CI enforces Node.js v18 (≥16), satisfyingcheck-disk-space@3.4.0’s requirement; add an"engines": { "node": ">=16" }entry to package.json for clarity and upstream tooling support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.prettierrc(1 hunks)package.json(3 hunks)src/middlewares/metrics.middleware.ts(1 hunks)src/routes/health.routes.ts(3 hunks)src/routes/metrics.routes.ts(1 hunks)src/services/MetricsService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/metrics.routes.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/middlewares/metrics.middleware.ts (2)
src/utils/subscriptionScheduler.ts (1)
start(14-29)src/services/MetricsService.ts (1)
metricsService(524-524)
src/routes/health.routes.ts (1)
src/config/redisConfig.ts (1)
redisClient(78-78)
src/services/MetricsService.ts (3)
src/services/ConfigurationService.ts (1)
configurationService(780-780)src/config/redisConfig.ts (1)
redisClient(78-78)src/utils/mailer.ts (1)
sendEmail(11-31)
🪛 GitHub Check: build-and-test
src/middlewares/metrics.middleware.ts
[warning] 25-25:
'err' is defined but never used
[warning] 13-13:
'err' is defined but never used
🔇 Additional comments (7)
src/routes/health.routes.ts (2)
95-116: LGTM! Latency measurement added correctly.The
/dbendpoint now properly measures and reports query latency, providing valuable observability for database performance monitoring.
175-237: LGTM! Robust per-dependency health checks.The
/dependenciesendpoint now implements separate try-catch blocks for each dependency check (Stellar, Redis, Database) with individual latency measurements. This addresses the previously flagged fragile error identification logic and provides granular failure information.src/middlewares/metrics.middleware.ts (2)
10-16: LGTM! Error swallowing is appropriate here.The middleware correctly swallows errors from
observeRequestto prevent metrics collection failures from impacting the request lifecycle. The unused catch binding flagged by static analysis is intentional and acceptable in this context.
21-28: LGTM! Periodic collector implementation is correct.The periodic collector safely invokes system and external status checks with error swallowing, ensuring that failures in metrics collection don't crash the scheduler.
src/services/MetricsService.ts (3)
229-305: LGTM! External checks now have proper timeouts.The implementation correctly wraps DB, Redis, and Stellar checks with bounded timeouts using
Promise.raceandAbortController, preventing the endpoints from hanging under failure scenarios. Latency measurements and error handling are appropriate.
393-473: LGTM! Prometheus exposition format is correct.The Prometheus metrics exposition properly escapes label values (lines 398-403) and follows the 0.0.4 format specification with HELP and TYPE comments. The label value escaping handles backslashes, quotes, newlines, and carriage returns correctly.
476-521: LGTM! Alert rate limiting and email delivery are sound.The alert system implements proper rate limiting (10-minute minimum interval per alert key) and gracefully handles email failures without crashing. The use of configurable recipient lists and structured error logging is appropriate.
3 participants
Summary
Key Changes
Added
MetricsService.tsmetrics.routes.tsapp.tshealth.routes.tsUpdated
index.tsTypeScript Fixes
metrics.routes.tscausing server-side parsing issues.voidper Express v5 types.getConfig.MetricsService.ts.Exposed Endpoints
/metrics/summary/metrics/historicalmetric,windowMinutes,aggregation/metrics/prometheus/metrics/dashboard/health/system/health/dependencies/health/dbAlerting Configuration
METRICS_API_LATENCY_THRESHOLD_MSMETRICS_DB_LATENCY_THRESHOLD_MSMETRICS_CPU_THRESHOLDMETRICS_MEMORY_THRESHOLDMETRICS_ALERT_EMAILSDependencies
prom-clientcheck-disk-spaceHow to Test
/metrics/dashboardto view live charts./metrics/summary/metrics/historical/metrics/prometheus.envto trigger alerts and confirm email notifications are sent./health/systemand/health/dependenciesreturn correct metrics and latency data.Risks & Considerations
check-disk-space.Checklist
Summary by CodeRabbit
New Features
Chores