diff --git a/CHANGELOG.md b/CHANGELOG.md index eb22f876c52..0cd752082b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ * [BUGFIX] Fix panic in autocomplete when query condition had wrong type [#3277](https://github.com/grafana/tempo/pull/3277) (@mapno) * [BUGFIX] Fix TLS when GRPC is enabled on HTTP [#3300](https://github.com/grafana/tempo/pull/3300) (@joe-elliott) * [BUGFIX] Correctly return 400 when max limit is requested on search. [#3340](https://github.com/grafana/tempo/pull/3340) (@joe-elliott) +* [BUGFIX] Fix autocomplete filters sometimes returning erroneous results. [#3339](https://github.com/grafana/tempo/pull/3339) (@joe-elliott) ## v2.3.1 / 2023-11-28 diff --git a/tempodb/encoding/vparquet3/block_autocomplete.go b/tempodb/encoding/vparquet3/block_autocomplete.go index 2cc48e8ed66..1f010633636 100644 --- a/tempodb/encoding/vparquet3/block_autocomplete.go +++ b/tempodb/encoding/vparquet3/block_autocomplete.go @@ -25,7 +25,8 @@ func (b *backendBlock) FetchTagValues(ctx context.Context, req traceql.Autocompl return err } - if len(req.Conditions) <= 1 || mingledConditions { // Last check. No conditions, use old path. It's much faster. + // Last check. No conditions, use old path. It's much faster. + if len(req.Conditions) <= 1 || mingledConditions { // <= 1 because we always have a "OpNone" condition for the tag name return b.SearchTagValuesV2(ctx, req.TagName, common.TagCallbackV2(cb), common.DefaultSearchOptions()) } @@ -64,7 +65,7 @@ func (b *backendBlock) FetchTagValues(ctx context.Context, req traceql.Autocompl // autocompleteIter creates an iterator that will collect values for a given attribute/tag. func autocompleteIter(ctx context.Context, req traceql.AutocompleteRequest, pf *parquet.File, opts common.SearchOptions, dc backend.DedicatedColumns) (parquetquery.Iterator, error) { - iter, err := createDistinctIterator(ctx, nil, req.Conditions, req.TagName, pf, opts, dc) + iter, err := createDistinctIterator(ctx, req.Conditions, req.TagName, pf, opts, dc) if err != nil { return nil, fmt.Errorf("error creating iterator: %w", err) } @@ -74,7 +75,6 @@ func autocompleteIter(ctx context.Context, req traceql.AutocompleteRequest, pf * func createDistinctIterator( ctx context.Context, - primaryIter parquetquery.Iterator, conds []traceql.Condition, tag traceql.Attribute, pf *parquet.File, @@ -96,7 +96,7 @@ func createDistinctIterator( var currentIter parquetquery.Iterator if len(spanConditions) > 0 { - currentIter, err = createDistinctSpanIterator(makeIter, keep, tag, primaryIter, spanConditions, dc) + currentIter, err = createDistinctSpanIterator(makeIter, keep, tag, currentIter, spanConditions, dc) if err != nil { return nil, errors.Wrap(err, "creating span iterator") } @@ -371,16 +371,16 @@ func createDistinctAttributeIterator( var valueIters []parquetquery.Iterator if len(attrStringPreds) > 0 { - valueIters = append(valueIters, makeIter(strPath, parquetquery.NewOrPredicate(attrStringPreds...), "string")) + valueIters = append(valueIters, makeIter(strPath, orIfNeeded(attrStringPreds), "string")) } if len(attrIntPreds) > 0 { - valueIters = append(valueIters, makeIter(intPath, parquetquery.NewOrPredicate(attrIntPreds...), "int")) + valueIters = append(valueIters, makeIter(intPath, orIfNeeded(attrIntPreds), "int")) } if len(attrFltPreds) > 0 { - valueIters = append(valueIters, makeIter(floatPath, parquetquery.NewOrPredicate(attrFltPreds...), "float")) + valueIters = append(valueIters, makeIter(floatPath, orIfNeeded(attrFltPreds), "float")) } if len(boolPreds) > 0 { - valueIters = append(valueIters, makeIter(boolPath, parquetquery.NewOrPredicate(boolPreds...), "bool")) + valueIters = append(valueIters, makeIter(boolPath, orIfNeeded(boolPreds), "bool")) } if len(valueIters) > 0 || len(iters) > 0 { @@ -500,7 +500,7 @@ func createDistinctResourceIterator( iters = append(iters, makeIter(columnPath, orIfNeeded(predicates), columnSelectAs[columnPath])) } - attrIter, err := createDistinctAttributeIterator(makeIter, keep, tag, genericConditions, DefinitionLevelResourceSpans, + attrIter, err := createDistinctAttributeIterator(makeIter, keep, tag, genericConditions, DefinitionLevelResourceAttrs, columnPathResourceAttrKey, columnPathResourceAttrString, columnPathResourceAttrInt, columnPathResourceAttrDouble, columnPathResourceAttrBool) if err != nil { return nil, errors.Wrap(err, "creating span attribute iterator") diff --git a/tempodb/encoding/vparquet3/block_traceql_test.go b/tempodb/encoding/vparquet3/block_traceql_test.go index 2fc10df5591..6cffd39d81b 100644 --- a/tempodb/encoding/vparquet3/block_traceql_test.go +++ b/tempodb/encoding/vparquet3/block_traceql_test.go @@ -27,7 +27,7 @@ func TestOne(t *testing.T) { wantTr := fullyPopulatedTestTrace(nil) b := makeBackendBlockWithTraces(t, []*Trace{wantTr}) ctx := context.Background() - q := `{ traceDuration > 1s }` + q := `{ resource.region != nil && resource.service.name = "bar" }` req := traceql.MustExtractFetchSpansRequestWithMetadata(q) req.StartTimeUnixNanos = uint64(1000 * time.Second) diff --git a/tempodb/tempodb_search_test.go b/tempodb/tempodb_search_test.go index 36ccbe29327..881d79e3ab3 100644 --- a/tempodb/tempodb_search_test.go +++ b/tempodb/tempodb_search_test.go @@ -6,6 +6,7 @@ import ( "fmt" "math/rand" "path" + "sort" "strings" "testing" "time" @@ -33,7 +34,7 @@ import ( "github.com/grafana/tempo/tempodb/wal" ) -type runnerFn func(*testing.T, *tempopb.Trace, *tempopb.TraceSearchMetadata, []*tempopb.SearchRequest, []*tempopb.SearchRequest, *backend.BlockMeta, Reader) +type runnerFn func(*testing.T, *tempopb.Trace, *tempopb.TraceSearchMetadata, []*tempopb.SearchRequest, []*tempopb.SearchRequest, *backend.BlockMeta, Reader, common.BackendBlock) const attributeWithTerminalChars = `{ } ( ) = ~ ! < > & | ^` @@ -48,12 +49,13 @@ func TestSearchCompleteBlock(t *testing.T) { groupTraceQLRunner, traceQLStructural, traceQLExistence, + autoComplete, ) }) } } -func searchRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, searchesThatMatch, searchesThatDontMatch []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader) { +func searchRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, searchesThatMatch, searchesThatDontMatch []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader, _ common.BackendBlock) { ctx := context.Background() for _, req := range searchesThatMatch { @@ -72,7 +74,7 @@ func searchRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearchM } } -func traceQLRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, searchesThatMatch, searchesThatDontMatch []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader) { +func traceQLRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, searchesThatMatch, searchesThatDontMatch []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader, _ common.BackendBlock) { ctx := context.Background() e := traceql.NewEngine() @@ -120,7 +122,7 @@ func traceQLRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearch } } -func advancedTraceQLRunner(t *testing.T, wantTr *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader) { +func advancedTraceQLRunner(t *testing.T, wantTr *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader, _ common.BackendBlock) { ctx := context.Background() e := traceql.NewEngine() @@ -283,7 +285,7 @@ func advancedTraceQLRunner(t *testing.T, wantTr *tempopb.Trace, wantMeta *tempop } } -func groupTraceQLRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader) { +func groupTraceQLRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader, _ common.BackendBlock) { ctx := context.Background() e := traceql.NewEngine() @@ -479,7 +481,7 @@ func groupTraceQLRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceS } } -func traceQLStructural(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader) { +func traceQLStructural(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader, _ common.BackendBlock) { ctx := context.Background() e := traceql.NewEngine() @@ -820,7 +822,7 @@ func traceQLStructural(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSe } // existence -func traceQLExistence(t *testing.T, _ *tempopb.Trace, _ *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader) { +func traceQLExistence(t *testing.T, _ *tempopb.Trace, _ *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader, _ common.BackendBlock) { ctx := context.Background() e := traceql.NewEngine() const intrinsicName = "name" @@ -913,6 +915,117 @@ func traceQLExistence(t *testing.T, _ *tempopb.Trace, _ *tempopb.TraceSearchMeta } } +// autoComplete! +func autoComplete(t *testing.T, _ *tempopb.Trace, _ *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, _ *backend.BlockMeta, _ Reader, bb common.BackendBlock) { + ctx := context.Background() + e := traceql.NewEngine() + + tcs := []struct { + name string + tag traceql.Attribute + query string + expected []tempopb.TagValue + }{ + { + name: "no matches", + tag: traceql.NewAttribute("resource.service.name"), + query: "{ span.foo = `bar` }", + expected: []tempopb.TagValue{}, + }, + { + name: "no filtering all service.names", + tag: traceql.NewScopedAttribute(traceql.AttributeScopeResource, false, "service.name"), + query: "{}", + expected: []tempopb.TagValue{ + {Type: "string", Value: "RootService"}, + {Type: "string", Value: "Service3"}, + {Type: "string", Value: "BrokenService"}, + {Type: "string", Value: "MyService"}, + {Type: "string", Value: "test-service"}, + }, + }, + { + name: "resource filtered by resource", + tag: traceql.NewScopedAttribute(traceql.AttributeScopeResource, false, "service.name"), + query: "{ resource.cluster = `MyCluster` }", + expected: []tempopb.TagValue{ + {Type: "string", Value: "MyService"}, + }, + }, + { + name: "span filtered by resource", + tag: traceql.NewScopedAttribute(traceql.AttributeScopeSpan, false, "span-dedicated.01"), + query: "{ resource.cluster = `MyCluster` }", + expected: []tempopb.TagValue{ + {Type: "string", Value: "span-1a"}, + }, + }, + { + name: "span filtered by span", + tag: traceql.NewScopedAttribute(traceql.AttributeScopeSpan, false, "span-dedicated.01"), + query: "{ span.http.url = `url/Hello/World` }", + expected: []tempopb.TagValue{ + {Type: "string", Value: "span-1a"}, + }, + }, + { + name: "resource filtered by span", + tag: traceql.NewScopedAttribute(traceql.AttributeScopeResource, false, "service.name"), + query: "{ span.foo = `Bar` }", + expected: []tempopb.TagValue{ + {Type: "string", Value: "MyService"}, + {Type: "string", Value: "RootService"}, + }, + }, + { + name: "multiple conditions", + tag: traceql.NewScopedAttribute(traceql.AttributeScopeResource, false, "res-dedicated.01"), + query: "{ resource.res-dedicated.02 = `res-2a` && span.http.status_code = 500 }", + expected: []tempopb.TagValue{ + {Type: "string", Value: "res-1a"}, + }, + }, + { + name: "unscoped not supported", // todo: add support for unscoped. currently it falls back to old logic and returns everything + tag: traceql.NewScopedAttribute(traceql.AttributeScopeResource, false, "service.name"), + query: "{ .foo = `Bar` }", + expected: []tempopb.TagValue{ + {Type: "string", Value: "RootService"}, + {Type: "string", Value: "Service3"}, + {Type: "string", Value: "BrokenService"}, + {Type: "string", Value: "MyService"}, + {Type: "string", Value: "test-service"}, + }, + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + fetcher := traceql.NewAutocompleteFetcherWrapper(func(ctx context.Context, req traceql.AutocompleteRequest, cb traceql.AutocompleteCallback) error { + return bb.FetchTagValues(ctx, req, cb, common.DefaultSearchOptions()) + }) + + valueCollector := util.NewDistinctValueCollector[tempopb.TagValue](0, func(v tempopb.TagValue) int { return 0 }) + err := e.ExecuteTagValues(ctx, tc.tag, tc.query, traceql.MakeCollectTagValueFunc(valueCollector.Collect), fetcher) + if errors.Is(err, common.ErrUnsupported) { + return + } + require.NoError(t, err, "autocomplete request: %+v", tc) + + expected := tc.expected + sort.Slice(expected, func(i, j int) bool { + return expected[i].Value < expected[j].Value + }) + actual := valueCollector.Values() + sort.Slice(actual, func(i, j int) bool { + return actual[i].Value < actual[j].Value + }) + + require.Equal(t, expected, actual) + }) + } +} + // oneQueryRunner is a good place to place a single query for debugging // func oneQueryRunner(t *testing.T, wantTr *tempopb.Trace, wantMeta *tempopb.TraceSearchMetadata, _, _ []*tempopb.SearchRequest, meta *backend.BlockMeta, r Reader) { // ctx := context.Background() @@ -1071,7 +1184,7 @@ func runCompleteBlockSearchTest(t *testing.T, blockVersion string, runners ...ru meta := block.BlockMeta() for _, r := range runners { - r(t, wantTr, wantMeta, searchesThatMatch, searchesThatDontMatch, meta, rw) + r(t, wantTr, wantMeta, searchesThatMatch, searchesThatDontMatch, meta, rw, block) } // todo: do some compaction and then call runner again