Search: parse tags from a tags query parameter

CHANGELOG.md

@@ -11,6 +11,7 @@
   `/services` moved to `/status/services`
 * [CHANGE] **BREAKING CHANGE** Change ingester metric `ingester_bytes_metric_total` in favor of `ingester_bytes_received_total` [#979](https://github.com/grafana/tempo/pull/979) (@mapno)
 * [CHANGE] Add troubleshooting language to config for `server.grpc_server_max_recv_msg_size` and `server.grpc_server_max_send_msg_size` when handling large traces [#1023](https://github.com/grafana/tempo/pull/1023) (@thejosephstevens)
+* [CHANGE] Parse search query tags from `tags` query parameter [#1055](https://github.com/grafana/tempo/pull/1055) (@kvrhdn)
 * [FEATURE] Add ability to search ingesters for traces [#806](https://github.com/grafana/tempo/pull/806) (@mdisibio)
 * [FEATURE] Add runtime config handler  [#936](https://github.com/grafana/tempo/pull/936) (@mapno)
 * [FEATURE] Search WAL reload and compression(versioned encoding) support [#1000](https://github.com/grafana/tempo/pull/1000) (@annanay25, @mdisibio)

go.mod

@@ -16,6 +16,7 @@ require (
 	github.com/dustin/go-humanize v1.0.0
 	github.com/go-kit/kit v0.11.0
 	github.com/go-kit/log v0.1.0
+	github.com/go-logfmt/logfmt v0.5.0
 	github.com/go-test/deep v1.0.7
 	github.com/gogo/protobuf v1.3.2
 	github.com/gogo/status v1.1.0
@@ -124,7 +125,6 @@ require (
 	github.com/form3tech-oss/jwt-go v3.2.3+incompatible // indirect
 	github.com/fsnotify/fsnotify v1.4.9 // indirect
 	github.com/fsouza/fake-gcs-server v1.7.0 // indirect
-	github.com/go-logfmt/logfmt v0.5.0 // indirect
 	github.com/go-openapi/analysis v0.20.0 // indirect
 	github.com/go-openapi/errors v0.20.0 // indirect
 	github.com/go-openapi/jsonpointer v0.19.5 // indirect

modules/querier/querier_search.go

@@ -5,13 +5,17 @@ import (
 	"fmt"
 	"net/http"
 	"strconv"
+	"strings"
 	"time"
 
+	"github.com/go-logfmt/logfmt"
+
 	"github.com/grafana/tempo/pkg/api"
 	"github.com/grafana/tempo/pkg/tempopb"
 )
 
 const (
+	urlParamTags        = "tags"
 	urlParamMinDuration = "minDuration"
 	urlParamMaxDuration = "maxDuration"
 )
@@ -22,9 +26,12 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e
 		Limit: q.cfg.SearchDefaultResultLimit,
 	}
 
+	// Passing tags as individual query parameters is not supported anymore, clients should use the tags
+	// query parameter instead. We still parse these tags since the initial Grafana implementation uses this.
+	// As Grafana gets updated and/or versions using this get old we can remove this section.
 	for k, v := range r.URL.Query() {
 		// Skip reserved keywords
-		if k == urlParamMinDuration || k == urlParamMaxDuration || k == api.URLParamLimit {
+		if k == urlParamTags || k == urlParamMinDuration || k == urlParamMaxDuration || k == api.URLParamLimit {
 			continue
 		}
 
@@ -33,6 +40,27 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e
 		}
 	}
 
+	if encodedTags, ok := extractQueryParam(r, urlParamTags); ok {
+		decoder := logfmt.NewDecoder(strings.NewReader(encodedTags))
+
+		for decoder.ScanRecord() {
+			for decoder.ScanKeyval() {
+				key := string(decoder.Key())
+				if _, ok := req.Tags[key]; ok {
+					return nil, fmt.Errorf("invalid tags: tag %s has been set twice", key)
+				}
+				req.Tags[key] = string(decoder.Value())
+			}
+		}
+
+		if err := decoder.Err(); err != nil {
+			if syntaxErr, ok := err.(*logfmt.SyntaxError); ok {
+				return nil, fmt.Errorf("invalid tags: %s at pos %d", syntaxErr.Msg, syntaxErr.Pos)
+			}
+			return nil, fmt.Errorf("invalid tags: %w", err)
+		}
+	}
+
 	if s, ok := extractQueryParam(r, urlParamMinDuration); ok {
 		dur, err := time.ParseDuration(s)
 		if err != nil {

modules/querier/querier_search_test.go

@@ -1,7 +1,9 @@
 package querier
 
 import (
+	"fmt"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 
 	"github.com/grafana/tempo/pkg/tempopb"
@@ -23,45 +25,45 @@ func TestQuerierParseSearchRequest(t *testing.T) {
 		expected *tempopb.SearchRequest
 	}{
 		{
-			name: "Empty url query",
+			name: "empty query",
 			expected: &tempopb.SearchRequest{
 				Tags:  map[string]string{},
 				Limit: q.cfg.SearchDefaultResultLimit,
 			},
 		},
 		{
-			name:     "With limit set",
+			name:     "limit set",
 			urlQuery: "limit=10",
 			expected: &tempopb.SearchRequest{
 				Tags:  map[string]string{},
 				Limit: 10,
 			},
 		},
 		{
-			name:     "With limit exceeding max",
+			name:     "limit exceeding max",
 			urlQuery: "limit=120",
 			expected: &tempopb.SearchRequest{
 				Tags:  map[string]string{},
 				Limit: q.cfg.SearchMaxResultLimit,
 			},
 		},
 		{
-			name:     "With zero limit",
+			name:     "zero limit",
 			urlQuery: "limit=0",
 			err:      "invalid limit: must be a positive number",
 		},
 		{
-			name:     "With negative limit",
+			name:     "negative limit",
 			urlQuery: "limit=-5",
 			err:      "invalid limit: must be a positive number",
 		},
 		{
-			name:     "With non-numeric limit",
+			name:     "non-numeric limit",
 			urlQuery: "limit=five",
 			err:      "invalid limit: strconv.Atoi: parsing \"five\": invalid syntax",
 		},
 		{
-			name:     "With minDuration and maxDuration",
+			name:     "minDuration and maxDuration",
 			urlQuery: "minDuration=10s&maxDuration=20s",
 			expected: &tempopb.SearchRequest{
 				Tags:          map[string]string{},
@@ -71,36 +73,48 @@ func TestQuerierParseSearchRequest(t *testing.T) {
 			},
 		},
 		{
-			name:     "With minDuration greater than maxDuration",
+			name:     "minDuration greater than maxDuration",
 			urlQuery: "minDuration=20s&maxDuration=5s",
 			err:      "invalid maxDuration: must be greater than minDuration",
 		},
 		{
-			name:     "With invalid minDuration",
+			name:     "invalid minDuration",
 			urlQuery: "minDuration=10seconds",
 			err:      "invalid minDuration: time: unknown unit \"seconds\" in duration \"10seconds\"",
 		},
 		{
-			name:     "With invalid maxDuration",
+			name:     "invalid maxDuration",
 			urlQuery: "maxDuration=1msec",
 			err:      "invalid maxDuration: time: unknown unit \"msec\" in duration \"1msec\"",
 		},
 		{
-			name:     "With tags and limit",
-			urlQuery: "service.name=foo.bar&limit=5&query=1%2B1%3D2",
+			name:     "tags and limit",
+			urlQuery: "service.name=foo&tags=limit%3Dfive&limit=5&query=1%2B1%3D2",
 			expected: &tempopb.SearchRequest{
 				Tags: map[string]string{
-					"service.name": "foo.bar",
+					"service.name": "foo",
+					"limit":        "five",
 					"query":        "1+1=2",
 				},
 				Limit: 5,
 			},
 		},
+		{
+			name:     "tags query parameter with duplicate tag",
+			urlQuery: "tags=service.name%3Dfoo%20service.name%3Dbar",
+			err:      "invalid tags: tag service.name has been set twice",
+		},
+		{
+			name:     "top-level tags with conflicting query parameter tags",
+			urlQuery: "service.name=bar&tags=service.name%3Dfoo",
+			err:      "invalid tags: tag service.name has been set twice",
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			r := httptest.NewRequest("GET", "http://tempo/api/search?"+tt.urlQuery, nil)
+			fmt.Println("RequestURI:", r.RequestURI)
 
 			searchRequest, err := q.parseSearchRequest(r)
 
@@ -113,3 +127,55 @@ func TestQuerierParseSearchRequest(t *testing.T) {
 		})
 	}
 }
+
+func TestQuerierParseSearchRequestTags(t *testing.T) {
+	type strMap map[string]string
+
+	tests := []struct {
+		tags     string
+		expected map[string]string
+	}{
+		{"service.name=foo http.url=api/search", strMap{"service.name": "foo", "http.url": "api/search"}},
+		{"service%n@me=foo", strMap{"service%n@me": "foo"}},
+		{"service.name=foo error", strMap{"service.name": "foo", "error": ""}},
+		{"service.name=\"foo bar\"", strMap{"service.name": "foo bar"}},
+		{"service.name=\"foo=bar\"", strMap{"service.name": "foo=bar"}},
+		{"service.name=\"foo\\bar\"", strMap{"service.name": "foo\bar"}},
+		{"service.name=\"foo \\\"bar\\\"\"", strMap{"service.name": "foo \"bar\""}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.tags, func(t *testing.T) {
+			r := httptest.NewRequest("GET", "http://tempo/api/search?tags="+url.QueryEscape(tt.tags), nil)
+			fmt.Println("RequestURI:", r.RequestURI)
+
+			searchRequest, err := (&Querier{}).parseSearchRequest(r)
+
+			assert.NoError(t, err)
+			assert.Equal(t, tt.expected, searchRequest.Tags)
+		})
+	}
+}
+
+func TestQuerierParseSearchRequestTagsError(t *testing.T) {
+	tests := []struct {
+		tags string
+		err  string
+	}{
+		{"service.name=foo =error", "invalid tags: unexpected '=' at pos 18"},
+		{"service.name=foo=bar", "invalid tags: unexpected '=' at pos 17"},
+		{"service.name=\"foo bar", "invalid tags: unterminated quoted value at pos 22"},
+		{"\"service name\"=foo", "invalid tags: unexpected '\"' at pos 1"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.tags, func(t *testing.T) {
+			r := httptest.NewRequest("GET", "http://tempo/api/search?tags="+url.QueryEscape(tt.tags), nil)
+			fmt.Println("RequestURI:", r.RequestURI)
+
+			_, err := (&Querier{}).parseSearchRequest(r)
+
+			assert.EqualError(t, err, tt.err)
+		})
+	}
+}