From 2f579efd0e9253ae71c7032be2d132ad5ce61454 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Tue, 19 Oct 2021 11:47:43 +0200 Subject: [PATCH 01/13] Add initial support for parsing tags query parameter --- modules/querier/querier_search.go | 16 ++++++++- modules/querier/querier_search_test.go | 45 ++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/modules/querier/querier_search.go b/modules/querier/querier_search.go index 887eac809d7..e3311f13a59 100644 --- a/modules/querier/querier_search.go +++ b/modules/querier/querier_search.go @@ -5,12 +5,14 @@ import ( "fmt" "net/http" "strconv" + "strings" "time" "github.com/grafana/tempo/pkg/tempopb" ) const ( + urlParamTags = "tags" urlParamMinDuration = "minDuration" urlParamMaxDuration = "maxDuration" URLParamLimit = "limit" @@ -26,7 +28,7 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e for k, v := range r.URL.Query() { // Skip reserved keywords - if k == urlParamMinDuration || k == urlParamMaxDuration || k == URLParamLimit { + if k == urlParamTags || k == urlParamMinDuration || k == urlParamMaxDuration || k == URLParamLimit { continue } @@ -35,6 +37,18 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e } } + if encodedTags, ok := extractQueryParam(r, urlParamTags); ok { + // TODO handle tag values wrapped in quotes + tags := strings.Split(encodedTags, " ") + + for _, tag := range tags { + splitTags := strings.SplitN(tag, "=", 2) + if len(splitTags) == 2 { + req.Tags[splitTags[0]] = splitTags[1] + } + } + } + if s, ok := extractQueryParam(r, urlParamMinDuration); ok { dur, err := time.ParseDuration(s) if err != nil { diff --git a/modules/querier/querier_search_test.go b/modules/querier/querier_search_test.go index c7268abd8c4..40345101dd6 100644 --- a/modules/querier/querier_search_test.go +++ b/modules/querier/querier_search_test.go @@ -96,6 +96,51 @@ func TestQuerierParseSearchRequest(t *testing.T) { Limit: 5, }, }, + { + name: "Tags query parameter", + urlQuery: "tags=service.name%3Dfoo%20http.url%3Dsearch", + expected: &tempopb.SearchRequest{ + Tags: map[string]string{ + "service.name": "foo", + "http.url": "search", + }, + Limit: q.cfg.SearchDefaultResultLimit, + }, + }, + { + name: "Tags query parameter with top-level tags", + urlQuery: "service.name=bar&tags=service.name%3Dfoo%20http.url%3Dsearch&test=bar", + expected: &tempopb.SearchRequest{ + Tags: map[string]string{ + "service.name": "foo", + "http.url": "search", + "test": "bar", + }, + Limit: q.cfg.SearchDefaultResultLimit, + }, + }, + { + name: "Tags query parameter with space in value", + urlQuery: "tags=service.name%3D%22my%20service%22%20http.url%3Dsearch", + expected: &tempopb.SearchRequest{ + Tags: map[string]string{ + "service.name": "my service", + "http.url": "search", + }, + Limit: q.cfg.SearchDefaultResultLimit, + }, + }, + { + name: "Tags query parameter with space in tag name", + urlQuery: "tags=%22service%20name%22%3Dfoo%20http.url%3Dsearch", + expected: &tempopb.SearchRequest{ + Tags: map[string]string{ + "service name": "foo", + "http.url": "search", + }, + Limit: q.cfg.SearchDefaultResultLimit, + }, + }, } for _, tt := range tests { From 08189e410e53889b300165349df82692b43a6c9d Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Tue, 19 Oct 2021 18:21:50 +0200 Subject: [PATCH 02/13] Use go-logfmt/logfmt --- modules/querier/querier_search.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/modules/querier/querier_search.go b/modules/querier/querier_search.go index e3311f13a59..05ffe80de5e 100644 --- a/modules/querier/querier_search.go +++ b/modules/querier/querier_search.go @@ -8,6 +8,7 @@ import ( "strings" "time" + "github.com/go-logfmt/logfmt" "github.com/grafana/tempo/pkg/tempopb" ) @@ -38,13 +39,11 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e } if encodedTags, ok := extractQueryParam(r, urlParamTags); ok { - // TODO handle tag values wrapped in quotes - tags := strings.Split(encodedTags, " ") + d := logfmt.NewDecoder(strings.NewReader(encodedTags)) - for _, tag := range tags { - splitTags := strings.SplitN(tag, "=", 2) - if len(splitTags) == 2 { - req.Tags[splitTags[0]] = splitTags[1] + for d.ScanRecord() { + for d.ScanKeyval() { + req.Tags[string(d.Key())] = string(d.Value()) } } } From e515f0577ecaad0925591192507baee9434a3d46 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Tue, 19 Oct 2021 20:18:23 +0200 Subject: [PATCH 03/13] go mod tidy --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 1dafe51a478..06a6cc50a6e 100644 --- a/go.mod +++ b/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 From ff0037ce1fce1b4669e897021ecc7cc8d428980e Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Tue, 19 Oct 2021 20:28:13 +0200 Subject: [PATCH 04/13] Drop test case with space in tag name --- modules/querier/querier_search_test.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/modules/querier/querier_search_test.go b/modules/querier/querier_search_test.go index 40345101dd6..b823decdbc1 100644 --- a/modules/querier/querier_search_test.go +++ b/modules/querier/querier_search_test.go @@ -130,17 +130,6 @@ func TestQuerierParseSearchRequest(t *testing.T) { Limit: q.cfg.SearchDefaultResultLimit, }, }, - { - name: "Tags query parameter with space in tag name", - urlQuery: "tags=%22service%20name%22%3Dfoo%20http.url%3Dsearch", - expected: &tempopb.SearchRequest{ - Tags: map[string]string{ - "service name": "foo", - "http.url": "search", - }, - Limit: q.cfg.SearchDefaultResultLimit, - }, - }, } for _, tt := range tests { From a6f578d00033ce3d56ee33e3bea846b0f224ab03 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Tue, 19 Oct 2021 23:17:29 +0200 Subject: [PATCH 05/13] Add more test cases --- modules/querier/querier_search.go | 18 +++++--- modules/querier/querier_search_test.go | 60 ++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 7 deletions(-) diff --git a/modules/querier/querier_search.go b/modules/querier/querier_search.go index f58b53ca92c..7842e0efc32 100644 --- a/modules/querier/querier_search.go +++ b/modules/querier/querier_search.go @@ -38,13 +38,7 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e } if encodedTags, ok := extractQueryParam(r, urlParamTags); ok { - d := logfmt.NewDecoder(strings.NewReader(encodedTags)) - - for d.ScanRecord() { - for d.ScanKeyval() { - req.Tags[string(d.Key())] = string(d.Value()) - } - } + parseEncodedTags(encodedTags, req.Tags) } if s, ok := extractQueryParam(r, urlParamMinDuration); ok { @@ -89,3 +83,13 @@ func extractQueryParam(r *http.Request, param string) (string, bool) { value := r.URL.Query().Get(param) return value, value != "" } + +func parseEncodedTags(encodedTags string, tags map[string]string) { + d := logfmt.NewDecoder(strings.NewReader(encodedTags)) + + for d.ScanRecord() { + for d.ScanKeyval() { + tags[string(d.Key())] = string(d.Value()) + } + } +} diff --git a/modules/querier/querier_search_test.go b/modules/querier/querier_search_test.go index b823decdbc1..b86ddffd1e1 100644 --- a/modules/querier/querier_search_test.go +++ b/modules/querier/querier_search_test.go @@ -147,3 +147,63 @@ func TestQuerierParseSearchRequest(t *testing.T) { }) } } + +func TestParseEncodedTags(t *testing.T) { + tests := []struct { + name string + encodedTags string + tags map[string]string + }{ + { + name: "Tags", + encodedTags: "service.name=foo http.url=api/search", + tags: map[string]string{ + "service.name": "foo", + "http.url": "api/search", + }, + }, + { + name: "Tag value with space", + encodedTags: "service.name=\"foo bar\" http.url=api/search", + tags: map[string]string{ + "service.name": "foo bar", + "http.url": "api/search", + }, + }, + { + name: "Tag without value", + encodedTags: "service.name=\"foo bar\" http.url=api/search error", + tags: map[string]string{ + "service.name": "foo bar", + "http.url": "api/search", + "error": "", + }, + }, + { + name: "Tag without name", + encodedTags: "service.name=\"foo bar\" http.url=api/search =error", + tags: map[string]string{ + "service.name": "foo bar", + "http.url": "api/search", + }, + }, + { + name: "Funky characters", + encodedTags: "service%name=\"foo=bar\" http&url=\"foo\\\"bar\\\"bzz\"", + tags: map[string]string{ + "service%name": "foo=bar", + "http&url": "foo\"bar\"bzz", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tags := map[string]string{} + + parseEncodedTags(tt.encodedTags, tags) + + assert.Equal(t, tt.tags, tags) + }) + } +} From efb993e18ba38cbb7d2498d9507f27ef7804f7b3 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Tue, 19 Oct 2021 23:31:44 +0200 Subject: [PATCH 06/13] Error if tag has been set twice --- modules/querier/querier_search.go | 18 +++++++++++++++--- modules/querier/querier_search_test.go | 17 ++++++++++++++--- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/modules/querier/querier_search.go b/modules/querier/querier_search.go index 7842e0efc32..fab97b01de0 100644 --- a/modules/querier/querier_search.go +++ b/modules/querier/querier_search.go @@ -33,12 +33,18 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e } if len(v) > 0 && v[0] != "" { + if _, ok := req.Tags[k]; ok { + return nil, fmt.Errorf("invalid tags: tag %s has been set twice", k) + } req.Tags[k] = v[0] } } if encodedTags, ok := extractQueryParam(r, urlParamTags); ok { - parseEncodedTags(encodedTags, req.Tags) + err := parseEncodedTags(encodedTags, req.Tags) + if err != nil { + return nil, fmt.Errorf("invalid tags: %w", err) + } } if s, ok := extractQueryParam(r, urlParamMinDuration); ok { @@ -84,12 +90,18 @@ func extractQueryParam(r *http.Request, param string) (string, bool) { return value, value != "" } -func parseEncodedTags(encodedTags string, tags map[string]string) { +func parseEncodedTags(encodedTags string, tags map[string]string) error { d := logfmt.NewDecoder(strings.NewReader(encodedTags)) for d.ScanRecord() { for d.ScanKeyval() { - tags[string(d.Key())] = string(d.Value()) + key := string(d.Key()) + if _, ok := tags[key]; ok { + return fmt.Errorf("tag %s has been set twice", key) + } + tags[key] = string(d.Value()) } } + + return nil } diff --git a/modules/querier/querier_search_test.go b/modules/querier/querier_search_test.go index b86ddffd1e1..6b5d89a73c6 100644 --- a/modules/querier/querier_search_test.go +++ b/modules/querier/querier_search_test.go @@ -107,11 +107,17 @@ func TestQuerierParseSearchRequest(t *testing.T) { Limit: q.cfg.SearchDefaultResultLimit, }, }, + { + 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: "Tags query parameter with top-level tags", - urlQuery: "service.name=bar&tags=service.name%3Dfoo%20http.url%3Dsearch&test=bar", + urlQuery: "service.id=5&tags=service.name%3Dfoo%20http.url%3Dsearch&test=bar", expected: &tempopb.SearchRequest{ Tags: map[string]string{ + "service.id": "5", "service.name": "foo", "http.url": "search", "test": "bar", @@ -119,6 +125,11 @@ func TestQuerierParseSearchRequest(t *testing.T) { Limit: q.cfg.SearchDefaultResultLimit, }, }, + { + name: "Tags query parameter with top-level tags with duplicate tag", + urlQuery: "service.name=bar&tags=service.name%3Dfoo%20http.url%3Dsearch&test=bar", + err: "invalid tags: tag service.name has been set twice", + }, { name: "Tags query parameter with space in value", urlQuery: "tags=service.name%3D%22my%20service%22%20http.url%3Dsearch", @@ -199,9 +210,9 @@ func TestParseEncodedTags(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tags := map[string]string{} + tags := make(map[string]string) - parseEncodedTags(tt.encodedTags, tags) + _ = parseEncodedTags(tt.encodedTags, tags) assert.Equal(t, tt.tags, tags) }) From 17f8fe08e24c8d514a843c5ca88caec7f8583059 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Wed, 20 Oct 2021 00:45:37 +0200 Subject: [PATCH 07/13] Structure tests better --- modules/querier/querier_search.go | 3 - modules/querier/querier_search_test.go | 138 +++++++------------------ 2 files changed, 38 insertions(+), 103 deletions(-) diff --git a/modules/querier/querier_search.go b/modules/querier/querier_search.go index fab97b01de0..7b5bc42b163 100644 --- a/modules/querier/querier_search.go +++ b/modules/querier/querier_search.go @@ -33,9 +33,6 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e } if len(v) > 0 && v[0] != "" { - if _, ok := req.Tags[k]; ok { - return nil, fmt.Errorf("invalid tags: tag %s has been set twice", k) - } req.Tags[k] = v[0] } } diff --git a/modules/querier/querier_search_test.go b/modules/querier/querier_search_test.go index 6b5d89a73c6..24980e771db 100644 --- a/modules/querier/querier_search_test.go +++ b/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,14 +25,14 @@ 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{}, @@ -38,7 +40,7 @@ func TestQuerierParseSearchRequest(t *testing.T) { }, }, { - name: "With limit exceeding max", + name: "limit exceeding max", urlQuery: "limit=120", expected: &tempopb.SearchRequest{ Tags: map[string]string{}, @@ -46,22 +48,22 @@ func TestQuerierParseSearchRequest(t *testing.T) { }, }, { - 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,81 +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", - urlQuery: "tags=service.name%3Dfoo%20http.url%3Dsearch", - expected: &tempopb.SearchRequest{ - Tags: map[string]string{ - "service.name": "foo", - "http.url": "search", - }, - Limit: q.cfg.SearchDefaultResultLimit, - }, - }, - { - name: "Tags query parameter with duplicate tag", + 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: "Tags query parameter with top-level tags", - urlQuery: "service.id=5&tags=service.name%3Dfoo%20http.url%3Dsearch&test=bar", - expected: &tempopb.SearchRequest{ - Tags: map[string]string{ - "service.id": "5", - "service.name": "foo", - "http.url": "search", - "test": "bar", - }, - Limit: q.cfg.SearchDefaultResultLimit, - }, - }, - { - name: "Tags query parameter with top-level tags with duplicate tag", - urlQuery: "service.name=bar&tags=service.name%3Dfoo%20http.url%3Dsearch&test=bar", + name: "top-level tags with conflicing query parameter tags", + urlQuery: "service.name=bar&tags=service.name%3Dfoo", err: "invalid tags: tag service.name has been set twice", }, - { - name: "Tags query parameter with space in value", - urlQuery: "tags=service.name%3D%22my%20service%22%20http.url%3Dsearch", - expected: &tempopb.SearchRequest{ - Tags: map[string]string{ - "service.name": "my service", - "http.url": "search", - }, - Limit: q.cfg.SearchDefaultResultLimit, - }, - }, } 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) @@ -159,62 +128,31 @@ func TestQuerierParseSearchRequest(t *testing.T) { } } -func TestParseEncodedTags(t *testing.T) { +func TestQuerierParseSearchRequestTags(t *testing.T) { + type strMap map[string]string + tests := []struct { - name string - encodedTags string - tags map[string]string + tags string + expected map[string]string }{ - { - name: "Tags", - encodedTags: "service.name=foo http.url=api/search", - tags: map[string]string{ - "service.name": "foo", - "http.url": "api/search", - }, - }, - { - name: "Tag value with space", - encodedTags: "service.name=\"foo bar\" http.url=api/search", - tags: map[string]string{ - "service.name": "foo bar", - "http.url": "api/search", - }, - }, - { - name: "Tag without value", - encodedTags: "service.name=\"foo bar\" http.url=api/search error", - tags: map[string]string{ - "service.name": "foo bar", - "http.url": "api/search", - "error": "", - }, - }, - { - name: "Tag without name", - encodedTags: "service.name=\"foo bar\" http.url=api/search =error", - tags: map[string]string{ - "service.name": "foo bar", - "http.url": "api/search", - }, - }, - { - name: "Funky characters", - encodedTags: "service%name=\"foo=bar\" http&url=\"foo\\\"bar\\\"bzz\"", - tags: map[string]string{ - "service%name": "foo=bar", - "http&url": "foo\"bar\"bzz", - }, - }, + {"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 =error", strMap{"service.name": "foo"}}, + {"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.name, func(t *testing.T) { - tags := make(map[string]string) + 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) - _ = parseEncodedTags(tt.encodedTags, tags) + searchRequest, err := (&Querier{}).parseSearchRequest(r) - assert.Equal(t, tt.tags, tags) + assert.NoError(t, err) + assert.Equal(t, tt.expected, searchRequest.Tags) }) } } From 6143b55de71195c1b2fefc4feecb5e99fa257786 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Wed, 20 Oct 2021 00:47:55 +0200 Subject: [PATCH 08/13] Refactor --- modules/querier/querier_search.go | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/modules/querier/querier_search.go b/modules/querier/querier_search.go index 7b5bc42b163..0ba2976c10f 100644 --- a/modules/querier/querier_search.go +++ b/modules/querier/querier_search.go @@ -38,9 +38,16 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e } if encodedTags, ok := extractQueryParam(r, urlParamTags); ok { - err := parseEncodedTags(encodedTags, req.Tags) - if err != nil { - return nil, fmt.Errorf("invalid tags: %w", err) + d := logfmt.NewDecoder(strings.NewReader(encodedTags)) + + for d.ScanRecord() { + for d.ScanKeyval() { + key := string(d.Key()) + if _, ok := req.Tags[key]; ok { + return nil, fmt.Errorf("invalid tags: tag %s has been set twice", key) + } + req.Tags[key] = string(d.Value()) + } } } @@ -86,19 +93,3 @@ func extractQueryParam(r *http.Request, param string) (string, bool) { value := r.URL.Query().Get(param) return value, value != "" } - -func parseEncodedTags(encodedTags string, tags map[string]string) error { - d := logfmt.NewDecoder(strings.NewReader(encodedTags)) - - for d.ScanRecord() { - for d.ScanKeyval() { - key := string(d.Key()) - if _, ok := tags[key]; ok { - return fmt.Errorf("tag %s has been set twice", key) - } - tags[key] = string(d.Value()) - } - } - - return nil -} From 188b11b65994ed4f50a3efe0004aaac411cc9ddd Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Wed, 20 Oct 2021 00:53:18 +0200 Subject: [PATCH 09/13] Add explanation --- modules/querier/querier_search.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/querier/querier_search.go b/modules/querier/querier_search.go index 0ba2976c10f..0ab335ac65b 100644 --- a/modules/querier/querier_search.go +++ b/modules/querier/querier_search.go @@ -26,6 +26,9 @@ 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 == urlParamTags || k == urlParamMinDuration || k == urlParamMaxDuration || k == api.URLParamLimit { From 833c6bcfe10b7e978df0498e63f50f84c5325544 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Wed, 20 Oct 2021 00:55:17 +0200 Subject: [PATCH 10/13] Typo --- modules/querier/querier_search_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/querier/querier_search_test.go b/modules/querier/querier_search_test.go index 24980e771db..9140d24565b 100644 --- a/modules/querier/querier_search_test.go +++ b/modules/querier/querier_search_test.go @@ -105,7 +105,7 @@ func TestQuerierParseSearchRequest(t *testing.T) { err: "invalid tags: tag service.name has been set twice", }, { - name: "top-level tags with conflicing query parameter tags", + 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", }, From 8bafe6abfbb4d281ac38caa3b76942c6e6d5080a Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Wed, 20 Oct 2021 12:52:06 +0200 Subject: [PATCH 11/13] Bubble up errors from logfmt parsing --- modules/querier/querier_search.go | 17 ++++++++++++----- modules/querier/querier_search_test.go | 24 +++++++++++++++++++++++- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/modules/querier/querier_search.go b/modules/querier/querier_search.go index 0ab335ac65b..e9fd811894f 100644 --- a/modules/querier/querier_search.go +++ b/modules/querier/querier_search.go @@ -41,17 +41,24 @@ func (q *Querier) parseSearchRequest(r *http.Request) (*tempopb.SearchRequest, e } if encodedTags, ok := extractQueryParam(r, urlParamTags); ok { - d := logfmt.NewDecoder(strings.NewReader(encodedTags)) + decoder := logfmt.NewDecoder(strings.NewReader(encodedTags)) - for d.ScanRecord() { - for d.ScanKeyval() { - key := string(d.Key()) + 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(d.Value()) + 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 { diff --git a/modules/querier/querier_search_test.go b/modules/querier/querier_search_test.go index 9140d24565b..117b0e20eb1 100644 --- a/modules/querier/querier_search_test.go +++ b/modules/querier/querier_search_test.go @@ -138,7 +138,6 @@ func TestQuerierParseSearchRequestTags(t *testing.T) { {"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 =error", strMap{"service.name": "foo"}}, {"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\""}}, @@ -156,3 +155,26 @@ func TestQuerierParseSearchRequestTags(t *testing.T) { }) } } + +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) + }) + } +} From c268bf0f57f0c08924f080dfeacd59276bd35f03 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Wed, 20 Oct 2021 13:19:04 +0200 Subject: [PATCH 12/13] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f20e78b4785..448899a291f 100644 --- a/CHANGELOG.md +++ b/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) From 02a5f6766a8e9d1532aa9ccfcea77853cfc83003 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Wed, 20 Oct 2021 13:23:28 +0200 Subject: [PATCH 13/13] Add test case for tag value with = --- modules/querier/querier_search_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/querier/querier_search_test.go b/modules/querier/querier_search_test.go index 117b0e20eb1..15d51146805 100644 --- a/modules/querier/querier_search_test.go +++ b/modules/querier/querier_search_test.go @@ -139,6 +139,7 @@ func TestQuerierParseSearchRequestTags(t *testing.T) { {"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\""}}, }