fix(metrics): address PR #11 code review feedback

Major improvements to prometheus-metrics.ts:
- Use custom registry instead of global promClient.register (isolation)
- Remove duplicate localValues - prom-client is now single source of truth
- Fix reset(labels) to respect labels param using remove() instead of reset()
- Migrate Summary class to use prom-client internally
- getMetricsOutput() now uses customRegistry for proper isolation

Fix misleading comments in retry-utils.ts:
- Change "exponential backoff" to "custom progressive backoff"
- Fix comment claiming constants are "from Defaults" (now local)

Breaking changes to getValues()/get() - now async as they query prom-client
This commit is contained in:
Claude
2026-01-20 13:16:43 +00:00
parent b719349c57
commit 4924912f85
2 changed files with 192 additions and 167 deletions
+189 -164
View File
@@ -35,6 +35,9 @@ import { createServer, IncomingMessage, ServerResponse, Server } from 'http'
import * as os from 'os' import * as os from 'os'
import * as promClient from 'prom-client' import * as promClient from 'prom-client'
// Create a custom registry to avoid conflicts with global registry
const customRegistry = new promClient.Registry()
// ============================================ // ============================================
// Configuration // Configuration
// ============================================ // ============================================
@@ -177,7 +180,6 @@ export interface SummaryValue {
export class Counter implements BaseMetric { export class Counter implements BaseMetric {
readonly type = 'counter' as const readonly type = 'counter' as const
private promCounter: promClient.Counter<string> private promCounter: promClient.Counter<string>
private localValues: Map<string, MetricValue> = new Map() // For getValues() compatibility
constructor( constructor(
public name: string, public name: string,
@@ -188,6 +190,7 @@ export class Counter implements BaseMetric {
name, name,
help, help,
labelNames, labelNames,
registers: [customRegistry], // Use custom registry, not global
}) })
} }
@@ -209,54 +212,48 @@ export class Counter implements BaseMetric {
throw new Error('Counter cannot be decremented') throw new Error('Counter cannot be decremented')
} }
// Use prom-client // Use prom-client as single source of truth
if (Object.keys(labels).length > 0) { if (Object.keys(labels).length > 0) {
this.promCounter.labels(labels).inc(incValue) this.promCounter.labels(labels).inc(incValue)
} else { } else {
this.promCounter.inc(incValue) this.promCounter.inc(incValue)
} }
// Also track locally for getValues() compatibility
const key = this.labelsToKey(labels)
const existing = this.localValues.get(key)
if (existing) {
existing.value += incValue
existing.timestamp = Date.now()
} else {
this.localValues.set(key, {
labels,
value: incValue,
timestamp: Date.now(),
})
}
} }
/** /**
* Get current value (from local cache) * Get current value (async, from prom-client)
*/ */
get(labels: Labels = {}): number { async get(labels: Labels = {}): Promise<number> {
const metric = await this.promCounter.get()
const key = this.labelsToKey(labels) const key = this.labelsToKey(labels)
return this.localValues.get(key)?.value ?? 0 const value = metric.values.find(v => this.labelsToKey(v.labels as Labels) === key)
return value?.value ?? 0
} }
/** /**
* Reset the counter * Reset the counter
* If labels provided, removes only that label combination
* Otherwise resets all values
*/ */
reset(labels?: Labels): void { reset(labels?: Labels): void {
this.promCounter.reset() if (labels && Object.keys(labels).length > 0) {
if (labels) { // prom-client doesn't support removing specific label combinations
const key = this.labelsToKey(labels) // Use remove() to remove the specific label set
this.localValues.delete(key) this.promCounter.remove(labels)
} else { } else {
this.localValues.clear() this.promCounter.reset()
} }
} }
/** /**
* Get all values (from local cache for compatibility) * Get all values (async, from prom-client)
*/ */
getValues(): MetricValue[] { async getValues(): Promise<MetricValue[]> {
return Array.from(this.localValues.values()) const metric = await this.promCounter.get()
return metric.values.map(v => ({
labels: v.labels as Labels,
value: v.value,
}))
} }
private labelsToKey(labels: Labels): string { private labelsToKey(labels: Labels): string {
@@ -287,7 +284,6 @@ export class Counter implements BaseMetric {
export class Gauge implements BaseMetric { export class Gauge implements BaseMetric {
readonly type = 'gauge' as const readonly type = 'gauge' as const
private promGauge: promClient.Gauge<string> private promGauge: promClient.Gauge<string>
private localValues: Map<string, MetricValue> = new Map() // For getValues() compatibility
constructor( constructor(
public name: string, public name: string,
@@ -298,6 +294,7 @@ export class Gauge implements BaseMetric {
name, name,
help, help,
labelNames, labelNames,
registers: [customRegistry], // Use custom registry, not global
}) })
} }
@@ -315,20 +312,12 @@ export class Gauge implements BaseMetric {
setValue = value ?? 0 setValue = value ?? 0
} }
// Use prom-client // Use prom-client as single source of truth
if (Object.keys(labels).length > 0) { if (Object.keys(labels).length > 0) {
this.promGauge.labels(labels).set(setValue) this.promGauge.labels(labels).set(setValue)
} else { } else {
this.promGauge.set(setValue) this.promGauge.set(setValue)
} }
// Also track locally for getValues() compatibility
const key = this.labelsToKey(labels)
this.localValues.set(key, {
labels,
value: setValue,
timestamp: Date.now(),
})
} }
/** /**
@@ -345,22 +334,12 @@ export class Gauge implements BaseMetric {
incValue = value ?? 1 incValue = value ?? 1
} }
// Use prom-client // Use prom-client as single source of truth
if (Object.keys(labels).length > 0) { if (Object.keys(labels).length > 0) {
this.promGauge.labels(labels).inc(incValue) this.promGauge.labels(labels).inc(incValue)
} else { } else {
this.promGauge.inc(incValue) this.promGauge.inc(incValue)
} }
// Also track locally
const key = this.labelsToKey(labels)
const existing = this.localValues.get(key)
const currentValue = existing?.value ?? 0
this.localValues.set(key, {
labels,
value: currentValue + incValue,
timestamp: Date.now(),
})
} }
/** /**
@@ -377,22 +356,12 @@ export class Gauge implements BaseMetric {
decValue = value ?? 1 decValue = value ?? 1
} }
// Use prom-client // Use prom-client as single source of truth
if (Object.keys(labels).length > 0) { if (Object.keys(labels).length > 0) {
this.promGauge.labels(labels).dec(decValue) this.promGauge.labels(labels).dec(decValue)
} else { } else {
this.promGauge.dec(decValue) this.promGauge.dec(decValue)
} }
// Also track locally
const key = this.labelsToKey(labels)
const existing = this.localValues.get(key)
const currentValue = existing?.value ?? 0
this.localValues.set(key, {
labels,
value: currentValue - decValue,
timestamp: Date.now(),
})
} }
/** /**
@@ -403,31 +372,37 @@ export class Gauge implements BaseMetric {
} }
/** /**
* Get current value (from local cache) * Get current value (async, from prom-client)
*/ */
get(labels: Labels = {}): number { async get(labels: Labels = {}): Promise<number> {
const metric = await this.promGauge.get()
const key = this.labelsToKey(labels) const key = this.labelsToKey(labels)
return this.localValues.get(key)?.value ?? 0 const value = metric.values.find(v => this.labelsToKey(v.labels as Labels) === key)
return value?.value ?? 0
} }
/** /**
* Reset the gauge * Reset the gauge
* If labels provided, removes only that label combination
* Otherwise resets all values
*/ */
reset(labels?: Labels): void { reset(labels?: Labels): void {
this.promGauge.reset() if (labels && Object.keys(labels).length > 0) {
if (labels) { this.promGauge.remove(labels)
const key = this.labelsToKey(labels)
this.localValues.delete(key)
} else { } else {
this.localValues.clear() this.promGauge.reset()
} }
} }
/** /**
* Get all values (from local cache for compatibility) * Get all values (async, from prom-client)
*/ */
getValues(): MetricValue[] { async getValues(): Promise<MetricValue[]> {
return Array.from(this.localValues.values()) const metric = await this.promGauge.get()
return metric.values.map(v => ({
labels: v.labels as Labels,
value: v.value,
}))
} }
private labelsToKey(labels: Labels): string { private labelsToKey(labels: Labels): string {
@@ -476,7 +451,6 @@ export class Gauge implements BaseMetric {
export class Histogram implements BaseMetric { export class Histogram implements BaseMetric {
readonly type = 'histogram' as const readonly type = 'histogram' as const
private promHistogram: promClient.Histogram<string> private promHistogram: promClient.Histogram<string>
private localValues: Map<string, HistogramValue> = new Map() // For getValues() compatibility
private buckets: number[] private buckets: number[]
constructor( constructor(
@@ -491,6 +465,7 @@ export class Histogram implements BaseMetric {
help, help,
labelNames, labelNames,
buckets: this.buckets, buckets: this.buckets,
registers: [customRegistry], // Use custom registry, not global
}) })
} }
@@ -508,36 +483,12 @@ export class Histogram implements BaseMetric {
observeValue = value ?? 0 observeValue = value ?? 0
} }
// Use prom-client // Use prom-client as single source of truth
if (Object.keys(labels).length > 0) { if (Object.keys(labels).length > 0) {
this.promHistogram.labels(labels).observe(observeValue) this.promHistogram.labels(labels).observe(observeValue)
} else { } else {
this.promHistogram.observe(observeValue) this.promHistogram.observe(observeValue)
} }
// Also track locally for getValues() compatibility
const key = this.labelsToKey(labels)
let histValue = this.localValues.get(key)
if (!histValue) {
histValue = {
labels,
buckets: new Map(this.buckets.map((b) => [b, 0])),
sum: 0,
count: 0,
}
this.localValues.set(key, histValue)
}
// Increment appropriate buckets (cumulative)
for (const bucket of this.buckets) {
if (observeValue <= bucket) {
histValue.buckets.set(bucket, (histValue.buckets.get(bucket) ?? 0) + 1)
}
}
histValue.sum += observeValue
histValue.count++
} }
/** /**
@@ -565,31 +516,85 @@ export class Histogram implements BaseMetric {
} }
/** /**
* Get histogram values (from local cache) * Get histogram values (async, from prom-client)
*/ */
get(labels: Labels = {}): HistogramValue | undefined { async get(labels: Labels = {}): Promise<HistogramValue | undefined> {
const metric = await this.promHistogram.get()
const key = this.labelsToKey(labels) const key = this.labelsToKey(labels)
return this.localValues.get(key)
// Find values matching labels
const matchingValues = metric.values.filter(v => {
const vLabels = { ...v.labels } as Labels
delete (vLabels as Record<string, unknown>)['le'] // Remove bucket label
return this.labelsToKey(vLabels) === key
})
if (matchingValues.length === 0) return undefined
const buckets = new Map<number, number>()
let sum = 0
let count = 0
for (const v of matchingValues) {
if (v.metricName?.endsWith('_bucket')) {
const le = parseFloat((v.labels as Record<string, string>)['le'] ?? '0')
buckets.set(le, v.value)
} else if (v.metricName?.endsWith('_sum')) {
sum = v.value
} else if (v.metricName?.endsWith('_count')) {
count = v.value
}
}
return { labels, buckets, sum, count }
} }
/** /**
* Reset the histogram * Reset the histogram
* If labels provided, removes only that label combination
* Otherwise resets all values
*/ */
reset(labels?: Labels): void { reset(labels?: Labels): void {
this.promHistogram.reset() if (labels && Object.keys(labels).length > 0) {
if (labels) { this.promHistogram.remove(labels)
const key = this.labelsToKey(labels)
this.localValues.delete(key)
} else { } else {
this.localValues.clear() this.promHistogram.reset()
} }
} }
/** /**
* Get all values (from local cache for compatibility) * Get all values (async, from prom-client)
*/ */
getValues(): HistogramValue[] { async getValues(): Promise<HistogramValue[]> {
return Array.from(this.localValues.values()) const metric = await this.promHistogram.get()
const grouped = new Map<string, HistogramValue>()
for (const v of metric.values) {
const vLabels = { ...v.labels } as Labels
delete (vLabels as Record<string, unknown>)['le']
const key = this.labelsToKey(vLabels)
if (!grouped.has(key)) {
grouped.set(key, {
labels: vLabels,
buckets: new Map(),
sum: 0,
count: 0,
})
}
const histValue = grouped.get(key)!
if (v.metricName?.endsWith('_bucket')) {
const le = parseFloat((v.labels as Record<string, string>)['le'] ?? '0')
histValue.buckets.set(le, v.value)
} else if (v.metricName?.endsWith('_sum')) {
histValue.sum = v.value
} else if (v.metricName?.endsWith('_count')) {
histValue.count = v.value
}
}
return Array.from(grouped.values())
} }
/** /**
@@ -623,26 +628,32 @@ export class Histogram implements BaseMetric {
/** /**
* Summary class - value percentiles (quantiles) * Summary class - value percentiles (quantiles)
* Now backed by prom-client for full Prometheus compatibility
* *
* Summaries calculate quantiles over a sliding time window. * Summaries calculate quantiles over a sliding time window.
* Useful for tracking latency distributions. * Useful for tracking latency distributions.
*/ */
export class Summary implements BaseMetric { export class Summary implements BaseMetric {
readonly type = 'summary' as const readonly type = 'summary' as const
private values: Map<string, SummaryValue> = new Map() private promSummary: promClient.Summary<string>
private percentiles: number[] private percentiles: number[]
private maxAge: number // ms
private maxSize: number
constructor( constructor(
public name: string, public name: string,
public help: string, public help: string,
public labelNames: string[] = [], public labelNames: string[] = [],
options: { percentiles?: number[]; maxAge?: number; maxSize?: number } = {} options: { percentiles?: number[]; maxAgeSeconds?: number; ageBuckets?: number } = {}
) { ) {
this.percentiles = options.percentiles ?? DEFAULT_PERCENTILES this.percentiles = options.percentiles ?? DEFAULT_PERCENTILES
this.maxAge = options.maxAge ?? 600000 // 10 min this.promSummary = new promClient.Summary({
this.maxSize = options.maxSize ?? 1000 name,
help,
labelNames,
percentiles: this.percentiles,
maxAgeSeconds: options.maxAgeSeconds ?? 600, // 10 min default
ageBuckets: options.ageBuckets ?? 5,
registers: [customRegistry], // Use custom registry, not global
})
} }
/** /**
@@ -659,30 +670,11 @@ export class Summary implements BaseMetric {
observeValue = value ?? 0 observeValue = value ?? 0
} }
const key = this.labelsToKey(labels) // Use prom-client as single source of truth
let summaryValue = this.values.get(key) if (Object.keys(labels).length > 0) {
this.promSummary.labels(labels).observe(observeValue)
if (!summaryValue) { } else {
summaryValue = { this.promSummary.observe(observeValue)
labels,
values: [],
sum: 0,
count: 0,
}
this.values.set(key, summaryValue)
}
summaryValue.values.push(observeValue)
summaryValue.sum += observeValue
summaryValue.count++
// Limit size to prevent memory leaks
if (summaryValue.values.length > this.maxSize) {
const removed = summaryValue.values.shift()
if (removed !== undefined) {
summaryValue.sum -= removed
summaryValue.count--
}
} }
} }
@@ -699,46 +691,78 @@ export class Summary implements BaseMetric {
} }
/** /**
* Calculate percentile * Get summary values (async, from prom-client)
*/ */
getPercentile(labels: Labels, percentile: number): number | undefined { async get(labels: Labels = {}): Promise<SummaryValue | undefined> {
const metric = await this.promSummary.get()
const key = this.labelsToKey(labels) const key = this.labelsToKey(labels)
const summaryValue = this.values.get(key)
if (!summaryValue || summaryValue.values.length === 0) { // Find values matching labels
return undefined const matchingValues = metric.values.filter(v => {
const vLabels = { ...v.labels } as Labels
delete (vLabels as Record<string, unknown>)['quantile']
return this.labelsToKey(vLabels) === key
})
if (matchingValues.length === 0) return undefined
let sum = 0
let count = 0
for (const v of matchingValues) {
if (v.metricName?.endsWith('_sum')) {
sum = v.value
} else if (v.metricName?.endsWith('_count')) {
count = v.value
}
} }
const sorted = [...summaryValue.values].sort((a, b) => a - b) return { labels, values: [], sum, count }
const index = Math.ceil(percentile * sorted.length) - 1
return sorted[Math.max(0, index)]
}
/**
* Get summary values
*/
get(labels: Labels = {}): SummaryValue | undefined {
const key = this.labelsToKey(labels)
return this.values.get(key)
} }
/** /**
* Reset the summary * Reset the summary
* If labels provided, removes only that label combination
* Otherwise resets all values
*/ */
reset(labels?: Labels): void { reset(labels?: Labels): void {
if (labels) { if (labels && Object.keys(labels).length > 0) {
const key = this.labelsToKey(labels) this.promSummary.remove(labels)
this.values.delete(key)
} else { } else {
this.values.clear() this.promSummary.reset()
} }
} }
/** /**
* Get all values * Get all values (async, from prom-client)
*/ */
getValues(): SummaryValue[] { async getValues(): Promise<SummaryValue[]> {
return Array.from(this.values.values()) const metric = await this.promSummary.get()
const grouped = new Map<string, SummaryValue>()
for (const v of metric.values) {
const vLabels = { ...v.labels } as Labels
delete (vLabels as Record<string, unknown>)['quantile']
const key = this.labelsToKey(vLabels)
if (!grouped.has(key)) {
grouped.set(key, {
labels: vLabels,
values: [],
sum: 0,
count: 0,
})
}
const summaryValue = grouped.get(key)!
if (v.metricName?.endsWith('_sum')) {
summaryValue.sum = v.value
} else if (v.metricName?.endsWith('_count')) {
summaryValue.count = v.value
}
}
return Array.from(grouped.values())
} }
/** /**
@@ -844,18 +868,19 @@ export class MetricsRegistry {
/** /**
* Return metrics in Prometheus exposition format * Return metrics in Prometheus exposition format
* Uses prom-client's native metrics() for proper OpenMetrics format * Uses custom registry with configured prefix and defaultLabels
*/ */
async getMetricsOutput(): Promise<string> { async getMetricsOutput(): Promise<string> {
// Use prom-client's register for native Prometheus format // Use customRegistry (not global) for proper isolation
return await promClient.register.metrics() // Prefix and defaultLabels are applied when metrics are created
return await customRegistry.metrics()
} }
/** /**
* Return content type for Prometheus (using prom-client) * Return content type for Prometheus (using prom-client)
*/ */
contentType(): string { contentType(): string {
return promClient.register.contentType return customRegistry.contentType
} }
private formatLabels(labels: Labels): string { private formatLabels(labels: Labels): string {
+3 -3
View File
@@ -18,8 +18,8 @@ import { metrics } from './prometheus-metrics.js'
import type { CircuitBreaker } from './circuit-breaker.js' import type { CircuitBreaker } from './circuit-breaker.js'
/** /**
* Retry configuration with exponential backoff * Retry configuration with custom progressive backoff
* Delays in milliseconds: 1s, 2s, 5s, 10s, 20s * Fixed delay steps in milliseconds: 1s 2s 5s 10s 20s
* Defined locally to avoid circular dependency with Defaults * Defined locally to avoid circular dependency with Defaults
*/ */
const RETRY_BACKOFF_DELAYS = [1000, 2000, 5000, 10000, 20000] const RETRY_BACKOFF_DELAYS = [1000, 2000, 5000, 10000, 20000]
@@ -669,7 +669,7 @@ export const retryConfigs = {
/** /**
* Get retry delay with jitter applied * Get retry delay with jitter applied
* Uses RETRY_BACKOFF_DELAYS and RETRY_JITTER_FACTOR from Defaults * Uses RETRY_BACKOFF_DELAYS and RETRY_JITTER_FACTOR defined locally
* *
* @param attempt - Current attempt number (1-based) * @param attempt - Current attempt number (1-based)
* @returns Delay in ms with jitter applied * @returns Delay in ms with jitter applied