Skip to content

feat: reduce allocs in query sharder#3932

Merged
joe-elliott merged 5 commits intografana:mainfrom
javiermolinar:reduce-allocs-trace-sharder
Aug 7, 2024
Merged

feat: reduce allocs in query sharder#3932
joe-elliott merged 5 commits intografana:mainfrom
javiermolinar:reduce-allocs-trace-sharder

Conversation

@javiermolinar
Copy link
Copy Markdown
Contributor

What this PR does:
Reduce memory allocations when sharding the query. Similar perf improvement as #3901

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

joe-elliott
joe-elliott previously approved these changes Aug 5, 2024
Copy link
Copy Markdown
Collaborator

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Looks like there's some minor conflicts to resolve. Once that's done we'll merge.

@joe-elliott joe-elliott dismissed their stale review August 5, 2024 13:36

had a second thought

Comment thread modules/frontend/traceid_sharder.go Outdated
reqs[i] = parent.Clone(ctx)

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

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
Copy Markdown
Contributor Author

@javiermolinar javiermolinar Aug 6, 2024

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
Copy Markdown
Contributor Author

@javiermolinar javiermolinar Aug 6, 2024

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
Copy Markdown
Collaborator

@joe-elliott joe-elliott Aug 6, 2024

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
Copy Markdown
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

@joe-elliott joe-elliott merged commit eae8c17 into grafana:main Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants