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

querier: check size of whole frontend response before sending it #10154

Merged
merged 12 commits into from
Dec 16, 2024

Conversation

dimitarvdimitrov
Copy link
Contributor

Background

Previously we'd check the size only of the body before sending a response to the query-frontend. The reason for this check is so that we don't leave it to the gRPC library. The library returns an error and doesn't tell the server anything. This results in timeouts at the query-frontend.

Problem

Because we only check the length of the body there are edge cases where the size of the whole response (including stats & headers) exceeds the limit. The gRPC library would still refuse to send the response and the frontend would time out waiting for it.

Is it really happening?

I discovered this in tests with very small limits. But there are probably realistic cases where this is happening: the body is just below the limit, add stats and headers, and it's over the limit.

Solution

Check the size of the whole response to the frontend, not just the httpgrpc body.

Checklist

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

@dimitarvdimitrov dimitarvdimitrov requested a review from a team as a code owner December 6, 2024 14:47
@chencs
Copy link
Contributor

chencs commented Dec 10, 2024

The CHANGELOG has just been cut to prepare for the next release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG.md document. Thanks!

Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

LGTM with some non-blocking comments

pkg/querier/worker/frontend_processor.go Outdated Show resolved Hide resolved
pkg/querier/worker/frontend_processor_test.go Show resolved Hide resolved
pkg/querier/worker/frontend_processor_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@narqo narqo left a comment

Choose a reason for hiding this comment

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

🔥

pkg/querier/worker/frontend_processor_test.go Outdated Show resolved Hide resolved
pkg/querier/worker/frontend_processor_test.go Outdated Show resolved Hide resolved
Signed-off-by: Dimitar Dimitrov <[email protected]>
### Background

Previously we'd check the size only of the body before sending a response to the query-frontend. The reason for this check is so that we don't leave it to the gRPC library. The library returns an error and doesn't tell the server anything. This results in timeouts at the query-frontend.

### Problem

Because we only check the length of the body there are edge cases where the size of the whole response (including stats & headers) exceeds the limit. The gRPC library would still refuse to send the response and the frontend would time out waiting for it.

### Is it really happening?

I discovered this in tests with very small limits. But there are probably realistic cases where this is happening: the body is just below the limit, add stats and headers, and it's over the limit.

Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
Signed-off-by: Dimitar Dimitrov <[email protected]>
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/querier/test-frontend-processor branch from 7d0c6c1 to 5c3cdb7 Compare December 16, 2024 15:15
@dimitarvdimitrov dimitarvdimitrov enabled auto-merge (squash) December 16, 2024 15:16
@dimitarvdimitrov dimitarvdimitrov merged commit ab55f55 into main Dec 16, 2024
29 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/querier/test-frontend-processor branch December 16, 2024 15:32
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.

4 participants