ruicesar/InfiniteAPI
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(metrics,retry): address gold standard code review

Browse Source 
Prometheus Metrics:
- Add METRICS_LABELS fallback for defaultLabels env config
- Separate includeSystem from collectDefaultMetrics with own env var
- Parse URL in /metrics endpoint to support querystrings and trailing slashes
- Sort keys in labelsToKey for stable/deterministic lookups
- Update documentation with all environment variables

Retry Utils:
- Remove abort listener on sleep completion to prevent memory leak
- Apply maxDelay cap AFTER jitter to guarantee max delay limit
- Validate/normalize attempt parameter in calculateDelay
- Count retries only when attempt > 1 (actual retry, not first try)
- Use metrics.retryExhausted instead of generic errors for exhausted retries

Tests:
- Add consistency tests to verify retryConfigs.rsocket matches constants
This commit is contained in:
Claude
2026-01-20 19:37:19 +00:00
parent 1944645556
commit 6f057b8895
3 changed files with 86 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Utils/prometheus-metrics.ts
+33 -10
View File
@@ -20,13 +20,16 @@
* - Circuit breaker metrics
* - Environment variable configuration
*
* Configuration via environment variables:
* Configuration via environment variables (supports BAILEYS_PROMETHEUS_* and METRICS_* prefixes):
* - BAILEYS_PROMETHEUS_ENABLED: Enable/disable metrics (default: false)
* - BAILEYS_PROMETHEUS_PORT: Port for HTTP metrics server (default: 9092)
* - BAILEYS_PROMETHEUS_HOST: Host/IP to bind (default: 127.0.0.1)
* - BAILEYS_PROMETHEUS_PATH: Path for metrics endpoint (default: /metrics)
* - BAILEYS_PROMETHEUS_PREFIX: Prefix for all metrics (default: baileys)
* - BAILEYS_PROMETHEUS_LABELS: JSON string with default labels (e.g. {"environment":"production"})
* - BAILEYS_PROMETHEUS_COLLECT_DEFAULT: Collect default Node.js metrics (default: true)
* - BAILEYS_PROMETHEUS_INCLUDE_SYSTEM: Include system metrics like CPU/memory (default: true)
* - BAILEYS_PROMETHEUS_COLLECT_INTERVAL_MS: Interval for system metrics collection (default: 10000)
*
* @module Utils/prometheus-metrics
*/
@@ -140,8 +143,11 @@ export function loadMetricsConfig(): MetricsConfig {
host: process.env.BAILEYS_PROMETHEUS_HOST || process.env.METRICS_HOST || '127.0.0.1',
path: process.env.BAILEYS_PROMETHEUS_PATH || process.env.METRICS_PATH || '/metrics',
prefix: process.env.BAILEYS_PROMETHEUS_PREFIX || process.env.METRICS_PREFIX || 'baileys',
defaultLabels: parseLabelsFromEnv(process.env.BAILEYS_PROMETHEUS_LABELS),
includeSystem: (process.env.BAILEYS_PROMETHEUS_COLLECT_DEFAULT ?? process.env.METRICS_INCLUDE_SYSTEM) !== 'false',
// Support both env prefixes for labels
defaultLabels: parseLabelsFromEnv(process.env.BAILEYS_PROMETHEUS_LABELS ?? process.env.METRICS_LABELS),
// Separate flag for system metrics (CPU/memory) - independent from collectDefaultMetrics
includeSystem: (process.env.BAILEYS_PROMETHEUS_INCLUDE_SYSTEM ?? process.env.METRICS_INCLUDE_SYSTEM) !== 'false',
// Flag for prom-client default Node.js metrics
collectDefaultMetrics: (process.env.BAILEYS_PROMETHEUS_COLLECT_DEFAULT ?? process.env.METRICS_COLLECT_DEFAULT) !== 'false',
collectIntervalMs: parseInt(process.env.BAILEYS_PROMETHEUS_COLLECT_INTERVAL_MS || process.env.METRICS_COLLECT_INTERVAL_MS || '10000', 10),
}
@@ -161,6 +167,19 @@ export type MetricType = 'counter' | 'gauge' | 'histogram' | 'summary'
*/
export type Labels = Record<string, string>
/**
* Convert labels to a stable key string for lookups
* Sorts keys to ensure consistent ordering regardless of object property order
*/
function stableLabelsToKey(labels: Labels): string {
const sortedKeys = Object.keys(labels).sort()
const sortedObj: Labels = {}
for (const key of sortedKeys) {
sortedObj[key] = labels[key]!
}
return JSON.stringify(sortedObj)
}
/**
* Default histogram buckets (in ms)
*/
@@ -312,7 +331,7 @@ export class Counter implements BaseMetric {
}
private labelsToKey(labels: Labels): string {
return JSON.stringify(labels)
return stableLabelsToKey(labels)
}
/**
@@ -462,7 +481,7 @@ export class Gauge implements BaseMetric {
}
private labelsToKey(labels: Labels): string {
return JSON.stringify(labels)
return stableLabelsToKey(labels)
}
/**
@@ -662,7 +681,7 @@ export class Histogram implements BaseMetric {
}
private labelsToKey(labels: Labels): string {
return JSON.stringify(labels)
return stableLabelsToKey(labels)
}
/**
@@ -831,7 +850,7 @@ export class Summary implements BaseMetric {
}
private labelsToKey(labels: Labels): string {
return JSON.stringify(labels)
return stableLabelsToKey(labels)
}
/**
@@ -1202,7 +1221,12 @@ export class MetricsServer {
}
this.server = createServer(async (req: IncomingMessage, res: ServerResponse) => {
if (req.url === this.config.path && req.method === 'GET') {
// Parse URL to handle querystrings and trailing slashes
const parsedUrl = new URL(req.url || '/', `http://${req.headers.host || 'localhost'}`)
const pathname = parsedUrl.pathname.replace(/\/+$/, '') || '/' // Normalize trailing slashes
const configPath = this.config.path.replace(/\/+$/, '') || '/'
if (pathname === configPath && req.method === 'GET') {
try {
// Collect system metrics before responding
this.systemCollector?.collect()
@@ -1211,7 +1235,6 @@ export class MetricsServer {
res.writeHead(200, { 'Content-Type': this.registry.contentType() })
res.end(metricsOutput)
} catch (error) {
// FIX: More descriptive error message
const errorMessage = error instanceof Error ? error.message : 'Unknown error'
console.error(`[Prometheus] Error collecting metrics: ${errorMessage}`)
res.writeHead(500, { 'Content-Type': 'application/json' })
@@ -1221,7 +1244,7 @@ export class MetricsServer {
timestamp: new Date().toISOString()
}))
}
} else if (req.url === '/health' && req.method === 'GET') {
} else if (pathname === '/health' && req.method === 'GET') {
res.writeHead(200, { 'Content-Type': 'application/json' })
res.end(JSON.stringify({ status: 'ok', timestamp: new Date().toISOString() }))
} else {
src/Utils/retry-utils.ts
+25 -14
View File
@@ -131,15 +131,17 @@ export function calculateDelay(
multiplier: number,
jitter: number
): number {
// Normalize attempt to ensure valid calculation (must be >= 1)
const normalizedAttempt = Math.max(attempt, 1)
let delay: number
switch (strategy) {
case 'exponential':
delay = baseDelay * Math.pow(multiplier, attempt - 1)
delay = baseDelay * Math.pow(multiplier, normalizedAttempt - 1)
break
case 'linear':
delay = baseDelay * attempt
delay = baseDelay * normalizedAttempt
break
case 'constant':
@@ -147,7 +149,7 @@ export function calculateDelay(
break
case 'fibonacci': {
const fib = fibonacciNumber(attempt)
const fib = fibonacciNumber(normalizedAttempt)
delay = baseDelay * fib
break
}
@@ -155,7 +157,7 @@ export function calculateDelay(
case 'stepped': {
// Uses pre-defined delay array directly (ignores baseDelay/multiplier)
// Falls back to last delay if attempt exceeds array length
const index = Math.min(attempt - 1, RETRY_BACKOFF_DELAYS.length - 1)
const index = Math.min(normalizedAttempt - 1, RETRY_BACKOFF_DELAYS.length - 1)
delay = RETRY_BACKOFF_DELAYS[index] ?? RETRY_BACKOFF_DELAYS[0] ?? baseDelay
break
}
@@ -164,15 +166,15 @@ export function calculateDelay(
delay = baseDelay
}
// Apply max delay cap
delay = Math.min(delay, maxDelay)
// Apply jitter
// Apply jitter BEFORE capping to maxDelay
if (jitter > 0) {
const jitterAmount = delay * jitter
delay = delay + (Math.random() * 2 - 1) * jitterAmount
}
// Apply max delay cap AFTER jitter to ensure we never exceed maxDelay
delay = Math.min(delay, maxDelay)
return Math.max(0, Math.round(delay))
}
@@ -196,7 +198,15 @@ function fibonacciNumber(n: number): number {
*/
async function sleep(ms: number, signal?: AbortSignal): Promise<void> {
return new Promise((resolve, reject) => {
const timer = setTimeout(resolve, ms)
let abortHandler: (() => void) | undefined
const timer = setTimeout(() => {
// Cleanup abort listener on normal completion to prevent memory leak
if (signal && abortHandler) {
signal.removeEventListener('abort', abortHandler)
}
resolve()
}, ms)
if (signal) {
if (signal.aborted) {
@@ -205,7 +215,7 @@ async function sleep(ms: number, signal?: AbortSignal): Promise<void> {
return
}
const abortHandler = () => {
abortHandler = () => {
clearTimeout(timer)
reject(new RetryAbortedError(0))
}
@@ -324,8 +334,9 @@ export async function retry<T>(
result = await operation(context)
}
// Success
if (config.collectMetrics) {
// Success - only count as retry success if this wasn't the first attempt
if (config.collectMetrics && attempt > 1) {
// This was a successful retry (not first attempt)
metrics.retries.inc({ operation: config.operationName })
}
@@ -339,9 +350,9 @@ export async function retry<T>(
const shouldRetry = await config.shouldRetry(lastError, attempt)
if (!shouldRetry || attempt >= config.maxAttempts) {
// Final failure
// Final failure - use dedicated retry exhausted metric
if (config.collectMetrics) {
metrics.errors.inc({ category: 'retry', code: 'exhausted' })
metrics.retryExhausted.inc({ operation: config.operationName })
}
config.onFailure(lastError, attempt)
src/__tests__/Utils/retry-utils.test.ts
+28
View File
@@ -16,6 +16,8 @@ import {
retryConfigs,
getRetryDelayWithJitter,
getAllRetryDelaysWithJitter,
RETRY_BACKOFF_DELAYS,
RETRY_JITTER_FACTOR,
type RetryContext,
} from '../../Utils/retry-utils.js'
@@ -615,3 +617,29 @@ describe('getAllRetryDelaysWithJitter', () => {
expect(allSame).toBe(false)
})
})
describe('retryConfigs.rsocket consistency', () => {
/**
* This test ensures the hardcoded values in retryConfigs.rsocket
* stay synchronized with RETRY_BACKOFF_DELAYS and RETRY_JITTER_FACTOR.
*
* The values are hardcoded to avoid ESM initialization order issues,
* but this test will fail if someone updates the constants without
* updating retryConfigs.rsocket.
*/
it('should have maxAttempts equal to RETRY_BACKOFF_DELAYS.length', () => {
expect(retryConfigs.rsocket.maxAttempts).toBe(RETRY_BACKOFF_DELAYS.length)
})
it('should have baseDelay equal to RETRY_BACKOFF_DELAYS[0]', () => {
expect(retryConfigs.rsocket.baseDelay).toBe(RETRY_BACKOFF_DELAYS[0])
})
it('should have maxDelay equal to last RETRY_BACKOFF_DELAYS element', () => {
expect(retryConfigs.rsocket.maxDelay).toBe(RETRY_BACKOFF_DELAYS[RETRY_BACKOFF_DELAYS.length - 1])
})
it('should have jitter equal to RETRY_JITTER_FACTOR', () => {
expect(retryConfigs.rsocket.jitter).toBe(RETRY_JITTER_FACTOR)
})
})