Skip to content

fix(memory): accept string-form error filters in trace search #8217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yurishkuro merged 9 commits into from
Mar 27, 2026
Merged

fix(memory): accept string-form error filters in trace search #8217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2da7a1d
fix(jaegermcp): use PutBool instead of PutStr for error attribute in …
YoungHypo Mar 21, 2026
61a51b0
fix(jaegermcp): fix test assertions for error bool attribute
YoungHypo Mar 21, 2026
271b35b
fix(memory): accept string-form error filters in trace search
YoungHypo Mar 23, 2026
0c0afb3
Merge branch 'main' into haibo-fix-error-type
YoungHypo Mar 23, 2026
4247cbd
fix(memory): change variable name by copilot suggestion
YoungHypo Mar 23, 2026
37edd99
test(memory): add TestFindTraces_ErrorAttributeInvalidType
YoungHypo Mar 23, 2026
223f28b
Merge branch 'main' into haibo-fix-error-type
YoungHypo Mar 23, 2026
4d2f672
Merge branch 'main' into haibo-fix-error-type
YoungHypo Mar 24, 2026
600299e
Merge branch 'main' into haibo-fix-error-type
YoungHypo Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions cmd/jaeger/internal/extension/jaegermcp/internal/handlers/search_traces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (

"github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegermcp/internal/types"
"github.com/jaegertracing/jaeger/cmd/jaeger/internal/extension/jaegerquery/querysvc"
"github.com/jaegertracing/jaeger/internal/storage/v2/memory"
)

// mockQueryService and createTestTrace are defined in test_helpers.go
Expand Down Expand Up @@ -160,9 +161,9 @@ func TestSearchTracesHandler_Handle_WithErrorsFilter(t *testing.T) {
mock := &mockQueryService{
findTracesFunc: func(_ context.Context, query querysvc.TraceQueryParams) iter.Seq2[[]ptrace.Traces, error] {
// 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())

Comment on lines 163 to 167
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@YoungHypo YoungHypo Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return func(yield func([]ptrace.Traces, error) bool) {
// Return only error traces (simulating storage filtering)
Expand All @@ -187,6 +188,44 @@ func TestSearchTracesHandler_Handle_WithErrorsFilter(t *testing.T) {
assert.True(t, output.Traces[0].HasErrors)
}

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)
Comment thread
yurishkuro marked this conversation as resolved.
}
Comment on lines +191 to +227
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@YoungHypo YoungHypo Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


func TestSearchTracesHandler_Handle_SearchDepthDefault(t *testing.T) {
testTrace := createTestTrace("trace444", "test", "/test", false)

Expand Down
97 changes: 97 additions & 0 deletions internal/storage/v2/memory/memory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,103 @@ func TestFindTraces_WrongQuery(t *testing.T) {
}
}

func TestFindTraces_ErrorAttributeStringCompatibility(t *testing.T) {
store, err := NewStore(Configuration{
MaxTraces: 10,
})
require.NoError(t, err)

td := ptrace.NewTraces()
rs := td.ResourceSpans().AppendEmpty()
spans := rs.ScopeSpans().AppendEmpty().Spans()
traceIDOK := fromString(t, "00000000000000010000000000000000")
traceIDError := fromString(t, "00000000000000020000000000000000")

spanOK := spans.AppendEmpty()
spanOK.SetTraceID(traceIDOK)
spanOK.Status().SetCode(ptrace.StatusCodeOk)

spanError := spans.AppendEmpty()
spanError.SetTraceID(traceIDError)
spanError.Status().SetCode(ptrace.StatusCodeError)

err = store.WriteTraces(context.Background(), td)
require.NoError(t, err)

tests := []struct {
name string
queryValue string
expectedTraceID pcommon.TraceID
}{
{
name: "error=true",
queryValue: "true",
expectedTraceID: traceIDError,
},
{
name: "error=false",
queryValue: "false",
expectedTraceID: traceIDOK,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
queryAttributes := pcommon.NewMap()
queryAttributes.PutStr(errorAttribute, tt.queryValue)
iter := store.FindTraces(context.Background(), tracestore.TraceQueryParams{
Attributes: queryAttributes,
SearchDepth: 10,
})

iterLength := 0
for traces, err := range iter {
require.NoError(t, err)
iterLength++
assert.Len(t, traces, 1)
assert.Equal(t, tt.expectedTraceID, traces[0].ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0).TraceID())
}
assert.Equal(t, 1, iterLength)
})
}
}

func TestFindTraces_ErrorAttributeInvalidType(t *testing.T) {
store, err := NewStore(Configuration{
MaxTraces: 10,
})
require.NoError(t, err)

td := ptrace.NewTraces()
rs := td.ResourceSpans().AppendEmpty()
spans := rs.ScopeSpans().AppendEmpty().Spans()

spanOK := spans.AppendEmpty()
spanOK.SetTraceID(fromString(t, "00000000000000010000000000000000"))
spanOK.Status().SetCode(ptrace.StatusCodeOk)

spanError := spans.AppendEmpty()
spanError.SetTraceID(fromString(t, "00000000000000020000000000000000"))
spanError.Status().SetCode(ptrace.StatusCodeError)

err = store.WriteTraces(context.Background(), td)
require.NoError(t, err)

queryAttributes := pcommon.NewMap()
queryAttributes.PutInt(errorAttribute, 1)
iter := store.FindTraces(context.Background(), tracestore.TraceQueryParams{
Attributes: queryAttributes,
SearchDepth: 10,
})

iterLength := 0
for _, err := range iter {
require.NoError(t, err)
iterLength++
}
assert.Equal(t, 0, iterLength)
}

func TestFindTracesAttributesMatching(t *testing.T) {
stringVal := "val"
tests := []struct {
Expand Down
24 changes: 22 additions & 2 deletions internal/storage/v2/memory/tenant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package memory

import (
"errors"
"strconv"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -257,10 +258,14 @@ func validSpan(resourceAttributes pcommon.Map, scope pcommon.InstrumentationScop
}

if errAttribute, ok := query.Attributes.Get(errorAttribute); ok {
if errAttribute.Bool() && span.Status().Code() != ptrace.StatusCodeError {
errorVal, valid := errorQueryValue(errAttribute)
if !valid {
return false
}
if !errAttribute.Bool() && span.Status().Code() != ptrace.StatusCodeOk {
if errorVal && span.Status().Code() != ptrace.StatusCodeError {
return false
Comment thread
yurishkuro marked this conversation as resolved.
}
if !errorVal && span.Status().Code() != ptrace.StatusCodeOk {
return false
}
}
Expand Down Expand Up @@ -347,6 +352,21 @@ func fromOTELSpanKind(kind ptrace.SpanKind) string {
return strings.ToLower(kind.String())
}

func errorQueryValue(attr pcommon.Value) (value bool, valid bool) {
switch attr.Type() {
case pcommon.ValueTypeBool:
return attr.Bool(), true
case pcommon.ValueTypeStr:
val, err := strconv.ParseBool(attr.Str())
if err != nil {
return false, false
}
return val, true
default:
return false, false
}
}

func spanStatusFromString(statusStr string) ptrace.StatusCode {
switch strings.ToUpper(statusStr) {
case "OK":
Expand Down
Loading