Fix autocomplete returning erroneous values (#3339)

* correct definition level Signed-off-by: Joe Elliott <number101010@gmail.com> * removed unused param Signed-off-by: Joe Elliott <number101010@gmail.com> * Add autocomplete test Signed-off-by: Joe Elliott <number101010@gmail.com> * span unscoped Signed-off-by: Joe Elliott <number101010@gmail.com> * changelog Signed-off-by: Joe Elliott <number101010@gmail.com> * added multiple conditions and commented out unscoped test Signed-off-by: Joe Elliott <number101010@gmail.com> * lint Signed-off-by: Joe Elliott <number101010@gmail.com> --------- Signed-off-by: Joe Elliott <number101010@gmail.com>
grafana · Jan 29, 2024 · 870b367 · 870b367
1 parent cc08db3
commit 870b367
Show file tree

Hide file tree

Showing 4 changed files with 132 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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