-
Notifications
You must be signed in to change notification settings - Fork 706
Limit response sizes from query-frontends #13829
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
Conversation
|
💻 Deploy preview available (Limit response sizes from query-frontends): |
| size := payload.Size() | ||
| if uint64(size) > f.maxEncodedSize { | ||
| return nil, apierror.Newf(apierror.TypeTooLargeEntry, "Protobuf response (%d bytes) is too large: "+responseSizeTooLargeErrorFormat, size, f.maxEncodedSize) | ||
| } |
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.
Bug: Inconsistent zero-value handling between JSON and Protobuf formatters
The response size limit behaves inconsistently when MaxResponseSizeBytes is set to 0. In the JSON formatter (via the forked json-iterator), enforceMarshalledBytesLimit is set to config.maxMarshalledBytes > 0, meaning a value of 0 disables limit enforcement entirely (allowing unlimited sizes). However, in ProtobufFormatter, the check uint64(size) > f.maxEncodedSize is always true when maxEncodedSize is 0 and size > 0, causing all non-empty Protobuf responses to be rejected. This inconsistency could cause confusing behavior if someone sets the limit to 0.
Additional Locations (1)
| size := payload.Size() | ||
| if uint64(size) > f.maxEncodedSize { | ||
| return nil, apierror.Newf(apierror.TypeTooLargeEntry, "Protobuf response (%d bytes) is too large: "+responseSizeTooLargeErrorFormat, size, f.maxEncodedSize) | ||
| } |
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.
Bug: Inconsistent zero-value handling between JSON and Protobuf formatters
The response size limit behaves inconsistently when MaxResponseSizeBytes is set to 0. In the JSON formatter (via the forked json-iterator), enforceMarshalledBytesLimit is set to config.maxMarshalledBytes > 0, meaning a value of 0 disables limit enforcement entirely (allowing unlimited sizes). However, in ProtobufFormatter, the check uint64(size) > f.maxEncodedSize is always true when maxEncodedSize is 0 and size > 0, causing all non-empty Protobuf responses to be rejected. This inconsistency could cause confusing behavior if someone sets the limit to 0.
Additional Locations (1)
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.
Bug: Missing limit check in WriteUint16 early return path
In the forked json-iterator library, WriteUint16 is missing an enforceMaxBytes() call on its early return path when the value is less than 1000. While WriteUint32 correctly calls enforceMaxBytes() on all return paths (lines 82, 90, 104), WriteUint16 only calls it at line 61, not at the early return on line 56. This means when marshalling uint16/int16 values less than 1000, those specific bytes bypass the limit check, potentially allowing responses to slightly exceed the configured MaxResponseSizeBytes limit before the next write operation catches it.
vendor/github.com/json-iterator/go/stream_int.go#L53-L56
mimir/vendor/github.com/json-iterator/go/stream_int.go
Lines 53 to 56 in 9e702fe
| q1 := val / 1000 | |
| if q1 == 0 { | |
| stream.buf = writeFirstBuf(stream.buf, digits[val]) | |
| return |
| // WriteByte writes a single byte. | ||
| func (stream *Stream) writeByte(c byte) { | ||
| stream.buf = append(stream.buf, c) | ||
| stream.enforceMaxBytes() |
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.
Rather then call stream.enforceMaxBytes() everywhere, would it make sense to create an append() func on the Stream which does the buf append and calls this enforceMaxBytes()?
func (stream *Stream) write(obj ...byte) {
stream.buf = append(stream.buf, obj...)
stream.enforceMaxBytes()
}
Could also be a writeNoCheck() which does the append but does not do the max bytes checks - in the case of a string where we don't want to check the max bytes on every char appended.
| } | ||
| if i == valLen { | ||
| stream.buf = append(stream.buf, '"') | ||
| stream.enforceMaxBytes() |
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.
For strings, is it worth looking at the len(s) and peeking at the current accumulated buf len and making a preemptive test if the string is likely to exceed the max len?
This might provide against any really big strings coming into this.
| stream.enforceMaxBytes() | ||
| } | ||
|
|
||
| func (stream *Stream) enforceMaxBytes() { |
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.
Does the benchmark tool pick up any noticeable difference serializing large documents?
tcp13equals2
left a comment
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 left a couple of general comments - but overall this makes sense and looks good.
56quarters
left a comment
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 we already use quite a few replace directives but I don't like adding another one for something as fundamental as JSON encoding. Looks like the json-iterator repo was just archived. Is there any equivalent functionality in v2 of the stdlib JSON encoding?
| return | ||
| } | ||
|
|
||
| if uint64(len(stream.buf)) > stream.marshalledBytesLimitRemaining { |
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 panicking like this, what if we used the same approach that we use when reading/writing TSDB indexes and set an err member for the stream. This would allow callers to continue to call methods that don't return errors. Would that negate the purpose of the limit?
| f.BoolVar(&cfg.ShardActiveSeriesQueries, "query-frontend.shard-active-series-queries", false, "True to enable sharding of active series queries.") | ||
| f.BoolVar(&cfg.UseActiveSeriesDecoder, "query-frontend.use-active-series-decoder", false, "Set to true to use the zero-allocation response decoder for active series queries.") | ||
| f.BoolVar(&cfg.CacheSamplesProcessedStats, "query-frontend.cache-samples-processed-stats", false, "Cache statistics of processed samples on results cache. Deprecated: has no effect.") | ||
| f.Uint64Var(&cfg.MaxResponseSizeBytes, maxResponseSizeBytesFlag, 128*1024*1024, "Maximum allowed response size for query responses, in bytes.") |
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.
Can you clarify if this limit can be disabled and how? It doesn't seem like 0 will disable the limit.
| _, sp := tracer.Start(ctx, "APIResponse.ToHTTPResponse") | ||
| defer sp.End() | ||
|
|
||
| a, ok := res.GetPrometheusResponse() |
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.
The protobuf response object here has a method to compute its size. Maybe using the protobuf size as an estimate for how big the JSON response would be is good enough? It wouldn't require forking the JSON encoder.
tacole02
left a comment
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.
Docs look good! Thank you!
| * [CHANGE] Query-frontend: Removed support for calculating 'cache-adjusted samples processed' query statistic. The `-query-frontend.cache-samples-processed-stats` CLI flag has been deprecated and will be removed in a future release. Setting it has now no effect. #13582 | ||
| * [CHANGE] Querier: Renamed experimental flag `-querier.prefer-availability-zone` to `-querier.prefer-availability-zones` and changed it to accept a comma-separated list of availability zones. All zones in the list are given equal priority when querying ingesters and store-gateways. #13756 #13758 | ||
| * [CHANGE] Ingester: Stabilize experimental flag `-ingest-storage.write-logs-fsync-before-kafka-commit-concurrency` to fsync write logs before the offset is committed to Kafka. Remove `-ingest-storage.write-logs-fsync-before-kafka-commit-enabled` since this is always enabled now. #13591 | ||
| * [CHANGE] Query-frontend: Enforce a limit on the size of responses returned by query-frontends. Defaults to 128MB and can be configured with `-query-frontend.max-response-size-bytes`. #13829 |
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.
| * [CHANGE] Query-frontend: Enforce a limit on the size of responses returned by query-frontends. Defaults to 128MB and can be configured with `-query-frontend.max-response-size-bytes`. #13829 | |
| * [CHANGE] Query-frontend: Enforce a limit on the size of responses returned by query-frontends. Defaults to 128MB. You can configure this limit with `-query-frontend.max-response-size-bytes`. #13829 |
golang/go#56733 proposes a
We could close this and wait for a) this marshalling code to switch to using |
My preference is wait for this to (hopefully) be added to |
What this PR does
This PR introduces a limit on the response sizes produced by query-frontends.
If a response is larger than the configured limit, the request is aborted and an error is returned.
The default value, 128MB, is based on the response sizes we see in the real world for Grafana Cloud Metrics customers.
I've switched to a fork of
json-iterator/gocontaining the changes from json-iterator/go#721.Which issue(s) this PR fixes or relates to
(none)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.Note
Adds a configurable max query response size to query-frontends (default 128MB) and enforces it for JSON and Protobuf responses, updating docs, defaults, tests, and JSON library.
128MB) via-query-frontend.max-response-size-bytes/query-frontend.max_response_size_bytes.jsonandprotobufencodings; propagate size-limit errors usingglobalerror.MaxResponseSizeBytesandapierror.TypeTooLargeEntry.operations/mimir/mimir-flags-defaults.jsonand help output.help-all.txt.github.com/json-iterator/gowith a fork addingMaxMarshalledBytes; vendor updates to enforce output size.NewCodecsignature.Written by Cursor Bugbot for commit 9e702fe. This will update automatically on new commits. Configure here.