diff --git a/apps/server/instrumentation.mjs b/apps/server/instrumentation.mjs index 5d6b49a32..75df64558 100644 --- a/apps/server/instrumentation.mjs +++ b/apps/server/instrumentation.mjs @@ -16,7 +16,6 @@ import { env } from 'node:process' import { registerInstrumentations } from '@opentelemetry/instrumentation' -import { HttpInstrumentation } from '@opentelemetry/instrumentation-http' import { IORedisInstrumentation } from '@opentelemetry/instrumentation-ioredis' import { PgInstrumentation } from '@opentelemetry/instrumentation-pg' @@ -27,8 +26,8 @@ import { PgInstrumentation } from '@opentelemetry/instrumentation-pg' // no OLD-name consumer exists, so we go straight to STABLE-only — no `http/dup` // transition phase, no doubled cardinality. // Source: node_modules/.pnpm/@opentelemetry+instrumentation-http@0.215.0/.../build/src/http.js L25-72 -// MUST run before `new HttpInstrumentation(...)` below — its constructor reads -// the env var once and caches the result. +// MUST run before `new HttpInstrumentation(...)` (created in src/libs/otel.ts) — +// its constructor reads the env var once and caches the result. // // Use a truthy check (not `??=`): `process.env.X` is `''` when the platform // (e.g. Railway) registers the var without a value, and `??=` does NOT override @@ -48,11 +47,29 @@ if (!env.OTEL_SEMCONV_STABILITY_OPT_IN) { // until you query Prometheus and notice STABLE-name series are missing. console.info(`[otel-preload] OTEL_SEMCONV_STABILITY_OPT_IN=${env.OTEL_SEMCONV_STABILITY_OPT_IN}`) +// NOTICE: +// HttpInstrumentation is INTENTIONALLY constructed in src/libs/otel.ts (not +// here) and passed to NodeSDK's `instrumentations` config. Reason: the OTel +// metrics API does NOT have a proxy mechanism like traces — instruments +// created against a NoopMeterProvider stay noop forever. If we register +// HttpInstrumentation in this preload, its constructor caches a noop meter +// (because no MeterProvider is set yet), then `_recordServerDuration` writes +// to NoopHistogram for the entire process lifetime, and +// `http_server_request_duration_seconds_*` never appears in Prometheus. +// +// Putting it in NodeSDK config lets the SDK call `setMeterProvider` with the +// real provider at start(), which re-runs `_updateMetricInstruments()` and +// upgrades the histograms to real instruments. The patches it installs are +// `Server.prototype.emit` (incoming) — prototype-level, race-immune, so it +// doesn't matter that they install at SDK start instead of preload. +// +// pg / ioredis can stay here because they only emit spans, and the trace API +// DOES have a proxy that upgrades cleanly when the SDK installs its provider. +// +// Removal condition: when @opentelemetry/api adds a proxy MeterProvider that +// upgrades cached meters retroactively, all three can move back here. registerInstrumentations({ instrumentations: [ - new HttpInstrumentation({ - ignoreIncomingRequestHook: req => req.url === '/health', - }), new PgInstrumentation({ enhancedDatabaseReporting: true, }), diff --git a/apps/server/src/libs/otel.ts b/apps/server/src/libs/otel.ts index eeff9973d..5ad55afb2 100644 --- a/apps/server/src/libs/otel.ts +++ b/apps/server/src/libs/otel.ts @@ -10,6 +10,7 @@ import { logs, SeverityNumber } from '@opentelemetry/api-logs' import { OTLPLogExporter } from '@opentelemetry/exporter-logs-otlp-proto' import { OTLPMetricExporter } from '@opentelemetry/exporter-metrics-otlp-proto' import { OTLPTraceExporter } from '@opentelemetry/exporter-trace-otlp-proto' +import { HttpInstrumentation } from '@opentelemetry/instrumentation-http' import { RuntimeNodeInstrumentation } from '@opentelemetry/instrumentation-runtime-node' import { resourceFromAttributes } from '@opentelemetry/resources' import { BatchLogRecordProcessor } from '@opentelemetry/sdk-logs' @@ -214,11 +215,36 @@ export function initOtel(env: Env): OtelInstance | undefined { exportTimeoutMillis: 10_000, })], logRecordProcessors: [new BatchLogRecordProcessor(logExporter)], - // NOTICE: HttpInstrumentation, PgInstrumentation, and IORedisInstrumentation - // are registered in instrumentation.cjs (loaded via --require) so that - // require-in-the-middle can patch the CJS modules before tsx's ESM loader - // imports them. Only non-patching instrumentations belong here. + // NOTICE: PgInstrumentation and IORedisInstrumentation are registered in + // instrumentation.mjs (loaded via --import) so require-in-the-middle can + // patch their CJS modules before tsx's ESM loader caches them. They only + // emit spans (not metrics), and the trace API has a proxy that upgrades + // a noop tracer to the real one when the SDK starts — so registering + // early is safe for them. + // + // HttpInstrumentation MUST be here (NodeSDK config) and NOT in the + // preload, because: + // - It records `http.server.request.duration` to a Histogram instrument + // created against `this.meter`. + // - The OTel metrics API does NOT have a proxy mechanism (see comment + // below at sdk.start()). A meter obtained before the real + // MeterProvider is installed becomes a permanent NoopMeter, and the + // histogram inside it silently swallows every record() call. + // - NodeSDK calls setMeterProvider on its config-passed instrumentations + // AT start time, after the real provider is installed. That path + // re-runs `_updateMetricInstruments()` and gives the instrumentation + // a real histogram. + // - The patch HttpInstrumentation installs is `Server.prototype.emit` + // (incoming) — prototype-level, race-immune. Patching at SDK-start + // time instead of preload time still catches every Server instance + // created later. + // Source: node_modules/.../@opentelemetry+api/.../api/metrics.js + // (`getMeterProvider()` returns NoopMeterProvider until setGlobalMeterProvider + // is called; cached meters are not retroactively upgraded.) instrumentations: [ + new HttpInstrumentation({ + ignoreIncomingRequestHook: req => req.url === '/health', + }), new RuntimeNodeInstrumentation(), ], })