From 8eda83b724201d1e79cd88a209df91e43f24f018 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Fri, 23 Aug 2024 13:27:32 -0400 Subject: [PATCH 1/9] dedicated columns tracker thing Signed-off-by: Joe Elliott --- modules/frontend/dedicated_columns_to_json.go | 44 +++++++++++++ .../dedicated_columns_to_json_test.go | 66 +++++++++++++++++++ 2 files changed, 110 insertions(+) create mode 100644 modules/frontend/dedicated_columns_to_json.go create mode 100644 modules/frontend/dedicated_columns_to_json_test.go diff --git a/modules/frontend/dedicated_columns_to_json.go b/modules/frontend/dedicated_columns_to_json.go new file mode 100644 index 00000000000..3f7bbc2e68e --- /dev/null +++ b/modules/frontend/dedicated_columns_to_json.go @@ -0,0 +1,44 @@ +package frontend + +import ( + "encoding/json" + "unsafe" + + "github.com/grafana/tempo/tempodb/backend" +) + +type DedicatedColumnsToJSON struct { + columnsToJSON map[uint64]string +} + +func NewDedicatedColumnsToJSON() *DedicatedColumnsToJSON { + return &DedicatedColumnsToJSON{ + columnsToJSON: make(map[uint64]string), + } +} + +func (d *DedicatedColumnsToJSON) JSONForDedicatedColumns(cols backend.DedicatedColumns) (string, error) { + if len(cols) == 0 { + return "", nil // jpe - api.Build needs to handle empty string + } + + hash := cols.Hash() + if jsonString, ok := d.columnsToJSON[hash]; ok { + return jsonString, nil + } + + proto, err := cols.ToTempopb() + if err != nil { + return "", err + } + + jsonBytes, err := json.Marshal(proto) + if err != nil { + return "", err + } + + jsonString := unsafe.String(unsafe.SliceData(jsonBytes), len(jsonBytes)) + d.columnsToJSON[hash] = jsonString + + return jsonString, nil +} diff --git a/modules/frontend/dedicated_columns_to_json_test.go b/modules/frontend/dedicated_columns_to_json_test.go new file mode 100644 index 00000000000..c4a90073670 --- /dev/null +++ b/modules/frontend/dedicated_columns_to_json_test.go @@ -0,0 +1,66 @@ +package frontend + +import ( + "encoding/json" + "math/rand/v2" + "testing" + + "github.com/grafana/tempo/pkg/util/test" + "github.com/grafana/tempo/tempodb/backend" + "github.com/stretchr/testify/require" +) + +func TestDedicatedColumnsToJson(t *testing.T) { + d := NewDedicatedColumnsToJSON() + + testCols := []backend.DedicatedColumns{} + for i := 0; i < 10; i++ { + testCols = append(testCols, randoDedicatedCols()) + } + + // do all test cols 2x to test caching + for i := 0; i < 2; i++ { + for _, cols := range testCols { + expectedJSON := dedicatedColsToJSON(t, cols) + actualJSON, err := d.JSONForDedicatedColumns(cols) + require.NoError(t, err) + + require.Equal(t, expectedJSON, actualJSON, "iteration %d, cols: %v", i, cols) + } + } +} + +func dedicatedColsToJSON(t *testing.T, cols backend.DedicatedColumns) string { + t.Helper() + + proto, err := cols.ToTempopb() + require.NoError(t, err) + + jsonBytes, err := json.Marshal(proto) + require.NoError(t, err) + + return string(jsonBytes) +} + +// randoDedicatedCols generates a random set of cols for testing +func randoDedicatedCols() backend.DedicatedColumns { + colCount := rand.IntN(5) + 1 + ret := make([]backend.DedicatedColumn, 0, colCount) + + for i := 0; i < colCount; i++ { + scope := backend.DedicatedColumnScopeSpan + if rand.IntN(2) == 0 { + scope = backend.DedicatedColumnScopeResource + } + + col := backend.DedicatedColumn{ + Scope: scope, + Name: test.RandomString(), + Type: backend.DedicatedColumnTypeString, + } + + ret = append(ret, col) + } + + return ret +} From 85463b4d1a067aec65dc15bb75bbc06317b6e4e3 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 26 Aug 2024 09:55:44 -0400 Subject: [PATCH 2/9] update traceql search Signed-off-by: Joe Elliott --- modules/frontend/dedicated_columns_to_json.go | 5 ++-- .../dedicated_columns_to_json_test.go | 2 +- modules/frontend/search_sharder.go | 28 ++++++++++--------- modules/querier/external/client.go | 7 ++++- pkg/api/http.go | 12 ++++---- pkg/api/http_test.go | 6 +++- 6 files changed, 35 insertions(+), 25 deletions(-) diff --git a/modules/frontend/dedicated_columns_to_json.go b/modules/frontend/dedicated_columns_to_json.go index 3f7bbc2e68e..769e158d679 100644 --- a/modules/frontend/dedicated_columns_to_json.go +++ b/modules/frontend/dedicated_columns_to_json.go @@ -7,11 +7,12 @@ import ( "github.com/grafana/tempo/tempodb/backend" ) +// jpe - move to api? type DedicatedColumnsToJSON struct { columnsToJSON map[uint64]string } -func NewDedicatedColumnsToJSON() *DedicatedColumnsToJSON { +func newDedicatedColumnsToJSON() *DedicatedColumnsToJSON { return &DedicatedColumnsToJSON{ columnsToJSON: make(map[uint64]string), } @@ -19,7 +20,7 @@ func NewDedicatedColumnsToJSON() *DedicatedColumnsToJSON { func (d *DedicatedColumnsToJSON) JSONForDedicatedColumns(cols backend.DedicatedColumns) (string, error) { if len(cols) == 0 { - return "", nil // jpe - api.Build needs to handle empty string + return "", nil } hash := cols.Hash() diff --git a/modules/frontend/dedicated_columns_to_json_test.go b/modules/frontend/dedicated_columns_to_json_test.go index c4a90073670..88696ce514a 100644 --- a/modules/frontend/dedicated_columns_to_json_test.go +++ b/modules/frontend/dedicated_columns_to_json_test.go @@ -11,7 +11,7 @@ import ( ) func TestDedicatedColumnsToJson(t *testing.T) { - d := NewDedicatedColumnsToJSON() + d := newDedicatedColumnsToJSON() testCols := []backend.DedicatedColumns{} for i := 0; i < 10; i++ { diff --git a/modules/frontend/search_sharder.go b/modules/frontend/search_sharder.go index e87d0b0f96d..6d9826fee13 100644 --- a/modules/frontend/search_sharder.go +++ b/modules/frontend/search_sharder.go @@ -304,6 +304,8 @@ func buildBackendRequests(ctx context.Context, tenantID string, parent *http.Req queryHash := hashForSearchRequest(searchReq) + colsToJSON := newDedicatedColumnsToJSON() + for _, m := range metas { pages := pagesPerRequest(m, bytesPerRequest) if pages == 0 { @@ -314,25 +316,25 @@ func buildBackendRequests(ctx context.Context, tenantID string, parent *http.Req for startPage := 0; startPage < int(m.TotalRecords); startPage += pages { subR := parent.Clone(ctx) - dc, err := m.DedicatedColumns.ToTempopb() + jsonString, err := colsToJSON.JSONForDedicatedColumns(m.DedicatedColumns) if err != nil { errFn(fmt.Errorf("failed to convert dedicated columns. block: %s tempopb: %w", blockID, err)) continue } subR, err = api.BuildSearchBlockRequest(subR, &tempopb.SearchBlockRequest{ - BlockID: blockID, - StartPage: uint32(startPage), - PagesToSearch: uint32(pages), - Encoding: m.Encoding.String(), - IndexPageSize: m.IndexPageSize, - TotalRecords: m.TotalRecords, - DataEncoding: m.DataEncoding, - Version: m.Version, - Size_: m.Size, - FooterSize: m.FooterSize, - DedicatedColumns: dc, - }) + BlockID: blockID, + StartPage: uint32(startPage), + PagesToSearch: uint32(pages), + Encoding: m.Encoding.String(), + IndexPageSize: m.IndexPageSize, + TotalRecords: m.TotalRecords, + DataEncoding: m.DataEncoding, + Version: m.Version, + Size_: m.Size, + FooterSize: m.FooterSize, + // DedicatedColumns: dc, for perf reason we pass dedicated columns json in directly to not have to recalc everytime + }, jsonString) if err != nil { errFn(fmt.Errorf("failed to build search block request. block: %s tempopb: %w", blockID, err)) continue diff --git a/modules/querier/external/client.go b/modules/querier/external/client.go index 3b77edad92c..1be7dfa1f61 100644 --- a/modules/querier/external/client.go +++ b/modules/querier/external/client.go @@ -3,6 +3,7 @@ package external import ( "bytes" "context" + "encoding/json" "fmt" "io" "math/rand" @@ -149,7 +150,11 @@ func (s *Client) Search(ctx context.Context, maxBytes int, searchReq *tempopb.Se if err != nil { return nil, fmt.Errorf("external endpoint failed to make new request: %w", err) } - req, err = api.BuildSearchBlockRequest(req, searchReq) + columnsJSON, err := json.Marshal(searchReq.DedicatedColumns) + if err != nil { + return nil, err + } + req, err = api.BuildSearchBlockRequest(req, searchReq, string(columnsJSON)) if err != nil { return nil, fmt.Errorf("external endpoint failed to build search block request: %w", err) } diff --git a/pkg/api/http.go b/pkg/api/http.go index f0753ebc07e..50165075c87 100644 --- a/pkg/api/http.go +++ b/pkg/api/http.go @@ -442,6 +442,7 @@ func BuildQueryInstantRequest(req *http.Request, searchReq *tempopb.QueryInstant return req } +// jpe - convert me func BuildQueryRangeRequest(req *http.Request, searchReq *tempopb.QueryRangeRequest) *http.Request { if req == nil { req = &http.Request{ @@ -466,6 +467,7 @@ func BuildQueryRangeRequest(req *http.Request, searchReq *tempopb.QueryRangeRequ qb.addParam(urlParamEncoding, searchReq.Encoding) qb.addParam(urlParamSize, strconv.Itoa(int(searchReq.Size_))) qb.addParam(urlParamFooterSize, strconv.Itoa(int(searchReq.FooterSize))) + if len(searchReq.DedicatedColumns) > 0 { columnsJSON, _ := json.Marshal(searchReq.DedicatedColumns) qb.addParam(urlParamDedicatedColumns, string(columnsJSON)) @@ -642,7 +644,7 @@ func BuildSearchRequest(req *http.Request, searchReq *tempopb.SearchRequest) (*h // BuildSearchBlockRequest takes a tempopb.SearchBlockRequest and populates the passed http.Request // with the appropriate params. If no http.Request is provided a new one is created. -func BuildSearchBlockRequest(req *http.Request, searchReq *tempopb.SearchBlockRequest) (*http.Request, error) { +func BuildSearchBlockRequest(req *http.Request, searchReq *tempopb.SearchBlockRequest, dedicatedColumsJSON string) (*http.Request, error) { if req == nil { req = &http.Request{ URL: &url.URL{}, @@ -665,12 +667,8 @@ func BuildSearchBlockRequest(req *http.Request, searchReq *tempopb.SearchBlockRe qb.addParam(urlParamDataEncoding, searchReq.DataEncoding) qb.addParam(urlParamVersion, searchReq.Version) qb.addParam(urlParamFooterSize, strconv.FormatUint(uint64(searchReq.FooterSize), 10)) - if len(searchReq.DedicatedColumns) > 0 { - columnsJSON, err := json.Marshal(searchReq.DedicatedColumns) - if err != nil { - return nil, err - } - qb.addParam(urlParamDedicatedColumns, string(columnsJSON)) + if len(dedicatedColumsJSON) > 0 && dedicatedColumsJSON != "null" { // if a caller marshals a nil dedicated cols we will receive the string "null" + qb.addParam(urlParamDedicatedColumns, string(dedicatedColumsJSON)) } req.URL.RawQuery = qb.query() diff --git a/pkg/api/http_test.go b/pkg/api/http_test.go index 0204f03dd21..bfe9b2227aa 100644 --- a/pkg/api/http_test.go +++ b/pkg/api/http_test.go @@ -1,6 +1,7 @@ package api import ( + "encoding/json" "fmt" "net/http" "net/http/httptest" @@ -477,7 +478,10 @@ func TestBuildSearchBlockRequest(t *testing.T) { } for _, tc := range tests { - actualURL, err := BuildSearchBlockRequest(tc.httpReq, tc.req) + jsonBytes, err := json.Marshal(tc.req.DedicatedColumns) + require.NoError(t, err) + + actualURL, err := BuildSearchBlockRequest(tc.httpReq, tc.req, string(jsonBytes)) assert.NoError(t, err) assert.Equal(t, tc.query, actualURL.URL.String()) } From e36d95e48514b72341c6a2a3fba2471c01336933 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 26 Aug 2024 10:15:24 -0400 Subject: [PATCH 3/9] query range Signed-off-by: Joe Elliott --- modules/frontend/metrics_query_handler.go | 4 +-- .../frontend/metrics_query_range_handler.go | 2 +- .../metrics_query_range_handler_test.go | 2 +- .../frontend/metrics_query_range_sharder.go | 34 ++++++++----------- modules/frontend/search_sharder.go | 7 ++-- pkg/api/http.go | 14 ++++---- pkg/api/http_test.go | 5 ++- 7 files changed, 32 insertions(+), 36 deletions(-) diff --git a/modules/frontend/metrics_query_handler.go b/modules/frontend/metrics_query_handler.go index 2038d5887eb..46a83816039 100644 --- a/modules/frontend/metrics_query_handler.go +++ b/modules/frontend/metrics_query_handler.go @@ -45,7 +45,7 @@ func newQueryInstantStreamingGRPCHandler(cfg Config, next pipeline.AsyncRoundTri URL: &url.URL{Path: downstreamPath}, Header: http.Header{}, Body: io.NopCloser(bytes.NewReader([]byte{})), - }, qr) + }, qr, "") // dedicated cols are never passed from the caller httpReq = httpReq.Clone(ctx) var finalResponse *tempopb.QueryInstantResponse @@ -110,7 +110,7 @@ func newMetricsQueryInstantHTTPHandler(cfg Config, next pipeline.AsyncRoundTripp // Clone existing to keep it unaltered. req = req.Clone(req.Context()) req.URL.Path = strings.ReplaceAll(req.URL.Path, api.PathMetricsQueryInstant, api.PathMetricsQueryRange) - req = api.BuildQueryRangeRequest(req, qr) + req = api.BuildQueryRangeRequest(req, qr, "") // dedicated cols are never passed from the caller combiner, err := combiner.NewTypedQueryRange(qr, false) if err != nil { diff --git a/modules/frontend/metrics_query_range_handler.go b/modules/frontend/metrics_query_range_handler.go index dd8e760a466..4c0ed89afe1 100644 --- a/modules/frontend/metrics_query_range_handler.go +++ b/modules/frontend/metrics_query_range_handler.go @@ -29,7 +29,7 @@ func newQueryRangeStreamingGRPCHandler(cfg Config, next pipeline.AsyncRoundTripp URL: &url.URL{Path: downstreamPath}, Header: http.Header{}, Body: io.NopCloser(bytes.NewReader([]byte{})), - }, req) + }, req, "") // dedicated cols are never passed from the caller ctx := srv.Context() httpReq = httpReq.WithContext(ctx) diff --git a/modules/frontend/metrics_query_range_handler_test.go b/modules/frontend/metrics_query_range_handler_test.go index 19c50dac201..43fcd5ae20a 100644 --- a/modules/frontend/metrics_query_range_handler_test.go +++ b/modules/frontend/metrics_query_range_handler_test.go @@ -56,7 +56,7 @@ func TestQueryRangeHandlerSucceeds(t *testing.T) { Start: uint64(1100 * time.Second), End: uint64(1200 * time.Second), Step: uint64(100 * time.Second), - }) + }, "") ctx := user.InjectOrgID(httpReq.Context(), tenant) httpReq = httpReq.WithContext(ctx) diff --git a/modules/frontend/metrics_query_range_sharder.go b/modules/frontend/metrics_query_range_sharder.go index 0357b480159..2d6c1e3edc2 100644 --- a/modules/frontend/metrics_query_range_sharder.go +++ b/modules/frontend/metrics_query_range_sharder.go @@ -208,6 +208,7 @@ func (s *queryRangeSharder) buildBackendRequests(ctx context.Context, tenantID s defer close(reqCh) queryHash := hashForQueryRangeRequest(&searchReq) + colsToJSON := newDedicatedColumnsToJSON() exemplarsPerBlock := s.exemplarsPerShard(uint32(len(metas))) for _, m := range metas { @@ -231,7 +232,7 @@ func (s *queryRangeSharder) buildBackendRequests(ctx context.Context, tenantID s for startPage := 0; startPage < int(m.TotalRecords); startPage += pages { subR := parent.Clone(ctx) - dc, err := m.DedicatedColumns.ToTempopb() + dedColsJSON, err := colsToJSON.JSONForDedicatedColumns(m.DedicatedColumns) if err != nil { // errFn(fmt.Errorf("failed to convert dedicated columns. block: %s tempopb: %w", blockID, err)) continue @@ -253,18 +254,18 @@ func (s *queryRangeSharder) buildBackendRequests(ctx context.Context, tenantID s Step: step, QueryMode: searchReq.QueryMode, // New RF1 fields - BlockID: m.BlockID.String(), - StartPage: uint32(startPage), - PagesToSearch: uint32(pages), - Version: m.Version, - Encoding: m.Encoding.String(), - Size_: m.Size, - FooterSize: m.FooterSize, - DedicatedColumns: dc, - Exemplars: exemplars, + BlockID: m.BlockID.String(), + StartPage: uint32(startPage), + PagesToSearch: uint32(pages), + Version: m.Version, + Encoding: m.Encoding.String(), + Size_: m.Size, + FooterSize: m.FooterSize, + // DedicatedColumns: dc, for perf reason we pass dedicated columns json in directly to not have to realloc object -> proto -> json + Exemplars: exemplars, } - subR = api.BuildQueryRangeRequest(subR, queryRangeReq) + subR = api.BuildQueryRangeRequest(subR, queryRangeReq, dedColsJSON) prepareRequestForQueriers(subR, tenantID) pipelineR := pipeline.NewHTTPRequest(subR) @@ -302,16 +303,11 @@ func (s *queryRangeSharder) generatorRequest(searchReq tempopb.QueryRangeRequest searchReq.QueryMode = querier.QueryModeRecent searchReq.Exemplars = uint32(s.cfg.MaxExemplars) // TODO: Review this - req := s.toUpstreamRequest(parent.Context(), searchReq, parent, tenantID) - - return req -} - -func (s *queryRangeSharder) toUpstreamRequest(ctx context.Context, req tempopb.QueryRangeRequest, parent *http.Request, tenantID string) *http.Request { - subR := parent.Clone(ctx) - subR = api.BuildQueryRangeRequest(subR, &req) + subR := parent.Clone(parent.Context()) + subR = api.BuildQueryRangeRequest(subR, &searchReq, "") // dedicated cols are never passed to the generators prepareRequestForQueriers(subR, tenantID) + return subR } diff --git a/modules/frontend/search_sharder.go b/modules/frontend/search_sharder.go index 6d9826fee13..a83168d4fd6 100644 --- a/modules/frontend/search_sharder.go +++ b/modules/frontend/search_sharder.go @@ -303,7 +303,6 @@ func buildBackendRequests(ctx context.Context, tenantID string, parent *http.Req defer close(reqCh) queryHash := hashForSearchRequest(searchReq) - colsToJSON := newDedicatedColumnsToJSON() for _, m := range metas { @@ -316,7 +315,7 @@ func buildBackendRequests(ctx context.Context, tenantID string, parent *http.Req for startPage := 0; startPage < int(m.TotalRecords); startPage += pages { subR := parent.Clone(ctx) - jsonString, err := colsToJSON.JSONForDedicatedColumns(m.DedicatedColumns) + dedColsJSON, err := colsToJSON.JSONForDedicatedColumns(m.DedicatedColumns) if err != nil { errFn(fmt.Errorf("failed to convert dedicated columns. block: %s tempopb: %w", blockID, err)) continue @@ -333,8 +332,8 @@ func buildBackendRequests(ctx context.Context, tenantID string, parent *http.Req Version: m.Version, Size_: m.Size, FooterSize: m.FooterSize, - // DedicatedColumns: dc, for perf reason we pass dedicated columns json in directly to not have to recalc everytime - }, jsonString) + // DedicatedColumns: dc, for perf reason we pass dedicated columns json in directly to not have to realloc object -> proto -> json + }, dedColsJSON) if err != nil { errFn(fmt.Errorf("failed to build search block request. block: %s tempopb: %w", blockID, err)) continue diff --git a/pkg/api/http.go b/pkg/api/http.go index 50165075c87..b9c0476f34d 100644 --- a/pkg/api/http.go +++ b/pkg/api/http.go @@ -442,8 +442,7 @@ func BuildQueryInstantRequest(req *http.Request, searchReq *tempopb.QueryInstant return req } -// jpe - convert me -func BuildQueryRangeRequest(req *http.Request, searchReq *tempopb.QueryRangeRequest) *http.Request { +func BuildQueryRangeRequest(req *http.Request, searchReq *tempopb.QueryRangeRequest, dedicatedColumnsJSON string) *http.Request { if req == nil { req = &http.Request{ URL: &url.URL{}, @@ -468,9 +467,8 @@ func BuildQueryRangeRequest(req *http.Request, searchReq *tempopb.QueryRangeRequ qb.addParam(urlParamSize, strconv.Itoa(int(searchReq.Size_))) qb.addParam(urlParamFooterSize, strconv.Itoa(int(searchReq.FooterSize))) - if len(searchReq.DedicatedColumns) > 0 { - columnsJSON, _ := json.Marshal(searchReq.DedicatedColumns) - qb.addParam(urlParamDedicatedColumns, string(columnsJSON)) + if len(dedicatedColumnsJSON) > 0 && dedicatedColumnsJSON != "null" { // if a caller marshals a nil dedicated cols we will receive the string "null" + qb.addParam(urlParamDedicatedColumns, string(dedicatedColumnsJSON)) } if len(searchReq.Query) > 0 { @@ -644,7 +642,7 @@ func BuildSearchRequest(req *http.Request, searchReq *tempopb.SearchRequest) (*h // BuildSearchBlockRequest takes a tempopb.SearchBlockRequest and populates the passed http.Request // with the appropriate params. If no http.Request is provided a new one is created. -func BuildSearchBlockRequest(req *http.Request, searchReq *tempopb.SearchBlockRequest, dedicatedColumsJSON string) (*http.Request, error) { +func BuildSearchBlockRequest(req *http.Request, searchReq *tempopb.SearchBlockRequest, dedicatedColumnsJSON string) (*http.Request, error) { if req == nil { req = &http.Request{ URL: &url.URL{}, @@ -667,8 +665,8 @@ func BuildSearchBlockRequest(req *http.Request, searchReq *tempopb.SearchBlockRe qb.addParam(urlParamDataEncoding, searchReq.DataEncoding) qb.addParam(urlParamVersion, searchReq.Version) qb.addParam(urlParamFooterSize, strconv.FormatUint(uint64(searchReq.FooterSize), 10)) - if len(dedicatedColumsJSON) > 0 && dedicatedColumsJSON != "null" { // if a caller marshals a nil dedicated cols we will receive the string "null" - qb.addParam(urlParamDedicatedColumns, string(dedicatedColumsJSON)) + if len(dedicatedColumnsJSON) > 0 && dedicatedColumnsJSON != "null" { // if a caller marshals a nil dedicated cols we will receive the string "null" + qb.addParam(urlParamDedicatedColumns, string(dedicatedColumnsJSON)) } req.URL.RawQuery = qb.query() diff --git a/pkg/api/http_test.go b/pkg/api/http_test.go index bfe9b2227aa..6f340ebd38e 100644 --- a/pkg/api/http_test.go +++ b/pkg/api/http_test.go @@ -720,7 +720,10 @@ func TestQueryRangeRoundtrip(t *testing.T) { for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - httpReq := BuildQueryRangeRequest(nil, tc.req) + jsonBytes, err := json.Marshal(tc.req.DedicatedColumns) + require.NoError(t, err) + + httpReq := BuildQueryRangeRequest(nil, tc.req, string(jsonBytes)) actualReq, err := ParseQueryRangeRequest(httpReq) require.NoError(t, err) assert.Equal(t, tc.req, actualReq) From 4f8a6794bd1ae672a9e817619dc1bb64d915e578 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 26 Aug 2024 13:55:41 -0400 Subject: [PATCH 4/9] Add query builder test Signed-off-by: Joe Elliott --- modules/frontend/dedicated_columns_to_json.go | 1 - pkg/api/query_builder.go | 12 ++++-- pkg/api/query_builder_test.go | 38 +++++++++++++++++++ 3 files changed, 46 insertions(+), 5 deletions(-) create mode 100644 pkg/api/query_builder_test.go diff --git a/modules/frontend/dedicated_columns_to_json.go b/modules/frontend/dedicated_columns_to_json.go index 769e158d679..7ed976de371 100644 --- a/modules/frontend/dedicated_columns_to_json.go +++ b/modules/frontend/dedicated_columns_to_json.go @@ -7,7 +7,6 @@ import ( "github.com/grafana/tempo/tempodb/backend" ) -// jpe - move to api? type DedicatedColumnsToJSON struct { columnsToJSON map[uint64]string } diff --git a/pkg/api/query_builder.go b/pkg/api/query_builder.go index 4addb11dea0..7a43191ffec 100644 --- a/pkg/api/query_builder.go +++ b/pkg/api/query_builder.go @@ -19,17 +19,21 @@ func newQueryBuilder(init string) *queryBuilder { return qb } -// jpe - test me - // addParam adds a new key/val pair to the query // like https://cs.opensource.google/go/go/+/refs/tags/go1.22.5:src/net/url/url.go;l=972 func (qb *queryBuilder) addParam(key, value string) { if qb.builder.Len() > 0 { qb.builder.WriteByte('&') } - qb.builder.WriteString(url.QueryEscape(key)) + + keyStr := url.QueryEscape(key) + valueStr := url.QueryEscape(value) + + qb.builder.Grow(len(keyStr) + len(valueStr) + 1) + + qb.builder.WriteString(keyStr) qb.builder.WriteByte('=') - qb.builder.WriteString(url.QueryEscape(value)) + qb.builder.WriteString(valueStr) } func (qb *queryBuilder) query() string { diff --git a/pkg/api/query_builder_test.go b/pkg/api/query_builder_test.go new file mode 100644 index 00000000000..159c6086002 --- /dev/null +++ b/pkg/api/query_builder_test.go @@ -0,0 +1,38 @@ +package api + +import ( + "math/rand/v2" + "net/url" + "testing" + + "github.com/grafana/tempo/pkg/util/test" + "github.com/stretchr/testify/require" +) + +func TestQueryBuilder(t *testing.T) { + numParams := rand.IntN(10) + 1 + + qb := newQueryBuilder("") + params := url.Values{} + + for i := 0; i < numParams; i++ { + key := test.RandomString() + value := test.RandomString() + + qb.addParam(key, value) + params.Add(key, value) + } + + // url sorts params but query builder does not. parse the query builder + // string with url to guarantee its valid and also to sort the params + actualQuery, err := url.ParseQuery(qb.query()) + require.NoError(t, err) + + actual := url.URL{} + actual.RawQuery = actualQuery.Encode() + + expected := url.URL{} + expected.RawQuery = params.Encode() + + require.Equal(t, expected.RawQuery, actual.RawQuery) +} From 7ef7daa8838d69261c96ae82d6ecbb403e909ebd Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 26 Aug 2024 14:00:24 -0400 Subject: [PATCH 5/9] changelog Signed-off-by: Joe Elliott --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81ddc29d5d7..746bd9a6c47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [ENHANCEMENT] Prevent massive allocations in the frontend if there is not sufficient pressure from the query pipeline. [#3996](https://github.com/grafana/tempo/pull/3996) (@joe-elliott) **BREAKING CHANGE** Removed `querier_forget_delay` setting from the frontend. This configuration option did nothing. * [ENHANCEMENT] Update metrics-generator config in Tempo distributed docker compose example to serve TraceQL metrics [#4003](https://github.com/grafana/tempo/pull/4003) (@javiermolinar) +* [ENHANCEMENT] Reduce allocs related to marshalling dedicated columns repeatedly in the query frontend. [#4007](https://github.com/grafana/tempo/pull/4007) (@joe-elliott) # v2.6.0-rc.0 From 34bd367e10660b135a7f30395e3d885a9031dde7 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Mon, 26 Aug 2024 14:02:29 -0400 Subject: [PATCH 6/9] fix cli Signed-off-by: Joe Elliott --- cmd/tempo-cli/cmd-query-metrics-query-range.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/tempo-cli/cmd-query-metrics-query-range.go b/cmd/tempo-cli/cmd-query-metrics-query-range.go index 56d06aa5dad..7f15b766b81 100644 --- a/cmd/tempo-cli/cmd-query-metrics-query-range.go +++ b/cmd/tempo-cli/cmd-query-metrics-query-range.go @@ -114,7 +114,7 @@ func (cmd *metricsQueryCmd) queryRangeHTTP(req *tempopb.QueryRangeRequest) error return err } - httpReq = api.BuildQueryRangeRequest(httpReq, req) + httpReq = api.BuildQueryRangeRequest(httpReq, req, "") httpReq.Header = http.Header{} err = user.InjectOrgIDIntoHTTPRequest(user.InjectOrgID(context.Background(), cmd.OrgID), httpReq) if err != nil { From d0a2597379dbc405011929673a1ddd61c359390e Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Tue, 27 Aug 2024 15:48:16 -0400 Subject: [PATCH 7/9] lint Signed-off-by: Joe Elliott --- pkg/api/http.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/api/http.go b/pkg/api/http.go index b9c0476f34d..610215e2d91 100644 --- a/pkg/api/http.go +++ b/pkg/api/http.go @@ -468,7 +468,7 @@ func BuildQueryRangeRequest(req *http.Request, searchReq *tempopb.QueryRangeRequ qb.addParam(urlParamFooterSize, strconv.Itoa(int(searchReq.FooterSize))) if len(dedicatedColumnsJSON) > 0 && dedicatedColumnsJSON != "null" { // if a caller marshals a nil dedicated cols we will receive the string "null" - qb.addParam(urlParamDedicatedColumns, string(dedicatedColumnsJSON)) + qb.addParam(urlParamDedicatedColumns, dedicatedColumnsJSON) } if len(searchReq.Query) > 0 { @@ -666,7 +666,7 @@ func BuildSearchBlockRequest(req *http.Request, searchReq *tempopb.SearchBlockRe qb.addParam(urlParamVersion, searchReq.Version) qb.addParam(urlParamFooterSize, strconv.FormatUint(uint64(searchReq.FooterSize), 10)) if len(dedicatedColumnsJSON) > 0 && dedicatedColumnsJSON != "null" { // if a caller marshals a nil dedicated cols we will receive the string "null" - qb.addParam(urlParamDedicatedColumns, string(dedicatedColumnsJSON)) + qb.addParam(urlParamDedicatedColumns, dedicatedColumnsJSON) } req.URL.RawQuery = qb.query() From 782e6170b69ed74ef3eb89f456b3a1caf1d682a9 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Tue, 27 Aug 2024 15:53:19 -0400 Subject: [PATCH 8/9] lint and review Signed-off-by: Joe Elliott --- modules/frontend/metrics_query_range_sharder.go | 2 +- {modules/frontend => pkg/api}/dedicated_columns_to_json.go | 4 ++-- .../frontend => pkg/api}/dedicated_columns_to_json_test.go | 4 ++-- pkg/api/http.go | 5 +++++ 4 files changed, 10 insertions(+), 5 deletions(-) rename {modules/frontend => pkg/api}/dedicated_columns_to_json.go (91%) rename {modules/frontend => pkg/api}/dedicated_columns_to_json_test.go (96%) diff --git a/modules/frontend/metrics_query_range_sharder.go b/modules/frontend/metrics_query_range_sharder.go index 2d6c1e3edc2..eaa57cab2e6 100644 --- a/modules/frontend/metrics_query_range_sharder.go +++ b/modules/frontend/metrics_query_range_sharder.go @@ -208,7 +208,7 @@ func (s *queryRangeSharder) buildBackendRequests(ctx context.Context, tenantID s defer close(reqCh) queryHash := hashForQueryRangeRequest(&searchReq) - colsToJSON := newDedicatedColumnsToJSON() + colsToJSON := api.NewDedicatedColumnsToJSON() exemplarsPerBlock := s.exemplarsPerShard(uint32(len(metas))) for _, m := range metas { diff --git a/modules/frontend/dedicated_columns_to_json.go b/pkg/api/dedicated_columns_to_json.go similarity index 91% rename from modules/frontend/dedicated_columns_to_json.go rename to pkg/api/dedicated_columns_to_json.go index 7ed976de371..457ab161566 100644 --- a/modules/frontend/dedicated_columns_to_json.go +++ b/pkg/api/dedicated_columns_to_json.go @@ -1,4 +1,4 @@ -package frontend +package api import ( "encoding/json" @@ -11,7 +11,7 @@ type DedicatedColumnsToJSON struct { columnsToJSON map[uint64]string } -func newDedicatedColumnsToJSON() *DedicatedColumnsToJSON { +func NewDedicatedColumnsToJSON() *DedicatedColumnsToJSON { return &DedicatedColumnsToJSON{ columnsToJSON: make(map[uint64]string), } diff --git a/modules/frontend/dedicated_columns_to_json_test.go b/pkg/api/dedicated_columns_to_json_test.go similarity index 96% rename from modules/frontend/dedicated_columns_to_json_test.go rename to pkg/api/dedicated_columns_to_json_test.go index 88696ce514a..b6fc9b70cad 100644 --- a/modules/frontend/dedicated_columns_to_json_test.go +++ b/pkg/api/dedicated_columns_to_json_test.go @@ -1,4 +1,4 @@ -package frontend +package api import ( "encoding/json" @@ -11,7 +11,7 @@ import ( ) func TestDedicatedColumnsToJson(t *testing.T) { - d := newDedicatedColumnsToJSON() + d := NewDedicatedColumnsToJSON() testCols := []backend.DedicatedColumns{} for i := 0; i < 10; i++ { diff --git a/pkg/api/http.go b/pkg/api/http.go index 610215e2d91..2a4ed06c0f2 100644 --- a/pkg/api/http.go +++ b/pkg/api/http.go @@ -442,6 +442,9 @@ func BuildQueryInstantRequest(req *http.Request, searchReq *tempopb.QueryInstant return req } +// BuildQueryRangeRequest takes a tempopb.QueryRangeRequest and populates the passed http.Request +// dedicatedColumnsJSON should be generated using the DedicatedColumnsToJSON struct which produces the expected string +// value and memoizes results to prevent redundant marshaling. func BuildQueryRangeRequest(req *http.Request, searchReq *tempopb.QueryRangeRequest, dedicatedColumnsJSON string) *http.Request { if req == nil { req = &http.Request{ @@ -642,6 +645,8 @@ func BuildSearchRequest(req *http.Request, searchReq *tempopb.SearchRequest) (*h // BuildSearchBlockRequest takes a tempopb.SearchBlockRequest and populates the passed http.Request // with the appropriate params. If no http.Request is provided a new one is created. +// dedicatedColumnsJSON should be generated using the DedicatedColumnsToJSON struct which produces the expected string +// value and memoizes results to prevent redundant marshaling. func BuildSearchBlockRequest(req *http.Request, searchReq *tempopb.SearchBlockRequest, dedicatedColumnsJSON string) (*http.Request, error) { if req == nil { req = &http.Request{ From c3e60dd191341f80394f69cb60d4da8e5d6f7638 Mon Sep 17 00:00:00 2001 From: Joe Elliott Date: Tue, 27 Aug 2024 15:53:31 -0400 Subject: [PATCH 9/9] wups Signed-off-by: Joe Elliott --- modules/frontend/search_sharder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/frontend/search_sharder.go b/modules/frontend/search_sharder.go index a83168d4fd6..9278d7d74fc 100644 --- a/modules/frontend/search_sharder.go +++ b/modules/frontend/search_sharder.go @@ -303,7 +303,7 @@ func buildBackendRequests(ctx context.Context, tenantID string, parent *http.Req defer close(reqCh) queryHash := hashForSearchRequest(searchReq) - colsToJSON := newDedicatedColumnsToJSON() + colsToJSON := api.NewDedicatedColumnsToJSON() for _, m := range metas { pages := pagesPerRequest(m, bytesPerRequest)