Remove status.code and error from /api/search/tags (#2085) (#2158)

* Remove status.code from /api/search/tags * Update integration test * Remove error from search tags also * Increase test timeout * Update integration test * Update integration test (cherry picked from commit a29054c)
grafana · Mar 1, 2023 · 2349115 · 2349115
1 parent a9d4d98
commit 2349115
Show file tree

Hide file tree

Showing 9 changed files with 14 additions and 30 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,5 @@
 ## main / unreleased
-
+* [CHANGE] No longer return `status.code` from /api/search/tags unless it is an attribute present in the data [#2059](https://github.com/grafana/tempo/issues/2059) (@mdisibio)
 * [BUGFIX] Suppress logspam in single binary mode when metrics generator is disabled. [#2058](https://github.com/grafana/tempo/pull/2058) (@joe-elliott)
 * [BUGFIX] Error more gracefully while reading some blocks written by an interim commit between 1.5 and 2.0 [#2055](https://github.com/grafana/tempo/pull/2055) (@mdisibio)
 * [BUGFIX] Correctly coalesce trace level data when combining Parquet traces. [#2095](https://github.com/grafana/tempo/pull/2095) (@joe-elliott)

diff --git a/Makefile b/Makefile
@@ -35,7 +35,7 @@ ifeq ($(BUILD_DEBUG), 1)
 	GO_OPT+= -gcflags="all=-N -l"
 endif
 
-GOTEST_OPT?= -race -timeout 20m -count=1
+GOTEST_OPT?= -race -timeout 30m -count=1
 GOTEST_OPT_WITH_COVERAGE = $(GOTEST_OPT) -cover
 GOTEST=go test
 LINT=golangci-lint

diff --git a/integration/e2e/e2e_test.go b/integration/e2e/e2e_test.go
@@ -119,7 +119,7 @@ func TestAllInOne(t *testing.T) {
 			// test metrics
 			require.NoError(t, tempo.WaitSumMetrics(e2e.Equals(1), "tempo_ingester_blocks_flushed_total"))
 			require.NoError(t, tempo.WaitSumMetricsWithOptions(e2e.Equals(1), []string{"tempodb_blocklist_length"}, e2e.WaitMissingMetrics))
-			require.NoError(t, tempo.WaitSumMetrics(e2e.Equals(5), "tempo_query_frontend_queries_total"))
+			require.NoError(t, tempo.WaitSumMetrics(e2e.Equals(4), "tempo_query_frontend_queries_total"))
 
 			// query trace - should fetch from backend
 			queryAndAssertTrace(t, apiClient, info)
@@ -272,7 +272,7 @@ func TestMicroservicesWithKVStores(t *testing.T) {
 				require.NoError(t, i.WaitSumMetrics(e2e.Equals(1), "tempo_ingester_blocks_flushed_total"))
 			}
 			require.NoError(t, tempoQuerier.WaitSumMetrics(e2e.Equals(3), "tempodb_blocklist_length"))
-			require.NoError(t, tempoQueryFrontend.WaitSumMetrics(e2e.Equals(4), "tempo_query_frontend_queries_total"))
+			require.NoError(t, tempoQueryFrontend.WaitSumMetrics(e2e.Equals(3), "tempo_query_frontend_queries_total"))
 
 			// query trace - should fetch from backend
 			queryAndAssertTrace(t, apiClient, info)

diff --git a/integration/util.go b/integration/util.go
@@ -223,10 +223,8 @@ func SearchAndAssertTrace(t *testing.T, client *tempoUtil.Client, info *tempoUti
 
 	attr := tempoUtil.RandomAttrFromTrace(expected)
 
-	// verify attribute is present in tags
-	tagsResp, err := client.SearchTags()
-	require.NoError(t, err)
-	require.Contains(t, tagsResp.TagNames, attr.Key)
+	// NOTE: SearchTags doesn't include live traces anymore
+	// so don't check SearchTags
 
 	// verify attribute value is present in tag values
 	tagValuesResp, err := client.SearchTagValues(attr.Key)

diff --git a/modules/ingester/instance_search.go b/modules/ingester/instance_search.go
@@ -336,19 +336,6 @@ func (i *instance) SearchTags(ctx context.Context) (*tempopb.SearchTagsResponse,
 	limit := i.limiter.limits.MaxBytesPerTagValuesQuery(userID)
 	distinctValues := util.NewDistinctStringCollector(limit)
 
-	// live traces
-	kv := &tempofb.KeyValues{}
-	err = i.visitSearchEntriesLiveTraces(ctx, func(entry *tempofb.SearchEntry) {
-		for i, ii := 0, entry.TagsLength(); i < ii; i++ {
-			entry.Tags(kv, i)
-			key := string(kv.Key())
-			distinctValues.Collect(key)
-		}
-	})
-	if err != nil {
-		return nil, err
-	}
-
 	// wal + search blocks
 	if !distinctValues.Exceeded() {
 		err = i.visitSearchableBlocks(ctx, func(block search.SearchableBlock) error {

diff --git a/modules/querier/querier.go b/modules/querier/querier.go
@@ -353,11 +353,6 @@ func (q *Querier) SearchTags(ctx context.Context, req *tempopb.SearchTagsRequest
 	limit := q.limits.MaxBytesPerTagValuesQuery(userID)
 	distinctValues := util.NewDistinctStringCollector(limit)
 
-	// Virtual tags. Get these first
-	for _, k := range search.GetVirtualTags() {
-		distinctValues.Collect(k)
-	}
-
 	// Get results from all ingesters
 	replicationSet, err := q.ring.GetReplicationSetForOperation(ring.Read)
 	if err != nil {

diff --git a/tempodb/encoding/vparquet/block_search_tags.go b/tempodb/encoding/vparquet/block_search_tags.go
@@ -71,6 +71,11 @@ func searchTags(_ context.Context, cb common.TagCallback, pf *parquet.File) erro
 	// find indexes of all special columns
 	specialAttrIdxs := map[int]string{}
 	for lbl, col := range labelMappings {
+		if lbl == LabelStatusCode {
+			// Don't include this in the list of tags (but it can still be used in SearchTagValues)
+			continue
+		}
+
 		idx, _ := pq.GetColumnIndexByPath(pf, col)
 		if idx == -1 {
 			continue

diff --git a/tempodb/encoding/vparquet/block_search_tags_test.go b/tempodb/encoding/vparquet/block_search_tags_test.go
@@ -31,6 +31,9 @@ func TestBackendBlockSearchTags(t *testing.T) {
 
 	// test that all attrs are in found attrs
 	for k := range attrs {
+		if k == LabelStatusCode {
+			continue
+		}
 		_, ok := foundAttrs[k]
 		require.True(t, ok)
 	}

diff --git a/tempodb/search/util.go b/tempodb/search/util.go
@@ -8,10 +8,6 @@ import (
 	"github.com/grafana/tempo/pkg/util"
 )
 
-func GetVirtualTags() []string {
-	return []string{trace.ErrorTag}
-}
-
 func GetVirtualTagValues(tagName string) []string {
 	switch tagName {