Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: reduce allocs in query sharder #3932

Merged
merged 5 commits into from
Aug 7, 2024
Merged

feat: reduce allocs in query sharder #3932

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
35c6089
reduce allocs in query sharder
javiermolinar Aug 2, 2024
d448e65
changelog
javiermolinar Aug 2, 2024
494b88c
Merge branch 'main' into reduce-allocs-trace-sharder
javiermolinar Aug 6, 2024
ba5db1f
expose a single method to generate the url with query params
javiermolinar Aug 6, 2024
c0627c6
move helper method to common api/http.go
javiermolinar Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
* [ENHANCEMENT] Reduce log level verbosity for e2e tests[#3900](https://github.com/grafana/tempo/pull/3900) (@javiermolinar)
* [ENHANCEMENT] Added new Traces api V2[#3912](https://github.com/grafana/tempo/pull/3912) (@javiermolinar)
* [ENHANCEMENT] Update to the latest dskit [#3915](https://github.com/grafana/tempo/pull/3915) (@andreasgerstmayr)
* [ENHANCEMENT] Reduce allocs building queriers sharded requests [#3932](https://github.com/grafana/tempo/pull/3932) (@javiermolinar)

* [BUGFIX] Fix panic in certain metrics queries using `rate()` with `by` [#3847](https://github.com/grafana/tempo/pull/3847) (@stoewer)
* [BUGFIX] Fix double appending the primary iterator on second pass with event iterator [#3903](https://github.com/grafana/tempo/pull/3903) (@ie-pham)
Expand Down
14 changes: 8 additions & 6 deletions modules/frontend/traceid_sharder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/grafana/tempo/modules/frontend/combiner"
"github.com/grafana/tempo/modules/frontend/pipeline"
"github.com/grafana/tempo/modules/querier"
"github.com/grafana/tempo/pkg/api"
"github.com/grafana/tempo/pkg/blockboundary"
)

Expand Down Expand Up @@ -75,17 +76,18 @@ func (s *asyncTraceSharder) buildShardedRequests(ctx context.Context, parent *ht
for i := 0; i < len(s.blockBoundaries); i++ {
reqs[i] = parent.Clone(ctx)

q := reqs[i].URL.Query()
qb := api.NewQueryBuilder(reqs[i].URL.RawQuery)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt about making an api.Build...() method and leaving the query builder unexported from the pkg/api pacakge?

Copy link
Contributor Author

@javiermolinar javiermolinar Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this?

// It returns an URL with query params
func BuildQuery(url string, args ...string) (string, error) {
}

Copy link
Contributor Author

@javiermolinar javiermolinar Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to pass a map. The map can be reused to reduce allocations.

Copy link
Member

@joe-elliott joe-elliott Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking of something like this:

https://github.com/grafana/tempo/blob/main/pkg/api/http.go#L462

That takes this proto struct and encodes it into an http request:

https://github.com/grafana/tempo/blob/main/pkg/tempopb/tempo.proto#L50C9-L50C25

and turns it into an http request. Mainly for consistency with the other http requests. Not a blocker just something to consider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I've done the same but with the map and as a generic helper. I think it's nice to have it. I've moved the method to the commonplace and used the same structure


if i == 0 {
// ingester query
q.Add(querier.QueryModeKey, querier.QueryModeIngesters)
qb.AddParam(querier.QueryModeKey, querier.QueryModeIngesters)
} else {
// block queries
q.Add(querier.BlockStartKey, hex.EncodeToString(s.blockBoundaries[i-1]))
q.Add(querier.BlockEndKey, hex.EncodeToString(s.blockBoundaries[i]))
q.Add(querier.QueryModeKey, querier.QueryModeBlocks)
qb.AddParam(querier.BlockStartKey, hex.EncodeToString(s.blockBoundaries[i-1]))
qb.AddParam(querier.BlockEndKey, hex.EncodeToString(s.blockBoundaries[i]))
qb.AddParam(querier.QueryModeKey, querier.QueryModeBlocks)
}
reqs[i].URL.RawQuery = q.Encode()
reqs[i].URL.RawQuery = qb.Query()

prepareRequestForQueriers(reqs[i], userID)
}
Expand Down
2 changes: 1 addition & 1 deletion modules/frontend/traceid_sharder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,5 @@ func TestBuildShardedRequests(t *testing.T) {
require.Len(t, shardedReqs, queryShards)

require.Equal(t, "/querier?mode=ingesters", shardedReqs[0].RequestURI)
require.Equal(t, "/querier?blockEnd=ffffffffffffffffffffffffffffffff&blockStart=00000000000000000000000000000000&mode=blocks", shardedReqs[1].RequestURI)
urisEqual(t, []string{"/querier?blockEnd=ffffffffffffffffffffffffffffffff&blockStart=00000000000000000000000000000000&mode=blocks"}, []string{shardedReqs[1].RequestURI})
}
96 changes: 48 additions & 48 deletions pkg/api/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,12 +443,12 @@ func BuildQueryInstantRequest(req *http.Request, searchReq *tempopb.QueryInstant
return req
}

qb := newQueryBuilder("")
qb.addParam(urlParamStart, strconv.FormatUint(searchReq.Start, 10))
qb.addParam(urlParamEnd, strconv.FormatUint(searchReq.End, 10))
qb.addParam(urlParamQuery, searchReq.Query)
qb := NewQueryBuilder("")
qb.AddParam(urlParamStart, strconv.FormatUint(searchReq.Start, 10))
qb.AddParam(urlParamEnd, strconv.FormatUint(searchReq.End, 10))
qb.AddParam(urlParamQuery, searchReq.Query)

req.URL.RawQuery = qb.query()
req.URL.RawQuery = qb.Query()

return req
}
Expand All @@ -464,31 +464,31 @@ func BuildQueryRangeRequest(req *http.Request, searchReq *tempopb.QueryRangeRequ
return req
}

qb := newQueryBuilder("")
qb.addParam(urlParamStart, strconv.FormatUint(searchReq.Start, 10))
qb.addParam(urlParamEnd, strconv.FormatUint(searchReq.End, 10))
qb.addParam(urlParamStep, time.Duration(searchReq.Step).String())
qb.addParam(urlParamShard, strconv.FormatUint(uint64(searchReq.ShardID), 10))
qb.addParam(urlParamShardCount, strconv.FormatUint(uint64(searchReq.ShardCount), 10))
qb.addParam(QueryModeKey, searchReq.QueryMode)
qb := NewQueryBuilder("")
qb.AddParam(urlParamStart, strconv.FormatUint(searchReq.Start, 10))
qb.AddParam(urlParamEnd, strconv.FormatUint(searchReq.End, 10))
qb.AddParam(urlParamStep, time.Duration(searchReq.Step).String())
qb.AddParam(urlParamShard, strconv.FormatUint(uint64(searchReq.ShardID), 10))
qb.AddParam(urlParamShardCount, strconv.FormatUint(uint64(searchReq.ShardCount), 10))
qb.AddParam(QueryModeKey, searchReq.QueryMode)
// New RF1 params
qb.addParam(urlParamBlockID, searchReq.BlockID)
qb.addParam(urlParamStartPage, strconv.Itoa(int(searchReq.StartPage)))
qb.addParam(urlParamPagesToSearch, strconv.Itoa(int(searchReq.PagesToSearch)))
qb.addParam(urlParamVersion, searchReq.Version)
qb.addParam(urlParamEncoding, searchReq.Encoding)
qb.addParam(urlParamSize, strconv.Itoa(int(searchReq.Size_)))
qb.addParam(urlParamFooterSize, strconv.Itoa(int(searchReq.FooterSize)))
qb.AddParam(urlParamBlockID, searchReq.BlockID)
qb.AddParam(urlParamStartPage, strconv.Itoa(int(searchReq.StartPage)))
qb.AddParam(urlParamPagesToSearch, strconv.Itoa(int(searchReq.PagesToSearch)))
qb.AddParam(urlParamVersion, searchReq.Version)
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))
qb.AddParam(urlParamDedicatedColumns, string(columnsJSON))
}

if len(searchReq.Query) > 0 {
qb.addParam(urlParamQuery, searchReq.Query)
qb.AddParam(urlParamQuery, searchReq.Query)
}

req.URL.RawQuery = qb.query()
req.URL.RawQuery = qb.Query()

return req
}
Expand Down Expand Up @@ -597,24 +597,24 @@ func BuildSearchRequest(req *http.Request, searchReq *tempopb.SearchRequest) (*h
return req, nil
}

qb := newQueryBuilder("")
qb.addParam(urlParamStart, strconv.FormatUint(uint64(searchReq.Start), 10))
qb.addParam(urlParamEnd, strconv.FormatUint(uint64(searchReq.End), 10))
qb := NewQueryBuilder("")
qb.AddParam(urlParamStart, strconv.FormatUint(uint64(searchReq.Start), 10))
qb.AddParam(urlParamEnd, strconv.FormatUint(uint64(searchReq.End), 10))
if searchReq.Limit != 0 {
qb.addParam(urlParamLimit, strconv.FormatUint(uint64(searchReq.Limit), 10))
qb.AddParam(urlParamLimit, strconv.FormatUint(uint64(searchReq.Limit), 10))
}
if searchReq.MaxDurationMs != 0 {
qb.addParam(urlParamMaxDuration, strconv.FormatUint(uint64(searchReq.MaxDurationMs), 10)+"ms")
qb.AddParam(urlParamMaxDuration, strconv.FormatUint(uint64(searchReq.MaxDurationMs), 10)+"ms")
}
if searchReq.MinDurationMs != 0 {
qb.addParam(urlParamMinDuration, strconv.FormatUint(uint64(searchReq.MinDurationMs), 10)+"ms")
qb.AddParam(urlParamMinDuration, strconv.FormatUint(uint64(searchReq.MinDurationMs), 10)+"ms")
}
if searchReq.SpansPerSpanSet != 0 {
qb.addParam(urlParamSpansPerSpanSet, strconv.FormatUint(uint64(searchReq.SpansPerSpanSet), 10))
qb.AddParam(urlParamSpansPerSpanSet, strconv.FormatUint(uint64(searchReq.SpansPerSpanSet), 10))
}

if len(searchReq.Query) > 0 {
qb.addParam(urlParamQuery, searchReq.Query)
qb.AddParam(urlParamQuery, searchReq.Query)
}

if len(searchReq.Tags) > 0 {
Expand All @@ -628,10 +628,10 @@ func BuildSearchRequest(req *http.Request, searchReq *tempopb.SearchRequest) (*h
}
}

qb.addParam(urlParamTags, builder.String())
qb.AddParam(urlParamTags, builder.String())
}

