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.

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

Revert "Improving Performance on the API Gzip Handler (#12363)" #12476

Merged
merged 1 commit into from Jun 20, 2023

Conversation

roidelapluie
Copy link
Member

This reverts commit dfae954.

…)"

This reverts commit dfae954.

Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
@roidelapluie roidelapluie changed the base branch from main to release-2.45 June 20, 2023 11:40
@roidelapluie
Copy link
Member Author

/prombench v2.45.0-rc.0

@prombot
Copy link
Contributor

prombot commented Jun 20, 2023

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-12476 and v2.45.0-rc.0

After successful deployment, the benchmarking metrics can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart v2.45.0-rc.0

Copy link
Member

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

I don't understand the mechanism (and I spent several hours trying to), but I agree with the results.

@roidelapluie roidelapluie merged commit d5fb601 into prometheus:release-2.45 Jun 20, 2023
24 checks passed
@roidelapluie
Copy link
Member Author

/prombench restart v2.44.0

@prombot
Copy link
Contributor

prombot commented Jun 20, 2023

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-12476 and v2.44.0

After successful deployment, the benchmarking metrics can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart v2.44.0

@jesusvazquez
Copy link
Member

Latest benchmark shows that now the branch has better performance than 2.44
image

So we're going to cut 2.45.0-rc.1 out of the current state of the release branch

cc @alanprot We remove this change from the release due to the performance implications. Letting you know in case you want to invest more time finding out if we can use that new lib without the performance impact. 🙏

@jesusvazquez
Copy link
Member

/prombench cancel

@prombot
Copy link
Contributor

prombot commented Jun 20, 2023

Benchmark cancel is in progress.

@alanprot
Copy link
Contributor

Yeah... indeed seems to be using more CPU. But (if im reading this correctly) also seems also that at some point we have a better latency and more req/second using the new handler? Do we know what happens around 6 that the req/sec start to decrease and de latency to increase (and is where we see a better latency with the new handler)? Maybe at this time the response become bigger and is where the new gzip is better?

Screenshot 2023-06-20 at 8 35 59 AM

@bboreham
Copy link
Member

bboreham commented Jun 20, 2023

Some of the queries are from -2h to -1h, so they will start to see data at that point.

I think you have pointed out one part I didn't get - the requests/sec drops because queries are taking longer than the load-generator interval, and so the faster one (rc-0) is doing more work.
However rc-0 is slower before that point and using more CPU so I think it is worth looking into how we can improve that.

@alanprot
Copy link
Contributor

However rc-0 is slower before that point and using more CPU so I think it is worth looking into how we can improve that.

Yeah, I agree... rc-0 is always using more CPU and we need to understand why. I was just pointing out that seems the rc-0 is faster after some point. (I wonder if the new lib is less efficient with smaller payload but more efficient with big ones or something like that - just speculation for now). I will see if i have some time this week to look into it!

@roidelapluie
Copy link
Member Author

Thanks @alanprot :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants