diff --git a/gitnexus/src/cli/mcp.ts b/gitnexus/src/cli/mcp.ts index 6c0924799..e6edebdf1 100644 --- a/gitnexus/src/cli/mcp.ts +++ b/gitnexus/src/cli/mcp.ts @@ -55,11 +55,6 @@ export const mcpCommand = async () => { import('../mcp/metrics.js'), ]); - // OpenTelemetry metrics + Prometheus exporter. Opt-in via GITNEXUS_OTEL_METRICS. - // In stdio mode (this entrypoint), each agent host spawns its own process, so - // metrics are off by default. Operators who want server-side observability - // should run `gitnexus serve` and scrape that instance instead. The opt-in - // path still works here for local development / single-process setups. try { const result = await initMetrics(); if (result.enabled) { @@ -69,8 +64,6 @@ export const mcpCommand = async () => { ); } } catch (err: any) { - // Metrics must never crash the server. EADDRINUSE here is the common case; - // log and continue without metrics. logger.warn( { err: err?.message ?? String(err) }, 'GitNexus: failed to start metrics endpoint — continuing without metrics', diff --git a/gitnexus/src/cli/serve.ts b/gitnexus/src/cli/serve.ts index a0aedf1c3..9f7e2c341 100644 --- a/gitnexus/src/cli/serve.ts +++ b/gitnexus/src/cli/serve.ts @@ -32,9 +32,6 @@ export const serveCommand = async (options?: { port?: string; host?: string }) = const host = options?.host ?? 'localhost'; try { - // OpenTelemetry metrics + Prometheus exporter. Opt-in via - // GITNEXUS_OTEL_METRICS. `gitnexus serve` is the supported scrape target - // (see src/mcp/metrics.ts for the rationale). try { const { initMetrics } = await import('../mcp/metrics.js'); const result = await initMetrics(); @@ -45,7 +42,6 @@ export const serveCommand = async (options?: { port?: string; host?: string }) = ); } } catch (metricsErr: any) { - // Never crash the server because of metrics. Log and continue. logger.warn( { err: metricsErr?.message ?? String(metricsErr) }, '[gitnexus serve] failed to start metrics endpoint — continuing without metrics', diff --git a/gitnexus/src/mcp/metrics.ts b/gitnexus/src/mcp/metrics.ts index c390cf326..1925315dc 100644 --- a/gitnexus/src/mcp/metrics.ts +++ b/gitnexus/src/mcp/metrics.ts @@ -1,27 +1,8 @@ /** - * MCP server-side observability — OpenTelemetry metrics with Prometheus exposition. - * - * Answers issue #1351: provides ground-truth "is this MCP server being used?" - * for operators who scrape `/metrics`. - * - * Design constraints from the maintainer's framing: - * - OpenTelemetry format (not custom JSON, not pino). - * - Anonymous: no per-call identifiers, no input data, no error message text. - * Cypher parse errors echo user input, so error is recorded as a boolean only. - * - Prometheus: pull model via `/metrics`. No collector required. - * - Quantized: histograms with explicit buckets. No raw per-call values escape. - * - * Privacy boundary is type-enforced: observe() takes `(result) => boolean`, - * never a message-returning detector. The error label is `error="true|false"`, - * never a message. - * - * Initialization is opt-in via GITNEXUS_OTEL_METRICS=on. Bound to 127.0.0.1 by - * default. The exporter is configured to suppress `target_info` and scope info - * so the only labels emitted are `tool` (closed set) and `error` (boolean). - * - * Stdio mode emits no metrics by default — each agent host spawns its own - * process and binding ephemeral ports is operationally useless. Supported - * scrape target is `gitnexus serve`. + * Error messages from MCP tool handlers (notably cypher parse errors) echo + * user input, so `observe()` accepts a boolean error detector only — never + * the error string. Exporter is configured to suppress target_info / + * otel_scope_info so the label set stays bounded to {tool, error, le}. */ import type { Counter, Histogram, UpDownCounter } from '@opentelemetry/api'; @@ -30,18 +11,9 @@ import { PrometheusExporter } from '@opentelemetry/exporter-prometheus'; const METER_NAME = 'gitnexus.mcp'; -// Quantized buckets. Once published, these become a dashboard contract; -// boundaries are intentionally coarse so we don't have to recut them. -// -// Duration: tools that hit the in-memory graph (`context`, `query` on a small -// repo) commonly return in 1–30ms; `impact` / `detect_changes` on a large repo -// can run multiple seconds. 1ms..30s covers cold-cache outliers. +// Bucket boundaries are a published dashboard contract; do not recut without +// coordinating with scrape consumers. export const DURATION_BUCKETS_SECONDS = [0.001, 0.005, 0.025, 0.1, 0.5, 2.5, 10, 30]; - -// Result size: tool responses range from a few-hundred-byte error envelopes to -// multi-megabyte `context`/`cypher` payloads. The modal `context`/`route_map` -// answer lands in 2–8 KB; boundaries are chosen so that range straddles a -// boundary rather than collapsing into one bucket. export const RESULT_BYTES_BUCKETS = [512, 2048, 8192, 32768, 131072, 524288, 2097152]; let provider: MeterProvider | null = null; @@ -58,10 +30,6 @@ function envFlag(value: string | undefined): boolean { } export interface InitMetricsOptions { - /** - * Override the env-driven master switch. Use in tests to force initialization - * without setting process env. - */ forceEnabled?: boolean; } @@ -72,13 +40,7 @@ export interface InitMetricsResult { endpoint?: string; } -/** - * Initialize the OTel meter provider and start the Prometheus exporter HTTP - * server. No-op when GITNEXUS_OTEL_METRICS is unset, empty, off, false, or 0. - * - * Safe to call multiple times — second and later calls return the existing - * state without re-binding the port. - */ +/** Idempotent: subsequent calls return the bound state without re-binding. */ export async function initMetrics( opts: InitMetricsOptions = {}, ): Promise { @@ -103,9 +65,8 @@ export async function initMetrics( host, port, endpoint, - // Privacy: suppress target_info (would ship every resource attribute as a - // separate metric series) and otel_scope_info. Only labels we want to - // expose are `tool` and `error`. + // target_info would publish every resource attribute as its own series — + // keep the label set bounded. withoutTargetInfo: true, withoutScopeInfo: true, appendTimestamp: false, @@ -131,10 +92,8 @@ export async function initMetrics( ], }); - // Use the provider directly rather than the global registration. Each - // process gets exactly one provider (init is idempotent above); skipping the - // global avoids state-leak across vitest fork boundaries when tests cycle - // init/shutdown repeatedly. + // Provider stays local — global registration would leak state across + // repeat init/shutdown cycles. const meter = provider.getMeter(METER_NAME); requestsCounter = meter.createCounter('gitnexus_mcp_tool_requests_total', { @@ -155,17 +114,11 @@ export async function initMetrics( description: 'Currently executing MCP tool handlers, labelled by tool.', }); - // Wait until the exporter HTTP server is actually accepting connections so - // callers can integration-test the endpoint immediately after init. await exporter.startServer(); return { enabled: true, port, host, endpoint }; } -/** - * Shut down the meter provider and exporter HTTP server. Safe to call when - * init was a no-op (resets nothing). Use in graceful-shutdown paths. - */ export async function shutdownMetrics(): Promise { const p = provider; const e = exporter; @@ -179,21 +132,7 @@ export async function shutdownMetrics(): Promise { if (p) await p.shutdown().catch(() => {}); } -/** - * Wrap an MCP tool handler with metric instrumentation. Records: - * - requests counter (incremented once per call, with `error` label) - * - duration histogram (in seconds) - * - result bytes histogram (only on success — error-path size is meaningless) - * - in-flight up/down counter - * - * The `hasError` detector returns a boolean only. The result's error message - * is intentionally not accepted at this seam — error messages echo user input - * (cypher parse errors include the offending clause) and are an anonymity - * leak. - * - * No-op when init was disabled: the metric handles are null, calls are - * skipped, work runs untouched. - */ +/** hasError must be a boolean; never accept the error string — cypher errors echo user input. */ export async function observe( tool: string, fn: () => Promise, @@ -223,9 +162,6 @@ export async function observe( } return result; } catch (err) { - // Thrown errors are converted to error envelopes by the MCP handler upstream, - // so this branch is reached only on truly exceptional failures (transport, - // sentinel, programming errors). Record as error=true with zero bytes. const durationSeconds = Number(process.hrtime.bigint() - start) / 1e9; const labels = { tool, error: 'true' }; requestsCounter?.add(1, labels); @@ -236,9 +172,6 @@ export async function observe( } } -/** - * Returns true if metrics are currently initialized. Test helper. - */ export function isMetricsEnabled(): boolean { return provider !== null; } diff --git a/gitnexus/src/mcp/server.ts b/gitnexus/src/mcp/server.ts index 4894d826f..cda3c4bf9 100644 --- a/gitnexus/src/mcp/server.ts +++ b/gitnexus/src/mcp/server.ts @@ -163,10 +163,6 @@ export function createMCPServer(backend: LocalBackend): Server { })), })); - // Handle tool calls — append next-step hints to guide agent workflow. - // The body is wrapped in `observe()` (see `./metrics.ts`) so each call is - // recorded against the OTel meter when metrics are enabled. The wrapper is - // a no-op when metrics are disabled. server.setRequestHandler(CallToolRequestSchema, async (request) => { const { name, arguments: args } = request.params; @@ -201,16 +197,12 @@ export function createMCPServer(backend: LocalBackend): Server { } }, (result) => { - // Size = bytes of the text payload only. The envelope structure - // itself is not user data. const block = result.content?.[0]; if (block && 'text' in block && typeof block.text === 'string') { return Buffer.byteLength(block.text, 'utf8'); } return 0; }, - // hasError must return a boolean only. Never accept the error message - // string at this seam — messages echo user input. (result) => result.isError === true, ); }); diff --git a/gitnexus/test/unit/mcp/metrics.test.ts b/gitnexus/test/unit/mcp/metrics.test.ts index a776b4cf3..edaea736a 100644 --- a/gitnexus/test/unit/mcp/metrics.test.ts +++ b/gitnexus/test/unit/mcp/metrics.test.ts @@ -1,16 +1,3 @@ -/** - * Tests for src/mcp/metrics.ts — the OTel + Prometheus seam for issue #1351. - * - * Two privacy-critical invariants land here: - * 1. `observe()` accepts a boolean error detector only, never a message string. - * (Type-enforced; tested by passing a result with an explicit message and - * asserting the message does not appear in the exposed metrics text.) - * 2. The Prometheus exporter is configured to suppress `target_info` and - * scope info — only `tool` and `error` labels should appear. - * - * Lifecycle: each test that turns metrics on must `shutdownMetrics()` in afterEach, - * otherwise the exporter HTTP server leaks port + handles across tests. - */ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { initMetrics, @@ -31,8 +18,7 @@ function clearMetricsEnv(): void { } async function withRandomPort(fn: () => Promise): Promise { - // Use port 0 by hand-rolling a free port — Node http will pick one but the - // exporter wants a concrete number. Bind/listen/close pattern. + // Exporter needs a concrete port number, not 0 — pre-bind to discover one. const net = await import('node:net'); const srv = net.createServer(); const port = await new Promise((resolve, reject) => { @@ -113,7 +99,6 @@ describe('observe() — success path', () => { expect(text).toContain( 'gitnexus_mcp_tool_requests_total{tool="list_repos",error="false"} 1', ); - // Histogram is exported by the OTel Prom format with _bucket / _sum / _count. expect(text).toContain('gitnexus_mcp_tool_request_duration_seconds_count'); expect(text).toContain('gitnexus_mcp_tool_result_bytes_count'); }); @@ -138,7 +123,6 @@ describe('observe() — error path', () => { const text = await fetch(`http://127.0.0.1:${init.port}/metrics`).then((r) => r.text()); expect(text).toContain('gitnexus_mcp_tool_requests_total{tool="cypher",error="true"} 1'); - // PRIVACY INVARIANT: error messages echo user input. They MUST NOT leak. expect(text).not.toContain(SECRET_ECHO); expect(text).not.toContain('parse error'); }); @@ -186,8 +170,6 @@ describe('observe() — in-flight balance', () => { } const text = await fetch(`http://127.0.0.1:${init.port}/metrics`).then((r) => r.text()); - // After all calls complete (regardless of success/failure), inflight returns to 0. - // Prom emits the latest value; assert it's a "0" reading for impact. expect(text).toMatch(/gitnexus_mcp_tool_inflight\{tool="impact"\} 0\b/); }); }); @@ -208,7 +190,6 @@ describe('privacy: target_info / scope_info suppression', () => { const text = await fetch(`http://127.0.0.1:${init.port}/metrics`).then((r) => r.text()); expect(text).not.toMatch(/^target_info/m); expect(text).not.toMatch(/otel_scope_info/); - // The exposed label set is bounded: only `tool` and `error` ever appear. const labelLines = text.split('\n').filter((l) => l.includes('gitnexus_mcp_')); for (const line of labelLines) { const match = line.match(/\{([^}]*)\}/);