req.URL.RawQuery = qb.query()
req.URL.RawQuery = qb.Query()

return req, nil
}
Expand All @@ -650,26 +650,26 @@ func BuildSearchBlockRequest(req *http.Request, searchReq *tempopb.SearchBlockRe
return nil, err
}

qb := newQueryBuilder(req.URL.RawQuery)
qb.addParam(urlParamBlockID, searchReq.BlockID)
qb.addParam(urlParamPagesToSearch, strconv.FormatUint(uint64(searchReq.PagesToSearch), 10))
qb.addParam(urlParamSize, strconv.FormatUint(searchReq.Size_, 10))
qb.addParam(urlParamStartPage, strconv.FormatUint(uint64(searchReq.StartPage), 10))
qb.addParam(urlParamEncoding, searchReq.Encoding)
qb.addParam(urlParamIndexPageSize, strconv.FormatUint(uint64(searchReq.IndexPageSize), 10))
qb.addParam(urlParamTotalRecords, strconv.FormatUint(uint64(searchReq.TotalRecords), 10))
qb.addParam(urlParamDataEncoding, searchReq.DataEncoding)
qb.addParam(urlParamVersion, searchReq.Version)
qb.addParam(urlParamFooterSize, strconv.FormatUint(uint64(searchReq.FooterSize), 10))
qb := NewQueryBuilder(req.URL.RawQuery)
qb.AddParam(urlParamBlockID, searchReq.BlockID)
qb.AddParam(urlParamPagesToSearch, strconv.FormatUint(uint64(searchReq.PagesToSearch), 10))
qb.AddParam(urlParamSize, strconv.FormatUint(searchReq.Size_, 10))
qb.AddParam(urlParamStartPage, strconv.FormatUint(uint64(searchReq.StartPage), 10))
qb.AddParam(urlParamEncoding, searchReq.Encoding)
qb.AddParam(urlParamIndexPageSize, strconv.FormatUint(uint64(searchReq.IndexPageSize), 10))
qb.AddParam(urlParamTotalRecords, strconv.FormatUint(uint64(searchReq.TotalRecords), 10))
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))
qb.AddParam(urlParamDedicatedColumns, string(columnsJSON))
}

