-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Receive: add per request limits for remote write #5527
Merged
bwplotka
merged 27 commits into
thanos-io:main
from
douglascamata:douglascamata/add-per-request-write-limits
Aug 2, 2022
Merged
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
0dbf4ec
Add per request limits for remote write
douglascamata 9ac2340
Remove useless TODO item
douglascamata 7995468
Refactor write request limits test
douglascamata 87cef59
Add write concurrency limit to Receive
douglascamata 6cd031c
Change write limits config option name
douglascamata 83ab7ca
Document remote write concurrenty limit
douglascamata a6addd3
Merge branch 'main' of https://github.com/thanos-io/thanos into dougl…
douglascamata ab7fd37
Add changelog entry
douglascamata e836893
Format docs
douglascamata 3240f69
Extract request limiting logic from handler
douglascamata 1f38552
Add copyright header
douglascamata 77a404b
Add a TODO for per-tenant limits
douglascamata d00ea15
Add default value and hide the request limit flags
douglascamata 2400aac
Improve TODO comment in request limits
douglascamata 510248a
Update Receive docs after flags wre made hidden
douglascamata b8943a0
Add note about WIP in Receive request limits doc
douglascamata 7f5c41b
Fix typo in Receive docs
douglascamata 7071c22
Fix help text for concurrent request limit
douglascamata 2cd1014
Use byte unit helpers for improved readability
douglascamata 8561400
Removed check for nil writeGate
douglascamata 7efee5e
Better organize linebreaks
douglascamata 42d9f49
Fix help text for limits hit metric
douglascamata a98bcc3
Apply some english feedback
douglascamata cc83217
Improve limits & gates documentationb
douglascamata a9b5529
Fix import clause
douglascamata 58d362b
Use a 3 node hashring for write limits test
douglascamata 003d9aa
Fix comment
douglascamata File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of extending the existing handler, is it possible to make the limiter a middleware that wraps the handler? This could be cleaner to maintain long term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't think that's a good idea, because then the limiter middleware will have to do work that is done again in the main handler:
I wanted to avoid doing extra work at all costs to keep the "hot path" of Receive fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's a good point. Well initially I thought the middleware would be on this handler which already has the decoded protobuf request, but here we don't have the raw body correct?
Also if I may ask, what is the practical use of setting limits on the request body? As an admin, I am not sure I would know what to set this limit to. I can understand the impact of limiting series or samples, but I would find it hard to know how to configure the body size limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. When it gets to that handler (it does only forward request across the hashring) almost all the "heavy" work is done and some of it will be wasted. Examples:
The request size limit is more geared towards networking concerns, I would say. It prevents clients from writing very big requests (even when the other limits aren't configured or hit, i.e. very long label name/values, or when clients are writing "junk"). This can still cause OOMs or possibly create a situation similar to a slow client attack.
This makes me wonder why Receive's remote write endpoint doesn't have a configurable server-side timeout... possibly could add this in a follow up too.