fix(jaegermcp): Enforce response limits#8242
Conversation
MaxSpanDetailsPerRequest and MaxSearchResults were defined in the config but not consistently enforced across all MCP handlers: - get_trace_errors: collected all error spans without any cap; now continues iterating to count total errors but caps Spans slice at maxSpanDetailsPerRequest. ErrorCount reflects the true total. - get_trace_topology: collected all spans without any cap; now breaks once maxSpanDetailsPerRequest is reached - search_traces: aggregation loop had no hard cap; now breaks once maxResults is reached. Fixed searchDepth clamping to guard with maxResults > 0 so unlimited (0) is not incorrectly clamped to 0. - server.go: wire MaxSpanDetailsPerRequest into NewGetTraceErrorsHandler and NewGetTraceTopologyHandler constructors Tests added: - TestGetTraceErrorsHandler_Handle_LimitEnforced: 5 error spans, limit 3, asserts ErrorCount==5 and len(Spans)<=3 - TestGetTraceTopologyHandler_Handle_LimitEnforced: 6-span trace, limit 3, asserts len(Spans)<=3 - TestSearchTracesHandler_Handle_LimitEnforced: 5 traces, limit 3, asserts len(Traces)<=3 Fixes jaegertracing#8118 Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR enforces configured response-size limits in the Jaeger MCP extension handlers so MCP tool outputs stay bounded and consistent with MaxSpanDetailsPerRequest / MaxSearchResults.
Changes:
- Wire
MaxSpanDetailsPerRequestintoget_trace_errorsandget_trace_topologyhandlers and enforce span caps. - Enforce
MaxSearchResultsinsearch_tracesresult aggregation and fixsearchDepthclamping for the “unlimited” (0) case. - Add unit tests covering the new limit enforcement behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/jaeger/internal/extension/jaegermcp/server.go | Pass configured span limits into newly updated handler constructors. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_errors.go | Track total error count while capping collected span details. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_errors_test.go | Add test for get_trace_errors truncation + total error count. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_topology.go | Add span-detail limit to handler and stop collecting spans after cap. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_topology_test.go | Add test ensuring topology output is capped by the limit. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces.go | Stop adding summaries after maxResults; guard searchDepth clamp when maxResults==0. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces_test.go | Add test ensuring search results are capped. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_topology.go
Show resolved
Hide resolved
cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_errors.go
Show resolved
Hide resolved
cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces_test.go
Show resolved
Hide resolved
cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_topology_test.go
Outdated
Show resolved
Hide resolved
cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_errors_test.go
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8242 +/- ##
=======================================
Coverage 95.65% 95.65%
=======================================
Files 319 319
Lines 16795 16804 +9
=======================================
+ Hits 16065 16074 +9
Misses 577 577
Partials 153 153
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:
|
- Replace AggregateTraces with AggregateTracesWithLimit in get_trace_errors and get_trace_topology handlers so that memory is bounded at aggregation time, not just at response time. - Tighten limit-enforcement tests to use assert.Equal instead of assert.LessOrEqual, ensuring the handler returns exactly maxSpanDetailsPerRequest spans when the trace exceeds the limit. Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace assert.Equal(t, N, len(x)) with assert.Len(t, x, N) in all three limit-enforcement tests as required by the testifylint linter. Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_errors.go
Outdated
Show resolved
Hide resolved
cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_trace_errors_test.go
Outdated
Show resolved
Hide resolved
cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces.go
Outdated
Show resolved
Hide resolved
- Revert get_trace_errors to AggregateTraces (unbounded) so ErrorCount reflects the true total of error spans in the full trace; Spans is already capped inside the iteration loop via maxSpanDetailsPerRequest. - Update test to assert ErrorCount==5 (full trace) and len(Spans)==3 (capped), matching the intended split semantics. - Remove the redundant `h.maxResults > 0` guard in search_traces since config validation already enforces MaxSearchResults in [1, 1000]. Signed-off-by: abhay1999 <abhaychaurasiya19@gmail.com>
When MaxSearchResults is 0 (unlimited), the searchDepth clamp `if searchDepth > h.maxResults` evaluates as `10 > 0 == true` and sets searchDepth to 0. This causes the storage query to return no results even though the config intends no limit. Guard the clamp with `h.maxResults > 0` so that an unlimited config preserves the requested or default search depth. Found while reviewing jaegertracing#8242. Signed-off-by: Roshan <rosh.s568@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Roshan <rosh.s568@gmail.com>
What this PR does
MaxSpanDetailsPerRequestandMaxSearchResultswere defined in the config but not consistently enforced across all MCP handlers. This PR wires the limits all the way through.Changes
get_trace_errors.goSpansslice atmaxSpanDetailsPerRequestErrorCountnow reflects the true total error count, not the truncated slice lengthget_trace_topology.gomaxSpanDetailsPerRequestis reachedsearch_traces.gomaxResultsis reachedsearchDepthclamping: guards withmaxResults > 0so unlimited (0) is not incorrectly clamped to0server.goMaxSpanDetailsPerRequestintoNewGetTraceErrorsHandlerandNewGetTraceTopologyHandlerconstructorsTests added
TestGetTraceErrorsHandler_Handle_LimitEnforced— 5 error spans, limit 3, assertsErrorCount==5andlen(Spans)<=3TestGetTraceTopologyHandler_Handle_LimitEnforced— 6-span trace, limit 3, assertslen(Spans)<=3TestSearchTracesHandler_Handle_LimitEnforced— 5 traces, limit 3, assertslen(Traces)<=3Fixes #8118