From ad660c0c4345aaaf56ec4794e4d10bbc9b1879c7 Mon Sep 17 00:00:00 2001 From: Chris Guidry Date: Wed, 14 Jan 2026 09:29:56 -0500 Subject: [PATCH] Address Jeremiah's telemetry feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A few improvements based on code review: - Don't override existing trace context in `extract_trace_context` - if we're already in a valid trace (e.g., from HTTP propagation), preserve it rather than extracting from MCP meta - Add exception recording to `delegate_span` to match `server_span` pattern - Remove unused `get_meter` function (metrics not implemented yet) - Return `None` instead of `{}` from `inject_trace_context` when nothing to inject - Clean up trivial tests that were just testing OpenTelemetry's own API 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.5 --- src/fastmcp/server/telemetry.py | 8 ++++++- src/fastmcp/telemetry.py | 35 ++++++++++++++--------------- tests/telemetry/test_module.py | 39 +-------------------------------- 3 files changed, 24 insertions(+), 58 deletions(-) diff --git a/src/fastmcp/server/telemetry.py b/src/fastmcp/server/telemetry.py index cec9b893e..21adf5c23 100644 --- a/src/fastmcp/server/telemetry.py +++ b/src/fastmcp/server/telemetry.py @@ -98,6 +98,7 @@ def delegate_span( """Create an INTERNAL span for provider delegation. Used by FastMCPProvider when delegating to mounted servers. + Automatically records any exception on the span and sets error status. """ tracer = get_tracer() with tracer.start_as_current_span(f"delegate {name}") as span: @@ -107,7 +108,12 @@ def delegate_span( "fastmcp.component.key": component_key, } ) - yield span + try: + yield span + except Exception as e: + span.record_exception(e) + span.set_status(Status(StatusCode.ERROR)) + raise __all__ = [ diff --git a/src/fastmcp/telemetry.py b/src/fastmcp/telemetry.py index a843dc870..b30695b56 100644 --- a/src/fastmcp/telemetry.py +++ b/src/fastmcp/telemetry.py @@ -24,9 +24,8 @@ Example usage with SDK: from typing import Any from opentelemetry import context as otel_context -from opentelemetry import propagate +from opentelemetry import propagate, trace from opentelemetry.context import Context -from opentelemetry.metrics import get_meter as otel_get_meter from opentelemetry.trace import Span, Status, StatusCode, Tracer from opentelemetry.trace import get_tracer as otel_get_tracer @@ -48,26 +47,17 @@ def get_tracer(version: str | None = None) -> Tracer: return otel_get_tracer(INSTRUMENTATION_NAME, version) -def get_meter(version: str | None = None): - """Get the FastMCP meter for recording metrics. - - Args: - version: Optional version string for the instrumentation - - Returns: - A meter instance. Returns a no-op meter if no SDK is configured. - """ - return otel_get_meter(INSTRUMENTATION_NAME, version or "") - - -def inject_trace_context(meta: dict[str, Any] | None = None) -> dict[str, Any]: +def inject_trace_context( + meta: dict[str, Any] | None = None, +) -> dict[str, Any] | None: """Inject current trace context into a meta dict for MCP request propagation. Args: meta: Optional existing meta dict to merge with trace context Returns: - A new dict containing the original meta (if any) plus trace context keys + A new dict containing the original meta (if any) plus trace context keys, + or None if no trace context to inject and meta was None """ carrier: dict[str, str] = {} propagate.inject(carrier) @@ -80,7 +70,7 @@ def inject_trace_context(meta: dict[str, Any] | None = None) -> dict[str, Any]: if trace_meta: return {**(meta or {}), **trace_meta} - return meta or {} + return meta def record_span_error(span: Span, exception: BaseException) -> None: @@ -92,13 +82,21 @@ def record_span_error(span: Span, exception: BaseException) -> None: def extract_trace_context(meta: dict[str, Any] | None) -> Context: """Extract trace context from an MCP request meta dict. + If already in a valid trace (e.g., from HTTP propagation), the existing + trace context is preserved and meta is not used. + Args: meta: The meta dict from an MCP request (ctx.request_context.meta) Returns: An OpenTelemetry Context with the extracted trace context, - or the current context if no trace context found + or the current context if no trace context found or already in a trace """ + # Don't override existing trace context (e.g., from HTTP propagation) + current_span = trace.get_current_span() + if current_span.get_span_context().is_valid: + return otel_context.get_current() + if not meta: return otel_context.get_current() @@ -118,7 +116,6 @@ __all__ = [ "TRACE_PARENT_KEY", "TRACE_STATE_KEY", "extract_trace_context", - "get_meter", "get_tracer", "inject_trace_context", "record_span_error", diff --git a/tests/telemetry/test_module.py b/tests/telemetry/test_module.py index f769fe2fe..2fd74cb77 100644 --- a/tests/telemetry/test_module.py +++ b/tests/telemetry/test_module.py @@ -2,23 +2,13 @@ from __future__ import annotations -from opentelemetry.metrics import Meter, NoOpMeter from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter -from opentelemetry.trace import NonRecordingSpan, Tracer from fastmcp.server.telemetry import get_auth_span_attributes -from fastmcp.telemetry import INSTRUMENTATION_NAME, get_meter, get_tracer +from fastmcp.telemetry import INSTRUMENTATION_NAME, get_tracer class TestGetTracer: - def test_returns_tracer(self): - tracer = get_tracer() - assert isinstance(tracer, Tracer) - - def test_returns_tracer_with_version(self): - tracer = get_tracer("1.0.0") - assert isinstance(tracer, Tracer) - def test_tracer_uses_instrumentation_name( self, trace_exporter: InMemorySpanExporter ): @@ -32,35 +22,8 @@ class TestGetTracer: assert scope is not None assert scope.name == INSTRUMENTATION_NAME - def test_tracer_noop_without_sdk(self): - # Without SDK configured, spans are non-recording - from opentelemetry.trace import ProxyTracer - - tracer = get_tracer() - # ProxyTracer wraps real or no-op tracer - assert isinstance(tracer, (Tracer, ProxyTracer)) - span = tracer.start_span("test") - # Non-recording spans don't capture data - assert isinstance(span, NonRecordingSpan) or span.is_recording() - - -class TestGetMeter: - def test_returns_meter(self): - meter = get_meter() - assert isinstance(meter, (Meter, NoOpMeter)) - - def test_returns_meter_with_version(self): - meter = get_meter("1.0.0") - assert isinstance(meter, (Meter, NoOpMeter)) - - -class TestInstrumentationName: - def test_instrumentation_name(self): - assert INSTRUMENTATION_NAME == "fastmcp" - class TestGetAuthSpanAttributes: def test_returns_empty_dict_when_no_context(self): - # No request context available attrs = get_auth_span_attributes() assert attrs == {}