fix(memory): accept string-form error filters in trace search#8217
Conversation
…search_traces Signed-off-by: haibo <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR fixes Jaeger MCP’s search_traces.with_errors query construction to encode the internal error attribute as a boolean (instead of a string), aligning behavior across storage backends (notably the v2 memory store).
Changes:
- Update
search_tracesquery building to useattributes.PutBool("error", true). - Add a handler test that exercises
WithErrorsfiltering against the v2 memory store implementation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces.go | Switches the error attribute filter from string to boolean in buildQuery. |
| cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces_test.go | Adds a memory-store-backed test for WithErrors filtering and imports the v2 memory store. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: haibo <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8217 +/- ##
=======================================
Coverage 95.65% 95.65%
=======================================
Files 318 318
Lines 16804 16818 +14
=======================================
+ Hits 16074 16088 +14
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:
|
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
The UI sends all tag filters as strings today, and they are converted to Attributes object using PutStr:
jaeger/cmd/jaeger/internal/extension/jaegerquery/internal/util.go
Lines 28 to 34 in 4afa357
Why is this MCP tool need to do differently?
|
As you pointed out, existing query paths normalize tag filters to strings, for example in That means The inconsistency seems to be in the memory backend. In jaeger/internal/storage/v2/memory/tenant.go Lines 259 to 266 in 4afa357 Would you prefer the fix to be in the memory backend, by making it accept both string and boolean forms of the |
Signed-off-by: haibo <[email protected]>
YoungHypo
force-pushed
the
haibo-fix-error-type
branch
from
9f5b64e to
271b35b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Verify that error attribute filter is added | ||
| val, ok := query.Attributes.Get("error") | ||
| errorAttr, ok := query.Attributes.Get("error") | ||
| assert.True(t, ok) | ||
| assert.Equal(t, "true", val.Str()) | ||
| assert.Equal(t, "true", errorAttr.Str()) | ||
|
|
There was a problem hiding this comment.
This test still asserts the error query attribute is a string (errorAttr.Str() == "true"). Per the PR intent (and to avoid backend-specific behavior), the error attribute should be encoded as a boolean. Update the assertion to require a boolean type/value (e.g., errorAttr.Type() == pcommon.ValueTypeBool and errorAttr.Bool() == true), and ensure the handler builds the query accordingly.
There was a problem hiding this comment.
Disagree. This 271b35b reverts the error attribute from bool to string. The errorQueryValue() function in tenant.go adds backward compatibility to handle both types. The test correctly asserts string type.
| func TestSearchTracesHandler_Handle_WithErrorsFilter_UsingMemoryStore(t *testing.T) { | ||
| store, err := memory.NewStore(memory.Configuration{MaxTraces: 10}) | ||
| require.NoError(t, err) | ||
|
|
||
| require.NoError(t, store.WriteTraces(context.Background(), createTestTrace( | ||
| "trace111", | ||
| "test", | ||
| "/ok", | ||
| false, | ||
| ))) | ||
| require.NoError(t, store.WriteTraces(context.Background(), createTestTrace( | ||
| "trace222", | ||
| "test", | ||
| "/error", | ||
| true, | ||
| ))) | ||
|
|
||
| handler := &searchTracesHandler{ | ||
| queryService: querysvc.NewQueryService(store, store, querysvc.QueryServiceOptions{}), | ||
| maxResults: 100, | ||
| } | ||
|
|
||
| input := types.SearchTracesInput{ | ||
| StartTimeMin: "-1h", | ||
| ServiceName: "test", | ||
| WithErrors: true, | ||
| } | ||
|
|
||
| _, output, err := handler.handle(context.Background(), &mcp.CallToolRequest{}, input) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Len(t, output.Traces, 1) | ||
| assert.True(t, output.Traces[0].HasErrors) | ||
| assert.Equal(t, "test", output.Traces[0].RootService) | ||
| assert.Equal(t, "/error", output.Traces[0].RootSpanName) | ||
| assert.Empty(t, output.Error) | ||
| } |
There was a problem hiding this comment.
The new memory-store test validates returned results, but it does not actually assert that the handler encodes the error query attribute as a boolean. If the memory backend accepts both string and bool (as it now does), this test won’t catch regressions back to string encoding. Consider asserting the query attribute type/value (via a mock query service) or tightening the memory backend behavior for this handler path so the test enforces boolean semantics.
There was a problem hiding this comment.
Disagree. This test is intended to verify the end-to-end MCP -> QueryService -> memory behavior, not to enforce a boolean encoding in the handler.
The handler encoding is already covered by TestSearchTracesHandler_Handle_WithErrorsFilter, which explicitly checks that WithErrors is translated to the string query attribute error="true".
For this PR, string encoding is the intended behavior, and the memory backend change is specifically to make that existing query convention work correctly.
Signed-off-by: haibo <[email protected]>
|
@yurishkuro I’ve updated this PR to fix the issue in the memory backend instead of changing MCP. The memory backend now accepts both string and boolean forms of the |
YoungHypo
changed the title
Signed-off-by: haibo <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Which problem is this PR solving?
The existing query stack encodes tag filters as strings. In particular, MCP
search_tracestranslateswith_errors=trueinto the query attributeerror="true".The v2 memory backend handled the
errorquery attribute as bool-only during trace filtering, which made it inconsistent with the existing query convention and caused string-formerrorfilters to be misinterpreted.Description of the changes
search_tracesaligned with the existing string-based query attribute conventionerrorquery attributeerrorfilters in the memory backendwith_errorsflowHere is a Mermaid diagram to help explain why the current code had a bug from
TestSearchTracesHandler_Handle_WithErrorsFilter_UsingMemoryStore.sequenceDiagram participant T as Test participant H as handle() participant BQ as buildQuery() participant FT as FindTraces() participant M as memory.FindTraces() participant VS as validSpan() T->>H: handle(ctx, req, input) H->>BQ: buildQuery(input) Note over BQ: PutStr("error", "true") BQ-->>H: Attributes: {error: "true" (string)} H->>FT: FindTraces(ctx, query) FT->>M: FindTraces(ctx, query) M->>VS: validSpan(span, query) Note over VS: errAttribute.Bool() Note over VS: String.Bool() = false VS->>VS: !false && Error != Ok = true VS-->>M: return false M-->>FT: (no trace) FT-->>H: empty iterator H-->>T: output.Traces = []How was this change tested?
Checklist
make lint testAI Usage in this PR (choose one)
See AI Usage Policy.