feat: emit tool-call duration histogram metric#38
Conversation
Off by default. Opt in via CRDB_MCP_OTEL_FILE (JSON spans to a file) or OTEL_EXPORTER_OTLP_ENDPOINT (OTLP gRPC to any collector, honoring standard OTEL_* env vars). Neither set: providers stay no-op. A receiving middleware wraps each MCP tools/call in a span named after the tool. db/adapter hand-instruments Query and Exec with SQL child spans; db.query.text is redacted via cockroachdb-parser (FmtAnonymize + FmtHideConstants), matching managed-service, so no user data lands in span attributes. Zap records also mirror to the OTel log pipeline via the otelzap bridge; trace-context propagation onto log records is a follow-up.
Export mcp.tool.call.duration (seconds) keyed by tool name and outcome via the existing opt-in OTel pipeline. Histogram accuracy survives log sampling, so operators get p50/p95/p99, throughput, and error rate even with tool-call logs dropped.
| return nil, nil, nil, errors.Wrap(err, "build stdout log exporter") | ||
| return nil, nil, nil, nil, errors.Wrap(err, "build stdout log exporter") | ||
| } | ||
| me, err := stdoutmetric.New(stdoutmetric.WithWriter(f)) |
There was a problem hiding this comment.
🟢 Confidence: 42 (Low)
Bug (inconsistent cleanup): When stdoutmetric.New() fails, only the file is closed — the already-created te (trace exporter) and le (log exporter) are leaked. Contrast with the OTLP path (lines 155-159) which correctly shuts down all previously-created exporters on failure. Consider adding _ = te.Shutdown(ctx) and _ = le.Shutdown(ctx) (or at minimum closing them) before the return, matching the OTLP error-handling pattern. Note: the same leak existed pre-PR for te when stdoutlog.New() failed (line 135), but this PR extends the pattern to a third exporter.
| // tool name and outcome. Unlike per-call logs, histogram accuracy survives | ||
| // log sampling, so operators can derive p50/p95/p99, throughput, and error | ||
| // rate even with tool-call logs dropped. No-op until otel.Setup installs an | ||
| // exporter. |
There was a problem hiding this comment.
🟢 Confidence: 20 (Low)
Potential high-cardinality issue: mcp.tool.name is derived from the request's Name field via toolName(req). Since this middleware runs before the tool handler dispatches (and validates) the tool name, a misbehaving client could send arbitrary/fabricated tool names, creating unbounded metric label cardinality in the histogram. Over time this can exhaust memory in the MeterProvider. Consider validating the tool name against a known set of registered tools, or adding a cardinality guard (e.g., mapping unknown tool names to a single "unregistered" bucket). In practice MCP clients are typically trusted local processes, so the risk is low.
3dee420 to
2a9532e
Compare
2a9532e to
e3e5e0c
Compare
|
|
2 participants
Stacked on #28. Addresses the review feedback there about emitting tool-call latency as an OTel histogram so MCP performance can be measured off the metrics pipeline, with accuracy that survives log sampling.
Changes
mcp.tool.call.duration(units) via a newToolCallMetricsmiddleware, keyed bymcp.tool.nameandmcp.tool.call.outcome(success,error,tool_error).otel.Setupbehind the same opt-in gate:stdoutmetricin file mode,otlpmetricgrpcin OTLP mode, flushed on shutdown.toolNamehelper so the span and metrics middleware agree on theunknown_toolfallback.Testing
middleware/metrics_test.go: manual-reader assertions for all three outcomes plus the non-tool-method no-op path.otel/otel_test.go: file-mode Setup flushes the recorded histogram on shutdown.go test ./...passes.