jaegermcp: enforce MaxSpanDetailsPerRequest in get_span_details#8174
jaegermcp: enforce MaxSpanDetailsPerRequest in get_span_details#8174jkowall wants to merge 2 commits intojaegertracing:mainfrom
Conversation
Signed-off-by: Jonah Kowall <jkowall@kowall.net>
|
Porting over Yuri's review note from the fork PR for context:
Addressed in this PR by:
My reply: updated accordingly. The handler now relies on validated config instead of carrying its own backup default. |
There was a problem hiding this comment.
Pull request overview
This PR makes the get_span_details MCP tool respect the existing MaxSpanDetailsPerRequest configuration, preventing unbounded span ID requests from driving excessive CPU/memory work.
Changes:
- Pass
MaxSpanDetailsPerRequestfrom the MCP server into theget_span_detailshandler. - Enforce the max span ID count in
get_span_detailsrequest validation. - Strengthen config validation and expand unit test coverage for invalid config and oversized requests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cmd/jaeger/internal/extension/jaegermcp/server.go | Wires MaxSpanDetailsPerRequest into the get_span_details handler constructor. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_span_details.go | Adds a per-request span_ids length guard based on configured max. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/get_span_details_test.go | Updates handler construction in tests and adds coverage for oversized span_ids. |
| cmd/jaeger/internal/extension/jaegermcp/config.go | Adds an explicit low-end validation for MaxSpanDetailsPerRequest. |
| cmd/jaeger/internal/extension/jaegermcp/config_test.go | Adds a test case for too-low MaxSpanDetailsPerRequest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Jonah Kowall <jkowall@kowall.net>
bae483a to
c47b392
Compare
|
|
||
| if len(input.SpanIDs) > h.maxSpanDetailsPerRequest { | ||
| return querysvc.GetTraceParams{}, fmt.Errorf( | ||
| "span_ids must not exceed %d items", |
There was a problem hiding this comment.
| "span_ids must not exceed %d items", | |
| "Number of input Span IDs must not exceed %d items", |
| ServerVersion string `mapstructure:"server_version" valid:"required"` | ||
|
|
||
| // MaxSpanDetailsPerRequest limits the number of spans that can be fetched in a single request. | ||
| MaxSpanDetailsPerRequest int `mapstructure:"max_span_details_per_request" valid:"range(1|100)"` |
| if cfg.MaxSpanDetailsPerRequest < 1 { | ||
| return fmt.Errorf("max_span_details_per_request must be at least 1") | ||
| } | ||
| if cfg.MaxSpanDetailsPerRequest > 100 { |
There was a problem hiding this comment.
why do this manually instead of via govalidator.ValidateStruct ?
Motivation
get_span_detailsaccepted unboundedspan_idslists even though the MCP config already exposedMaxSpanDetailsPerRequest.Description
MaxSpanDetailsPerRequestinto theget_span_detailshandler from the server.span_idsexceed the configured limit.MaxSpanDetailsPerRequestinConfig.Validate()so the handler can rely on it.span_idsrequest case.Testing
make fmt/make linthad to be executed manually because this Windows + WSL environment cannot execute the repo shebang scripts directly).go test -race ./cmd/jaeger/internal/extension/jaegermcp/....make test; it still fails in this environment due unrelated existing failures incmd/anonymizer/app/uiconvandinternal/uimodel/converter/v1/json.