feat(observability): add mcp client initialize metrics - #17
Conversation
|
| Filename | Overview |
|---|---|
| internal/mcpserver/server.go | Adds recordInitializeMetrics receiving middleware that fires only on successful initialize method calls, extracting ClientInfo fields and the MCP-Protocol-Version header before delegating to RecordMCPInitialize; type-assertion and nil guards are correct. |
| internal/observability/attrs.go | Adds five new attribute constants, InitializeAttrs struct, its Attributes() method, and the metricString helper; design is consistent with existing attr types and correctly falls back to "unknown" for empty/whitespace values while omitting the optional header attr when absent. |
| internal/observability/mcp.go | Introduces mcpMetricSet with an atomic.Pointer sink and one OTel Int64Counter, following the same pattern as cache.go; RecordMCPInitialize writes to both the OTel counter and the optional sink. |
| internal/observability/mcp_test.go | Tests the sink-write path with a recording sink; correct in isolation, but uses t.Parallel() while mutating the package-level mcpMetrics.sink, which is fragile for future parallel tests that call initMCPMetrics. |
| internal/observability/attrs_test.go | Adds TestInitializeAttrs (whitespace trimming), TestInitializeAttrsDefaultsMissingClientInfo (omitted header attr, "unknown" fallbacks), and a shared assertStringAttr helper; thorough and self-contained. |
| internal/observability/observability.go | Wires initMCPMetrics symmetrically alongside the existing initCacheMetrics call in both the backend-present and no-backend branches; minimal and correct change. |
Sequence Diagram
sequenceDiagram
participant Client as MCP Client
participant Middleware as recordInitializeMetrics
participant Handler as mcp.MethodHandler
participant Obs as RecordMCPInitialize
participant OTel as OTel Counter
participant Sink as MetricSink
Client->>Middleware: initialize(params, MCP-Protocol-Version header)
Middleware->>Handler: next(ctx, "initialize", req)
Handler-->>Middleware: result, err
alt "err == nil"
Middleware->>Obs: recordInitialize(ctx, req)
Obs->>Obs: extract ClientInfo + ProtocolVersion + header
Obs->>OTel: mcp.initialize.count.Add(ctx, 1, attrs)
Obs->>Sink: Count(ctx, "mcp.initialize", 1, attrs)
end
Middleware-->>Client: result, err
Reviews (1): Last reviewed commit: "fix: add `t.Parallel()`" | Re-trigger Greptile
| func TestRecordMCPInitializeWritesMetricSink(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| sink := &recordingMetricSink{} | ||
| initMCPMetrics(sink) | ||
| t.Cleanup(func() { initMCPMetrics(nil) }) | ||
|
|
There was a problem hiding this comment.
Parallel test modifying package-level global sink
TestRecordMCPInitializeWritesMetricSink calls t.Parallel() and then immediately mutates the package-level mcpMetrics.sink via initMCPMetrics(sink). There are no other tests in this package that also call initMCPMetrics, so the pattern is safe today. However, if a future test similarly uses t.Parallel() and calls initMCPMetrics, the two tests could observe each other's sink even though the atomic pointer prevents data races at the memory level — the logical isolation would break and one test's metric emission could land in the wrong sink, producing a flaky count mismatch. Consider either running this test without t.Parallel() or using a per-test mcpMetricSet instead of the shared global.
initializecalls with standardclientInfofields for client name, title, and versioninitialize.params.protocolVersionand the streamable httpMCP-Protocol-Versionheader when presentmcp.initializemetric through the existing observability sink with focused attribute and sink tests