diff --git a/CHANGELOG.md b/CHANGELOG.md index 45941046cc1..123ba4c96ae 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/modules/ingester/instance_search.go b/modules/ingester/instance_search.go index 2de7b25f4a6..f556ad5b61b 100644 --- a/modules/ingester/instance_search.go +++ b/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 diff --git a/modules/ingester/instance_search_test.go b/modules/ingester/instance_search_test.go index a3be2669e1e..98636601446 100644 --- a/modules/ingester/instance_search_test.go +++ b/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: "{ && 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) + }) + } +}