req.URL.RawQuery = qb.query()
req.URL.RawQuery = qb.Query()

return req, nil
}
Expand All @@ -683,9 +683,9 @@ func AddServerlessParams(req *http.Request, maxBytes int) *http.Request {
}
}

qb := newQueryBuilder(req.URL.RawQuery)
qb.addParam(urlParamMaxBytes, strconv.FormatInt(int64(maxBytes), 10))
req.URL.RawQuery = qb.query()
qb := NewQueryBuilder(req.URL.RawQuery)
qb.AddParam(urlParamMaxBytes, strconv.FormatInt(int64(maxBytes), 10))
req.URL.RawQuery = qb.Query()

return req
}
Expand Down
12 changes: 6 additions & 6 deletions pkg/api/query_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import (
"strings"
)

type queryBuilder struct {
type QueryBuilder struct {
builder strings.Builder
}

// newQueryBuilder creates a new queryBuilder
func newQueryBuilder(init string) *queryBuilder {
qb := &queryBuilder{
// NewQueryBuilder creates a new queryBuilder
func NewQueryBuilder(init string) *QueryBuilder {
qb := &QueryBuilder{
builder: strings.Builder{},
}

Expand All @@ -23,7 +23,7 @@ func newQueryBuilder(init string) *queryBuilder {

// 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) {
func (qb *QueryBuilder) AddParam(key, value string) {
if qb.builder.Len() > 0 {
qb.builder.WriteByte('&')
}
Expand All @@ -32,6 +32,6 @@ func (qb *queryBuilder) addParam(key, value string) {
qb.builder.WriteString(url.QueryEscape(value))
}

func (qb *queryBuilder) query() string {
func (qb *QueryBuilder) Query() string {
return qb.builder.String()
}
70 changes: 35 additions & 35 deletions pkg/api/search_tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,13 +471,13 @@ func BuildSearchTagsRequest(req *http.Request, searchReq *tempopb.SearchTagsRequ
return req, nil
}

qb := newQueryBuilder("")
qb.addParam(urlParamStart, strconv.FormatUint(uint64(searchReq.Start), 10))
qb.addParam(urlParamEnd, strconv.FormatUint(uint64(searchReq.End), 10))
qb.addParam(urlParamScope, searchReq.Scope)
qb.addParam(urlParamQuery, searchReq.Query)
qb := NewQueryBuilder("")
qb.AddParam(urlParamStart, strconv.FormatUint(uint64(searchReq.Start), 10))
qb.AddParam(urlParamEnd, strconv.FormatUint(uint64(searchReq.End), 10))
qb.AddParam(urlParamScope, searchReq.Scope)
qb.AddParam(urlParamQuery, searchReq.Query)

req.URL.RawQuery = qb.query()
req.URL.RawQuery = qb.Query()

return req, nil
}
Expand All @@ -494,19 +494,19 @@ func BuildSearchTagsBlockRequest(req *http.Request, searchReq *tempopb.SearchTag
return nil, err
}

q := newQueryBuilder(req.URL.RawQuery)
q.addParam(urlParamSize, strconv.FormatUint(searchReq.Size_, 10))
q.addParam(urlParamBlockID, searchReq.BlockID)
q.addParam(urlParamStartPage, strconv.FormatUint(uint64(searchReq.StartPage), 10))
q.addParam(urlParamPagesToSearch, strconv.FormatUint(uint64(searchReq.PagesToSearch), 10))
q.addParam(urlParamEncoding, searchReq.Encoding)
q.addParam(urlParamIndexPageSize, strconv.FormatUint(uint64(searchReq.IndexPageSize), 10))
q.addParam(urlParamTotalRecords, strconv.FormatUint(uint64(searchReq.TotalRecords), 10))
q.addParam(urlParamDataEncoding, searchReq.DataEncoding)
q.addParam(urlParamVersion, searchReq.Version)
q.addParam(urlParamFooterSize, strconv.FormatUint(uint64(searchReq.FooterSize), 10))
q := NewQueryBuilder(req.URL.RawQuery)
q.AddParam(urlParamSize, strconv.FormatUint(searchReq.Size_, 10))
q.AddParam(urlParamBlockID, searchReq.BlockID)
q.AddParam(urlParamStartPage, strconv.FormatUint(uint64(searchReq.StartPage), 10))
q.AddParam(urlParamPagesToSearch, strconv.FormatUint(uint64(searchReq.PagesToSearch), 10))
q.AddParam(urlParamEncoding, searchReq.Encoding)
q.AddParam(urlParamIndexPageSize, strconv.FormatUint(uint64(searchReq.IndexPageSize), 10))
q.AddParam(urlParamTotalRecords, strconv.FormatUint(uint64(searchReq.TotalRecords), 10))
q.AddParam(urlParamDataEncoding, searchReq.DataEncoding)
q.AddParam(urlParamVersion, searchReq.Version)
q.AddParam(urlParamFooterSize, strconv.FormatUint(uint64(searchReq.FooterSize), 10))

req.URL.RawQuery = q.query()
req.URL.RawQuery = q.Query()

return req, nil
}
Expand All @@ -522,12 +522,12 @@ func BuildSearchTagValuesRequest(req *http.Request, searchReq *tempopb.SearchTag
return req, nil
}

qb := newQueryBuilder("")
qb.addParam(urlParamStart, strconv.FormatUint(uint64(searchReq.Start), 10))
qb.addParam(urlParamEnd, strconv.FormatUint(uint64(searchReq.End), 10))
qb.addParam(urlParamQuery, searchReq.Query)
qb := NewQueryBuilder("")
qb.AddParam(urlParamStart, strconv.FormatUint(uint64(searchReq.Start), 10))
qb.AddParam(urlParamEnd, strconv.FormatUint(uint64(searchReq.End), 10))
qb.AddParam(urlParamQuery, searchReq.Query)

req.URL.RawQuery = qb.query()
req.URL.RawQuery = qb.Query()

return req, nil
}
Expand All @@ -544,19 +544,19 @@ func BuildSearchTagValuesBlockRequest(req *http.Request, searchReq *tempopb.Sear
return nil, err
}

qb := newQueryBuilder(req.URL.RawQuery)
qb.addParam(urlParamSize, strconv.FormatUint(searchReq.Size_, 10))
qb.addParam(urlParamBlockID, searchReq.BlockID)
qb.addParam(urlParamStartPage, strconv.FormatUint(uint64(searchReq.StartPage), 10))
qb.addParam(urlParamPagesToSearch, strconv.FormatUint(uint64(searchReq.PagesToSearch), 10))
qb.addParam(urlParamEncoding, searchReq.Encoding)
qb.addParam(urlParamIndexPageSize, strconv.FormatUint(uint64(searchReq.IndexPageSize), 10))
qb.addParam(urlParamTotalRecords, strconv.FormatUint(uint64(searchReq.TotalRecords), 10))
qb.addParam(urlParamDataEncoding, searchReq.DataEncoding)
qb.addParam(urlParamVersion, searchReq.Version)
qb.addParam(urlParamFooterSize, strconv.FormatUint(uint64(searchReq.FooterSize), 10))
qb := NewQueryBuilder(req.URL.RawQuery)
qb.AddParam(urlParamSize, strconv.FormatUint(searchReq.Size_, 10))
qb.AddParam(urlParamBlockID, searchReq.BlockID)
qb.AddParam(urlParamStartPage, strconv.FormatUint(uint64(searchReq.StartPage), 10))
qb.AddParam(urlParamPagesToSearch, strconv.FormatUint(uint64(searchReq.PagesToSearch), 10))
qb.AddParam(urlParamEncoding, searchReq.Encoding)
qb.AddParam(urlParamIndexPageSize, strconv.FormatUint(uint64(searchReq.IndexPageSize), 10))
qb.AddParam(urlParamTotalRecords, strconv.FormatUint(uint64(searchReq.TotalRecords), 10))
qb.AddParam(urlParamDataEncoding, searchReq.DataEncoding)
qb.AddParam(urlParamVersion, searchReq.Version)
qb.AddParam(urlParamFooterSize, strconv.FormatUint(uint64(searchReq.FooterSize), 10))

req.URL.RawQuery = qb.query()
req.URL.RawQuery = qb.Query()

return req, nil
}
Loading