Repository navigation
feat(observability): bootstrap OTel SDK + Prometheus metrics + Pino-style logger - #4974
KooshaPari wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a dependency-free observability stack for OmniRoute, featuring OpenTelemetry tracing, Prometheus metrics, and a Pino-compatible structured logger, complete with unit tests and documentation. The review feedback identifies several critical issues: a concurrency bug in trace propagation caused by using a global array instead of AsyncLocalStorage, a performance bottleneck from querying process.env on every log call, and a logic bug that freezes event loop lag measurements. Additionally, the feedback highlights a repository style guide violation regarding the placement of .env.example.observability in the root, dynamic mutation of the metrics label allow-list, and a potential runtime error from using require in an ES module environment.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const namedTracers = new Map<string, Tracer>(); | ||
|
|
||
| /** Active-span stack, scoped to the current async context. */ | ||
| const activeSpanStack: Span[] = []; |
There was a problem hiding this comment.
Using a global array activeSpanStack to track active spans is a critical concurrency bug in Node.js. Because Node.js handles concurrent asynchronous requests on a single thread, spans from different concurrent requests will interleave and corrupt the stack. This will lead to incorrect parent-child span relationships, context leaks across requests/tenants, and corrupted trace trees in production. You must use AsyncLocalStorage to store and propagate the active span stack or context safely across asynchronous boundaries.
| const activeSpanStack: Span[] = []; | |
| import { AsyncLocalStorage } from "node:async_hooks"; | |
| const activeSpanStorage = new AsyncLocalStorage<Span[]>(); |
| function getMinLevel(): number { | ||
| const resolved = resolveMinLevel(); | ||
| if (resolved !== cachedMinLevel) cachedMinLevel = resolved; | ||
| return cachedMinLevel; | ||
| } | ||
|
|
||
| function getFormat(): "json" | "pretty" { | ||
| const resolved = resolveFormat(); | ||
| if (resolved !== cachedFormat) cachedFormat = resolved; | ||
| return resolved; | ||
| } |
There was a problem hiding this comment.
Accessing process.env on every single log call (via resolveMinLevel() and resolveFormat()) is a major performance bottleneck. Accessing process.env in Node.js is expensive because it queries the operating system environment and crosses the JS/C++ boundary. On hot paths, this will severely degrade application throughput. Since you already have refreshFromEnv() and cached variables (cachedMinLevel, cachedFormat), you should return the cached values directly in getMinLevel() and getFormat(), and only update them via an explicit setter or a throttled/periodic check if dynamic updates are required.
function getMinLevel(): number {
return cachedMinLevel;
}
function getFormat(): "json" | "pretty" {
return cachedFormat;
}| function measureEventLoopLagMs(): number { | ||
| // Coarse but free: schedule an immediate and time how long it takes to | ||
| // fire. Averaged across the last 5 samples. | ||
| if (!lagSamples.length) { | ||
| for (let i = 0; i < 5; i++) { | ||
| const start = Date.now(); | ||
| setImmediate(() => lagSamples.push(Date.now() - start)); | ||
| } | ||
| return 0; | ||
| } | ||
| return lagSamples.reduce((a, b) => a + b, 0) / lagSamples.length; | ||
| } |
There was a problem hiding this comment.
The measureEventLoopLagMs() function has a logic bug where it only measures the event loop lag once during the very first call. Once lagSamples reaches a length of 5, the function always returns the average of those initial 5 samples and never schedules any new setImmediate measurements. As a result, the event loop lag metric will remain completely frozen at its startup value forever. To fix this, you should schedule a new measurement on every call and maintain a rolling window of the last 5 samples.
function measureEventLoopLagMs(): number {
const start = Date.now();
setImmediate(() => {
lagSamples.push(Date.now() - start);
if (lagSamples.length > 5) {
lagSamples.shift();
}
});
if (lagSamples.length === 0) return 0;
return lagSamples.reduce((a, b) => a + b, 0) / lagSamples.length;
}| @@ -0,0 +1,28 @@ | |||
| # OpenTelemetry env vars (PR-001 default) | |||
There was a problem hiding this comment.
The file .env.example.observability is placed in the project root. According to the Repository Style Guide (Section 1: File Placement & Organization), the project root must only contain specific allowed files, and .env.example.observability is not on that list. Please merge these environment variables into the existing .env.example file in the root to keep the root clean and adhere to the organization rules.
References
- The Project Root must only contain specific allowed configuration, dependency, documentation, and CI/CD files. Loose environment files other than .env.example are not permitted. (link)
| function setGauge(name: string, help: string, value: number): void { | ||
| // We rely on the registry to track the family; this helper is for | ||
| // process-level gauges that have no labels. Use a private synthetic family. | ||
| const fullName = name as keyof typeof LABEL_ALLOWLIST & string; | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (LABEL_ALLOWLIST as any)[fullName] = []; | ||
| const family = ensureFamily(fullName, help, "gauge"); |
There was a problem hiding this comment.
Dynamically mutating the LABEL_ALLOWLIST object at runtime using (LABEL_ALLOWLIST as any)[fullName] = [] is a bad practice that bypasses TypeScript's type safety and can lead to runtime errors if the object is frozen. Since the process metrics (omniroute_process_heap_bytes, omniroute_process_resident_bytes, omniroute_event_loop_lag_seconds) are static and well-known, they should be declared directly in the static LABEL_ALLOWLIST definition. This eliminates the need for dynamic mutation and the eslint-disable-next-line comment.
function setGauge(name: string, help: string, value: number): void {
const fullName = name as keyof typeof LABEL_ALLOWLIST & string;
const family = ensureFamily(fullName, help, "gauge");
let series = family.values.get("");
if (!series) {
series = { value, labels: {} };
family.values.set("", series);
} else {
series.value = value;
}| // Optional: hostname (skip in Electron + dev for privacy/portability). | ||
| if (detected !== "electron" && process.env.OMNIROUTE_DISABLE_HOSTNAME !== "1") { | ||
| try { | ||
| const { hostname } = require("os") as typeof import("os"); |
There was a problem hiding this comment.
Using require("os") directly in an ES module ("type": "module") will throw a ReferenceError: require is not defined at runtime in strict ESM environments (like standard Node.js ESM or Edge runtimes). Since this module is explicitly designed to be compatible with the Edge runtime, using require is highly risky. You should check if require is defined before calling it, or use standard environment variables like process.env.HOSTNAME / process.env.COMPUTERNAME as a safe fallback.
if (typeof require !== "undefined") {
const { hostname } = require("os") as typeof import("os");
const host = hostname();
if (host) resource[SEMANTIC_RESOURCE_KEYS.HOST_NAME] = host;
} else if (process.env.HOSTNAME) {
resource[SEMANTIC_RESOURCE_KEYS.HOST_NAME] = process.env.HOSTNAME;
}
What