chore: add ingest request body size limit #4278
Conversation
| err = fmt.Errorf("request body too large: %w", err) | ||
| status = http.StatusRequestEntityTooLarge | ||
| validation.DiscardedBytes.WithLabelValues(string(validation.BodySizeLimit), tenantID).Add(float64(maxBytesError.Limit)) | ||
| validation.DiscardedProfiles.WithLabelValues(string(validation.BodySizeLimit), tenantID).Add(float64(1)) |
There was a problem hiding this comment.
For DiscardedProfiles, we don't get to read the body, so I increase it by 1, similarly to ingester.go.
For DiscardedBytes, only increased by Limit, we never get to read the rest of the message so we don't really know how big it was
| @@ -139,6 +140,7 @@ func (e LimitError) Error() string { | |||
| func (l *Limits) RegisterFlags(f *flag.FlagSet) { | |||
| f.Float64Var(&l.IngestionRateMB, "distributor.ingestion-rate-limit-mb", 4, "Per-tenant ingestion rate limit in sample size per second. Units in MB.") | |||
| f.Float64Var(&l.IngestionBurstSizeMB, "distributor.ingestion-burst-size-mb", 2, "Per-tenant allowed ingestion burst size (in sample size). Units in MB. The burst size refers to the per-distributor local rate limiter, and should be set at least to the maximum profile size expected in a single push request.") | |||
| f.Float64Var(&l.IngestionBodyLimitMB, "distributor.ingestion-body-limit-mb", 0, "Per-tenant ingestion body size limit in MB, before decompressing. 0 to disable.") | |||
There was a problem hiding this comment.
I opted to disable it by default, so we make sure no existing pyroscope starts failing after an update.
alsoba13
changed the title
alsoba13
requested review from
korniltsev,
marcsanmi,
aleks-p and
a team
as code owners
There was a problem hiding this comment.
Looks good overall.
Does an operator have enough information to act, in case the limit is breached? Would we easily know how often and by how much the limit is breached (e.g., will we see the actual values in the logs)?
Also, #4275 had real-world tests (exercising the actual /ingest path with profiling data). Is it possible we add that here to test the wiring itself?
I'll work on some tests in there, thank you! With this solution, an operator will only know that the limit was reached. This issue tries to bound the memory and CPU usage for huge incoming messages (typically from malfunctioning clients). |
Makes sense, lets not add extra work for logging purposes, especially if we don't intend to change this limit frequently. |
Properly approaches #4275