fix: include _internal/mcp in observability tracing paths - #4144
fix: include _internal/mcp in observability tracing paths#4144ecthelion77 wants to merge 1 commit into
Conversation
|
Suggested labels: |
479336e to
b8ca51e
Compare
3ccf338 to
e1b7026
Compare
e1b7026 to
edffeb5
Compare
edffeb5 to
497a427
Compare
497a427 to
6382d50
Compare
6382d50 to
02951c1
Compare
Signed-off-by: Olivier Gintrand <olivier.gintrand@forterro.com>
02951c1 to
8ab9346
Compare
jonpspri
left a comment
There was a problem hiding this comment.
Thank you for this fix, @ecthelion77 — the diagnosis in #4152 is accurate and the instinct to add /_internal/mcp to the tracing include-list is correct. However, two things have changed since this PR was opened in April that mean it can no longer be merged as-is.
1 — The fix is already in main via a different mechanism
The architecture for path-based trace gating has moved. Since this PR was opened, mcpgateway/observability.py has grown a dedicated _should_trace_request_path() predicate (used by OpenTelemetryRequestMiddleware) that unconditionally enables tracing for /_internal/mcp in code:
# mcpgateway/observability.py (current main, ~line 816)
if normalized.startswith("/_internal/mcp/"):
return TrueThat guard was added as part of the affinity-path OTEL work (PR #6164) and is present on main today. The otel_trace_include_patterns config list is no longer the single gate for this path, so adding the regex there would have no effect on the runtime behaviour described in #4152.
2 — Collateral deletion of unrelated security fields
The diff also removes three fields from Settings that are unrelated to tracing:
- meta_max_keys: int = Field(default=16, ...) # CWE-400 guard
- meta_max_depth: int = Field(default=2, ...) # CWE-400 guard
- meta_max_bytes: int = Field(default=4096, ...) # CWE-400 guardThese fields are still present and active on main — they cap user-supplied meta_data forwarded to upstream MCP servers to guard against unbounded input (CWE-400). Merging this PR would silently drop them, removing a security control. This looks like a rebase conflict resolution that went the wrong way.
Closing this PR
Given that:
- the bug described in #4152 is already fixed on
main, - the PR carries a regression against active security fields, and
- the
otel_trace_include_patternsconfig approach no longer matches the current tracing architecture,
I'm going to close this PR. Issue #4152 will be closed separately once verified against current main.
If there is a separate desire to also expose /_internal/mcp via otel_trace_include_patterns so operators can override it — that would be a new, narrowly-scoped PR against the current code. But the runtime problem from #4152 is already resolved.
Thank you again for the contribution — the investigation and direction were correct, the implementation just got overtaken by subsequent work.
|
Closing — see review comment above for the full rationale. The runtime fix for #4152 is already on |
🐛 Bug-fix PR
🔗 Issue
Closes #4152
📌 Summary
The
_internal/mcpendpoint path is not included in the observability tracing include patterns, causing internal MCP calls between gateway components to be invisible in distributed traces. This makes it impossible to trace the full request flow when meta-servers or virtual servers invoke internal MCP endpoints.🔁 Reproduction Steps
/_internal/mcp//_internal/mcpleg is missing from the trace🐞 Root Cause
The
OTEL_TRACE_INCLUDE_PATTERNSdefault inconfig.pylists explicit path patterns for tracing (/servers/*/sse,/servers/*/message,/a2a), but/_internal/mcpwas omitted.💡 Fix Description
Add
r"^/_internal/mcp(?:/|$)"to the defaultOTEL_TRACE_INCLUDE_PATTERNSlist inSettings, so internal MCP calls are traced by default.🧪 Verification
make lintmake testmake coverage/_internal/mcpspans📐 MCP Compliance (if relevant)
✅ Checklist
make black isort pre-commit)