[jaegermcp] Fix searchDepth clamped to zero when maxResults is unlimited#8250
[jaegermcp] Fix searchDepth clamped to zero when maxResults is unlimited#8250lopster568 wants to merge 1 commit intojaegertracing:mainfrom
Conversation
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>
There was a problem hiding this comment.
Pull request overview
Fixes an edge case in the Jaeger MCP search_traces handler where searchDepth was incorrectly clamped to 0 when MaxSearchResults is set to 0 (intended as “unlimited”), causing storage queries to request zero results.
Changes:
- Guard
searchDepthclamping logic so it only applies whenmaxResults > 0. - Add a unit test ensuring
searchDepthis not clamped whenmaxResults == 0.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces.go | Fixes clamping condition to avoid forcing searchDepth to 0 when maxResults is unlimited. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces_test.go | Adds regression coverage for the unlimited (maxResults=0) case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // When maxResults=0 (unlimited), searchDepth must not be clamped to 0 | ||
| assert.Equal(t, 50, query.SearchDepth) | ||
| return func(yield func([]ptrace.Traces, error) bool) { |
There was a problem hiding this comment.
This test assumes maxResults=0 is a supported “unlimited” configuration, but the Jaeger MCP config currently validates max_search_results as range(1|1000) (so 0 would be rejected). Consider either (a) updating the config validation/docs to explicitly allow 0 for unlimited, or (b) adding a brief note here explaining why 0 is still a meaningful/possible value for searchTracesHandler.maxResults (e.g., internal usage) to avoid confusion for future maintainers.
Considering that we have config validation for it, what is the path for this value to be zero? |
github-actions
bot
added
the
waiting-for-author
|
You're right, config validation enforces The zero path exists only in unit tests where handlers are constructed directly (e.g. Happy to close this if you think it's not worth the churn. |
github-actions
bot
removed
the
waiting-for-author
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8250 +/- ##
==========================================
+ Coverage 95.64% 95.67% +0.03%
==========================================
Files 319 319
Lines 16804 16804
==========================================
+ Hits 16072 16078 +6
+ Misses 578 574 -4
+ Partials 154 152 -2
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:
|
|
production code should not be sanity-checking every single parameter at every single turn just because the unit tests might invoke internal functions. |
github-actions
bot
added
the
waiting-for-author
Which problem is this PR solving?
Follow-up to #8242. When
MaxSearchResultsis0(unlimited), thesearchDepthclamp incorrectly evaluates10 > 0as true and setssearchDepthto0. This causes the storage query to request zero results even though the config intends no limit.Description of the changes
One-line guard:
if h.maxResults > 0 && searchDepth > h.maxResultsWithout the guard, the default
searchDepthof 10 is always clamped to 0 whenmaxResultsis 0 (unlimited), makingsearch_tracesreturn empty results.How was this change tested?
TestSearchTracesHandler_Handle_SearchDepthNotClampedWhenUnlimited— setsmaxResults=0withsearchDepth=50, asserts the query receivessearchDepth=50unchangedmake fmt && make lint && make test— all greenChecklist
make lintandmake testsuccessfully