[jaegermcp] Enforce config-driven limits in MCP handlers#8194
Conversation
Signed-off-by: Ritesh Tripathi <ritesh6263tripathi@gmail.com>
2d3be50 to
825da4a
Compare
There was a problem hiding this comment.
Pull request overview
Enforces Jaeger MCP extension configuration limits at runtime by threading configured maximums into the MCP tool handlers and applying them during request handling, preventing unbounded payload sizes and keeping behavior consistent with jaeger_mcp config.
Changes:
- Pass
MaxSearchResultsandMaxSpanDetailsPerRequestfromserver.gointo thesearch_tracesandget_span_detailshandlers. - Replace hardcoded
search_tracesdepth/result caps with config-driven limits. - Add span count validation in
get_span_detailsand update/add unit tests for the new behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/jaeger/internal/extension/jaegermcp/server.go | Wires config limits into handler constructors when registering MCP tools. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces.go | Removes hardcoded caps; clamps search_depth using injected configured maximum. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_span_details.go | Enforces max spans per request via injected configured maximum. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces_test.go | Updates tests to include the new handler limit field/constructor signature. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_span_details_test.go | Updates tests for new signature and adds coverage for exceeding the span limit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Remove hardcoded 'max: 100' reference from SearchDepth field documentation - Clarify that maximum is controlled by server configuration (MaxSearchResults) - Align documentation with config-driven limit enforcement implementation Signed-off-by: Ritesh Tripathi <ritesh6263tripathi@gmail.com>
Signed-off-by: Ritesh Tripathi <ritesh6263tripathi@gmail.com>
a6babd7 to
3ecb2b7
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes Jaeger MCP limit enforcement by wiring configuration-defined limits into MCP tool handlers and applying those limits at runtime to prevent unexpectedly large responses.
Changes:
- Pass
MaxSearchResultsandMaxSpanDetailsPerRequestfromserver.gointo thesearch_tracesandget_span_detailshandlers. - Replace
search_traceshardcoded caps with a handler-configured maximum. - Enforce
MaxSpanDetailsPerRequestinget_span_details, including a new unit test for the over-limit case.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/jaeger/internal/extension/jaegermcp/server.go | Passes configured limits into handler constructors during tool registration. |
| cmd/jaeger/internal/extension/jaegermcp/server_test.go | Updates server tests’ config setup to include required limit fields. |
| cmd/jaeger/internal/extension/jaegermcp/internal/types/search_traces.go | Updates schema/docs to reflect config-controlled maximum for search_depth. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces.go | Adds maxResults to handler and clamps SearchDepth using the configured limit. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces_test.go | Updates tests for the new handler struct/constructor signature. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_span_details.go | Adds per-request span ID count enforcement based on configured limit. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_span_details_test.go | Updates tests for new constructor signature and adds an exceeds-limit test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Ritesh Tripathi <ritesh6263tripathi@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8194 +/- ##
=======================================
Coverage 95.62% 95.63%
=======================================
Files 318 318
Lines 16771 16780 +9
=======================================
+ Hits 16038 16047 +9
Misses 578 578
Partials 155 155
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks! |
) ## Which problem is this PR solving? When an agent calls `search_traces`, the response includes `service_count` (an integer) but not the actual service names. The agent knows a trace spans 5 services but has no idea which ones. To find out, it must call `get_trace_topology` or `get_span_details` for every trace individually, which defeats the purpose of a lightweight summary endpoint. The data is already computed internally. `buildTraceSummary` builds a `services` map to derive `service_count`, then discards the map keys. This has been the case since the function was introduced in #7858, where the map was built but only `len(services)` was surfaced. Subsequent changes (#7859, #7863, #7916, #8194) restructured the types and renamed fields but never revisited the service data gap. ## Short description of the changes - Added `Services []string` to `TraceSummary`, populated from the existing `services` map - Sorted alphabetically via `slices.Sort` for deterministic output across calls - Added multi-service test with three services in non-alphabetical order, unique span IDs, and proper parent-child relationships to verify sort correctness - Updated existing summary tests to assert on the new field - `ServiceCount` preserved for backward compatibility ## Use case An agent investigating a latency spike searches for slow traces. The summary now returns: ```json { "service_count": 3, "services": ["api-gateway", "payment", "user-service"] } ``` The agent can immediately see that the payment service is involved and drill into that trace, instead of blindly fetching topology for every result. ## How was this change tested? - `go test ./cmd/jaeger/internal/extension/jaegermcp/...` - all passing - `make lint` - 0 issues - `make fmt` - clean - `make test` - 3053 tests passing ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully: `make lint test` ## AI Usage in this PR (choose one) - [x] **Light**: AI provided minor assistance (formatting, simple suggestions) Signed-off-by: Roshan Singh <roshansingh7890@gmail.com> Signed-off-by: Roshan <rosh.s568@gmail.com>
Which problem is this PR solving?
Fixes #8193
This PR addresses an issue where configuration-defined limits in the Jaeger MCP extension were not being enforced.
Specifically:
MaxSearchResultswas defined but not used insearch_traceshandler (hardcoded limits were used instead)MaxSpanDetailsPerRequestwas defined but not enforced inget_span_detailshandlerThis resulted in potential unbounded payload sizes and inconsistency between configuration and runtime behavior.
Description of the changes
MaxSearchResultsandMaxSpanDetailsPerRequestfromserver.gointo MCP handlerssearch_traceswith config-driven limitsget_span_detailsto enforceMaxSpanDetailsPerRequestHow was this change tested?
make testTestGetSpanDetailsHandler_ExceedsLimit) to verify:Checklist
make lint testAI Usage in this PR (choose one)