Handle invalid TraceQL query filter in tag values v2 disk cache (#4392)

* handle invalid TraceQL query filter in tag values v2 disk cache

* update CHANGELOG.md

CHANGELOG.md

@@ -69,6 +69,7 @@
 * [ENHANCEMENT] Reuse generator code to better refuse "too large" traces. [#4365](https://github.com/grafana/tempo/pull/4365) (@joe-elliott)
   This will cause the ingester to more aggressively and correctly refuse traces. Also added two metrics to better track bytes consumed per tenant in the ingester.
   `tempo_metrics_generator_live_trace_bytes` and `tempo_ingester_live_trace_bytes`.
+* [BUGFIX] Handle invalid TraceQL query filter in tag values v2 disk cache [#4392](https://github.com/grafana/tempo/pull/4392) (@electron0zero)
 * [BUGFIX] Replace hedged requests roundtrips total with a counter. [#4063](https://github.com/grafana/tempo/pull/4063) [#4078](https://github.com/grafana/tempo/pull/4078) (@galalen)
 * [BUGFIX] Metrics generators: Correctly drop from the ring before stopping ingestion to reduce drops during a rollout. [#4101](https://github.com/grafana/tempo/pull/4101) (@joe-elliott)
 * [BUGFIX] Correctly handle 400 Bad Request and 404 Not Found in gRPC streaming [#4144](https://github.com/grafana/tempo/pull/4144) (@mapno)

modules/ingester/instance_search.go

@@ -616,15 +616,20 @@ func includeBlock(b *backend.BlockMeta, req *tempopb.SearchRequest) bool {
 	return b.StartTime.Unix() <= end && b.EndTime.Unix() >= start
 }
 
+// searchTagValuesV2CacheKey generates a cache key for the searchTagValuesV2 request
+// cache key is used as the filename to store the protobuf data on disk
 func searchTagValuesV2CacheKey(req *tempopb.SearchTagValuesRequest, limit int, prefix string) string {
 	query := req.Query
 	if req.Query != "" {
 		ast, err := traceql.Parse(req.Query)
-		if err != nil { // this should never happen but in case it happens
-			return ""
+		if err == nil {
+			// forces the query into a canonical form
+			query = ast.String()
+		} else {
+			// In case of a bad TraceQL query, we ignore the query and return unfiltered results.
+			// if we fail to parse the query, we will assume query is empty and compute the cache key.
+			query = ""
 		}
-		// forces the query into a canonical form
-		query = ast.String()
 	}
 
 	// NOTE: we are not adding req.Start and req.End to the cache key because we don't respect the start and end

modules/ingester/instance_search_test.go

@@ -312,10 +312,13 @@ func TestInstanceSearchTagAndValuesV2(t *testing.T) {
 		otherTagValue         = qux
 		queryThatMatches      = fmt.Sprintf(`{ name = "%s" }`, spanName)
 		queryThatDoesNotMatch = `{ resource.service.name = "aaaaa" }`
+		emptyQuery            = `{ }`
+		invalidQuery          = `{ not_a_traceql = query }`
+		partInvalidQuery      = fmt.Sprintf(`{ name = "%s" && not_a_traceql = query  }`, spanName)
 	)
 
 	_, expectedTagValues, expectedEventTagValues, expectedLinkTagValues := writeTracesForSearch(t, i, spanName, tagKey, tagValue, true, true)
-	_, _, _, _ = writeTracesForSearch(t, i, "other-"+spanName, tagKey, otherTagValue, true, false)
+	_, otherTagValues, otherEventTagValues, otherLinkTagValues := writeTracesForSearch(t, i, "other-"+spanName, tagKey, otherTagValue, true, true)
 
 	userCtx := user.InjectOrgID(context.Background(), "fake")
 
@@ -351,6 +354,16 @@ func TestInstanceSearchTagAndValuesV2(t *testing.T) {
 
 	// test search is returning same results with cache
 	testSearchTagsAndValuesV2(t, userCtx, i, tagKey, queryThatMatches, expectedTagValues, expectedEventTagValues, expectedLinkTagValues)
+
+	// merge all tag values to test unfiltered query
+	expectedTagValues = append(expectedTagValues, otherTagValues...)
+	expectedEventTagValues = append(expectedEventTagValues, otherEventTagValues...)
+	expectedLinkTagValues = append(expectedLinkTagValues, otherLinkTagValues...)
+
+	// test un-filtered query and check that bad/invalid TraceQL query returns all tag values and is same as unfiltered query
+	testSearchTagsAndValuesV2(t, userCtx, i, tagKey, emptyQuery, expectedTagValues, expectedEventTagValues, expectedLinkTagValues)
+	testSearchTagsAndValuesV2(t, userCtx, i, tagKey, invalidQuery, expectedTagValues, expectedEventTagValues, expectedLinkTagValues)
+	testSearchTagsAndValuesV2(t, userCtx, i, tagKey, partInvalidQuery, expectedTagValues, expectedEventTagValues, expectedLinkTagValues)
 }
 
 // nolint:revive,unparam
@@ -1006,3 +1019,64 @@ func TestIncludeBlock(t *testing.T) {
 		})
 	}
 }
+
+func Test_searchTagValuesV2CacheKey(t *testing.T) {
+	tests := []struct {
+		name             string
+		req              *tempopb.SearchTagValuesRequest
+		limit            int
+		prefix           string
+		expectedCacheKey string
+	}{
+		{
+			name:             "prefix empty",
+			req:              &tempopb.SearchTagValuesRequest{TagName: "span.foo", Query: "{}"},
+			limit:            100,
+			prefix:           "",
+			expectedCacheKey: "_10963035328899851375.buf",
+		},
+		{
+			name:   "prefix not empty but same query",
+			req:    &tempopb.SearchTagValuesRequest{TagName: "span.foo", Query: "{}"},
+			limit:  100,
+			prefix: "my_amazing_prefix",
+			// hash should be same, only prefix should change
+			expectedCacheKey: "my_amazing_prefix_10963035328899851375.buf",
+		},
+		{
+			name:             "changing limit changes the cache key for same query",
+			req:              &tempopb.SearchTagValuesRequest{TagName: "span.foo", Query: "{}"},
+			limit:            500,
+			prefix:           "my_amazing_prefix",
+			expectedCacheKey: "my_amazing_prefix_10962052365504419966.buf",
+		},
+		{
+			name:             "different query generates different cache key",
+			req:              &tempopb.SearchTagValuesRequest{TagName: "span.foo", Query: "{ name = \"foo\" }"},
+			limit:            500,
+			prefix:           "my_amazing_prefix",
+			expectedCacheKey: "my_amazing_prefix_9241051696576633442.buf",
+		},
+		{
+			name:             "invalid query generates a valid cache key",
+			req:              &tempopb.SearchTagValuesRequest{TagName: "span.foo", Query: "{span.env=dev}"},
+			limit:            500,
+			prefix:           "my_amazing_prefix",
+			expectedCacheKey: "my_amazing_prefix_7849238702443650194.buf",
+		},
+		{
+			name:             "different invalid query generates the same valid cache key",
+			req:              &tempopb.SearchTagValuesRequest{TagName: "span.foo", Query: "{ <not valid traceql> && span.foo = \"bar\" }"},
+			limit:            500,
+			prefix:           "my_amazing_prefix",
+			expectedCacheKey: "my_amazing_prefix_7849238702443650194.buf",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cacheKey := searchTagValuesV2CacheKey(tt.req, tt.limit, tt.prefix)
+			require.Equal(t, tt.expectedCacheKey, cacheKey)
+		})
+	}
+}