Skip to content

Query-frontend: Performance Improvements and Refactor#3888

Merged
joe-elliott merged 11 commits intografana:mainfrom
joe-elliott:ordered-search
Jul 23, 2024
Merged

Query-frontend: Performance Improvements and Refactor#3888
joe-elliott merged 11 commits intografana:mainfrom
joe-elliott:ordered-search

Conversation

@joe-elliott
Copy link
Copy Markdown
Collaborator

What this PR does:
Refactors the frontend to pass a Request interface instead of an *http.Request. This is progress toward supporting deterministic search. Also added a search pipeline benchmark and found a perf improvement in prepareRequestForQueriers.

> benchstat before.txt after.txt
goos: darwin
goarch: arm64
pkg: github.com/grafana/tempo/modules/frontend
                  │  before.txt  │             after.txt              │
                  │    sec/op    │    sec/op     vs base              │
SearchPipeline-11   609.4m ± ∞ ¹   604.0m ± ∞ ¹  -0.87% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95

                  │   before.txt   │              after.txt               │
                  │      B/op      │     B/op       vs base               │
SearchPipeline-11   10.642Mi ± ∞ ¹   8.659Mi ± ∞ ¹  -18.63% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95

                  │  before.txt  │             after.txt              │
                  │  allocs/op   │  allocs/op    vs base              │
SearchPipeline-11   163.0k ± ∞ ¹   148.3k ± ∞ ¹  -9.02% (p=0.008 n=5)
¹ need >= 6 samples for confidence interval at level 0.95

Checklist

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

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Copy link
Copy Markdown
Contributor

@mapno mapno left a comment

Choose a reason for hiding this comment

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

Refactor looks OK. Any idea why this is faster or uses less memory?

// - adds the tenant header
// - sets the requesturi (see below for details)
func prepareRequestForQueriers(req *http.Request, tenant string, originalURI string, params url.Values) {
func prepareRequestForQueriers(req *http.Request, tenant string) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find this function pretty obscure. Why don't we return a new http.Request based on the original one?

func getQuerierHTTPRequest(origin_req *http.Request, tenant string) *http.Request{
}

That way we will reduce the cognitive overload reading the code

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I would prefer modifying in place to reduce allocs

@joe-elliott
Copy link
Copy Markdown
Collaborator Author

joe-elliott commented Jul 23, 2024

Refactor looks OK. Any idea why this is faster or uses less memory?

I removed these calls to .Query() which allocs a map by changing the way prepareRequestForQueriers works:

https://github.com/grafana/tempo/pull/3888/files#diff-85974350e62b77bee6b5d88d537e3946839ccb1bba5f6dc89450a1c5489b5d4fL443

I have a follow up PR coming that will remove millions of these calls from the search path.

@joe-elliott joe-elliott merged commit 2fbc1f4 into grafana:main Jul 23, 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.

3 participants