Skip to content

Commit a97f399

Browse files
ruslan-mikhailovruslan-mikhailov
authored
[bugfix] Fix search by trace:id for short IDs with leading zeros (#5587)
1 parent c3a7df8 commit a97f399

File tree

4 files changed

+119
-22
lines changed

4 files changed

+119
-22
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
* [BUGFIX] Correctly track and reject too large traces in live stores. [#5757](https://github.com/grafana/tempo/pull/5757) (@joe-elliott)
2525
* [BUGFIX] Fix issues related to integer dedicated columns in vParquet5-preview2 [#5716](https://github.com/grafana/tempo/pull/5716) (@stoewer)
2626
* [BUGFIX] Fix: handle collisions with job and instance labels when targetInfo is enabled [#5774](https://github.com/grafana/tempo/pull/5774) (@javiermolinar)
27+
* [BUGFIX] Fix search by trace:id for short IDs with leading zeros [#5587](https://github.com/grafana/tempo/pull/5587) (@ruslan-mikhailov)
2728

2829
# v2.9.0
2930

‎pkg/traceql/ast.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"hash/fnv"
88
"math"
99
"slices"
10+
"strings"
1011
"time"
1112
"unsafe"
1213

@@ -519,6 +520,17 @@ func newBinaryOperation(op Operator, lhs, rhs FieldExpression) FieldExpression {
519520
RHS: rhs,
520521
}
521522

523+
if attr, ok := lhs.(Attribute); ok && attr.Intrinsic == IntrinsicTraceID {
524+
if static, ok := rhs.(Static); ok {
525+
binop.RHS = normalizeTraceIDOperand(static)
526+
}
527+
}
528+
if attr, ok := rhs.(Attribute); ok && attr.Intrinsic == IntrinsicTraceID {
529+
if static, ok := lhs.(Static); ok {
530+
binop.LHS = normalizeTraceIDOperand(static)
531+
}
532+
}
533+
522534
// AST rewrite for simplification
523535
if !binop.referencesSpan() && binop.validate() == nil {
524536
if simplified, err := binop.execute(nil); err == nil {
@@ -533,6 +545,16 @@ func newBinaryOperation(op Operator, lhs, rhs FieldExpression) FieldExpression {
533545
return binop
534546
}
535547

548+
// normalizeTraceIDOperand normalizes a Static operand for trace ID
549+
func normalizeTraceIDOperand(operand Static) Static {
550+
if operand.Type != TypeString {
551+
return operand
552+
}
553+
traceID := operand.EncodeToString(false)
554+
traceID = strings.TrimLeft(traceID, "0")
555+
return NewStaticString(traceID)
556+
}
557+
536558
// nolint: revive
537559
func (BinaryOperation) __fieldExpression() {}
538560

‎pkg/traceql/ast_test.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1100,3 +1100,67 @@ func TestNeedsFullTrace(t *testing.T) {
11001100
})
11011101
}
11021102
}
1103+
1104+
func TestNewBinaryOperation_TraceIDNormalization(t *testing.T) {
1105+
traceIDAttr := NewIntrinsic(IntrinsicTraceID)
1106+
op := OpEqual
1107+
1108+
tests := []struct {
1109+
name string
1110+
expression FieldExpression
1111+
expected FieldExpression
1112+
}{
1113+
{
1114+
name: "with leading zeros",
1115+
expression: NewStaticString("0000123"),
1116+
expected: NewStaticString("123"),
1117+
},
1118+
{
1119+
name: "no leading zeros",
1120+
expression: NewStaticString("123456789abcdef123456789abcdef"),
1121+
expected: NewStaticString("123456789abcdef123456789abcdef"),
1122+
},
1123+
{
1124+
name: "int",
1125+
expression: NewStaticInt(123),
1126+
expected: NewStaticInt(123),
1127+
},
1128+
{
1129+
name: "float",
1130+
expression: NewStaticFloat(123.0),
1131+
expected: NewStaticFloat(123.0),
1132+
},
1133+
}
1134+
1135+
for _, tc := range []struct {
1136+
name string
1137+
reverse bool
1138+
}{
1139+
{name: "trace:id = value", reverse: false},
1140+
{name: "value = trace:id", reverse: true},
1141+
} {
1142+
for _, tt := range tests {
1143+
t.Run(tc.name+" "+tt.name, func(t *testing.T) {
1144+
var lhs FieldExpression = traceIDAttr
1145+
rhs := tt.expression
1146+
var expectedLHS FieldExpression = traceIDAttr
1147+
expectedRHS := tt.expected
1148+
1149+
if tc.reverse {
1150+
lhs = tt.expression
1151+
rhs = traceIDAttr
1152+
expectedLHS = tt.expected
1153+
expectedRHS = traceIDAttr
1154+
}
1155+
1156+
binop := newBinaryOperation(op, lhs, rhs)
1157+
1158+
// Verify the result is a BinaryOperation
1159+
require.IsType(t, &BinaryOperation{}, binop)
1160+
actualBinop := binop.(*BinaryOperation)
1161+
assert.Equal(t, expectedLHS, actualBinop.LHS)
1162+
assert.Equal(t, expectedRHS, actualBinop.RHS)
1163+
})
1164+
}
1165+
}
1166+
}

‎tempodb/tempodb_search_test.go‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2723,20 +2723,25 @@ func TestSearchByShortTraceID(t *testing.T) {
27232723
return block.Fetch(ctx, req, common.DefaultSearchOptions())
27242724
})
27252725

2726-
t.Run(fmt.Sprintf("%s: include trace id", v.Version()), func(t *testing.T) {
2727-
req := &tempopb.SearchRequest{Query: fmt.Sprintf(`{trace:id="%s"}`, wantMeta.TraceID)}
2728-
res, err := traceql.NewEngine().ExecuteSearch(ctx, req, fetcher, false)
2729-
require.NoError(t, err, "search request: %+v", req)
2730-
require.NotEmpty(t, res.Traces)
2731-
2732-
actual := actualForExpectedMeta(wantMeta, res)
2733-
require.NotNil(t, actual, "search request: %v", req)
2734-
2735-
actual.SpanSet = nil // todo: add the matching spansets to wantmeta
2736-
actual.SpanSets = nil
2737-
actual.ServiceStats = nil
2738-
require.Equal(t, wantMeta, actual, "search request: %v", req)
2739-
})
2726+
for name, traceID := range map[string]string{
2727+
"short trace id": wantMeta.TraceID,
2728+
"short trace id with leading zeros": strings.Repeat("0", 32-len(wantMeta.TraceID)) + wantMeta.TraceID,
2729+
} {
2730+
t.Run(fmt.Sprintf("%s: include trace id %s", v.Version(), name), func(t *testing.T) {
2731+
req := &tempopb.SearchRequest{Query: fmt.Sprintf(`{trace:id="%s"}`, traceID)}
2732+
res, err := traceql.NewEngine().ExecuteSearch(ctx, req, fetcher, false)
2733+
require.NoError(t, err, "search request: %+v", req)
2734+
require.NotEmpty(t, res.Traces)
2735+
2736+
actual := actualForExpectedMeta(wantMeta, res)
2737+
require.NotNil(t, actual, "search request: %v", req)
2738+
2739+
actual.SpanSet = nil // todo: add the matching spansets to wantmeta
2740+
actual.SpanSets = nil
2741+
actual.ServiceStats = nil
2742+
require.Equal(t, wantMeta, actual, "search request: %v", req)
2743+
})
2744+
}
27402745

27412746
t.Run(fmt.Sprintf("%s: include span id", v.Version()), func(t *testing.T) {
27422747
spanID := "0000000000010203"
@@ -2767,14 +2772,19 @@ func TestSearchByShortTraceID(t *testing.T) {
27672772
require.Equal(t, wantMeta, actual, "search request: %v", req)
27682773
})
27692774

2770-
t.Run(fmt.Sprintf("%s: exclude trace id", v.Version()), func(t *testing.T) {
2771-
req := &tempopb.SearchRequest{Query: fmt.Sprintf(`{trace:id!="%s"}`, wantMeta.TraceID)}
2772-
res, err := traceql.NewEngine().ExecuteSearch(ctx, req, fetcher, false)
2773-
require.NoError(t, err, "search request: %+v", req)
2774-
require.NotEmpty(t, res.Traces)
2775-
actual := actualForExpectedMeta(wantMeta, res)
2776-
require.Nil(t, actual, "search request: %v", req)
2777-
})
2775+
for name, traceID := range map[string]string{
2776+
"short trace id": wantMeta.TraceID,
2777+
"short trace id with leading zeros": strings.Repeat("0", 32-len(wantMeta.TraceID)) + wantMeta.TraceID,
2778+
} {
2779+
t.Run(fmt.Sprintf("%s: exclude trace id %s", v.Version(), name), func(t *testing.T) {
2780+
req := &tempopb.SearchRequest{Query: fmt.Sprintf(`{trace:id!="%s"}`, traceID)}
2781+
res, err := traceql.NewEngine().ExecuteSearch(ctx, req, fetcher, false)
2782+
require.NoError(t, err, "search request: %+v", req)
2783+
require.NotEmpty(t, res.Traces)
2784+
actual := actualForExpectedMeta(wantMeta, res)
2785+
require.Nil(t, actual, "search request: %v", req)
2786+
})
2787+
}
27782788

27792789
t.Run(fmt.Sprintf("%s: exclude span id", v.Version()), func(t *testing.T) {
27802790
spanID := "0000000000010203"

0 commit comments

Comments
 (0)