mirror of
https://github.com/PrefectHQ/fastmcp.git
synced 2026-08-24 06:24:18 +02:00
Address Jeremiah's telemetry feedback
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 <noreply@anthropic.com>
This commit is contained in:
parent
88182d2dae
commit
ad660c0c43
3 changed files with 24 additions and 58 deletions
|
|
@ -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__ = [
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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 == {}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue