Conversation
javiermolinar
requested review from
carles-grafana,
electron0zero,
ie-pham,
knylander-grafana,
mapno,
mattdurham,
mdisibio,
oleg-kozlyuk-grafana,
ruslan-mikhailov,
stoewer,
yvrhdn and
zalegrala
as code owners
|
💻 Deploy preview deleted (chore: bump ingestion limits). |
There was a problem hiding this comment.
Pull request overview
Bumps the default per-tenant ingestion rate limit and burst size for the distributor, and updates tests/docs to reflect the new defaults.
Changes:
- Increase default
rate_limit_bytesfrom 15MB/s to 30MB/s. - Increase default
burst_size_bytesfrom 20MB to 30MB. - Update configuration/troubleshooting docs and add a unit test to lock in the new defaults.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
modules/overrides/config.go |
Updates the default distributor ingestion rate and burst flag defaults to 30e6. |
modules/overrides/config_test.go |
Adds a test asserting the new default ingestion limits are applied. |
docs/sources/tempo/troubleshooting/send-traces/max-trace-limit-reached.md |
Updates the troubleshooting example error to use 30,000,000 bytes. |
docs/sources/tempo/configuration/manifest.md |
Updates the rendered config manifest defaults for ingestion limits. |
docs/sources/tempo/configuration/_index.md |
Updates the documented defaults and examples for ingestion rate strategy/limits. |
| f.IntVar(&c.Defaults.Ingestion.RateLimitBytes, "distributor.ingestion-rate-limit-bytes", 30e6, "Per-user ingestion rate limit in bytes per second.") | ||
| f.IntVar(&c.Defaults.Ingestion.BurstSizeBytes, "distributor.ingestion-burst-size-bytes", 30e6, "Per-user ingestion burst size in bytes. Should be set to the expected size (in bytes) of a single push request.") |
There was a problem hiding this comment.
MEDIUM: This changes the default ingestion rate/burst limits, which is a user-facing behavior change. Could you add a corresponding entry under ## main / unreleased in CHANGELOG.md (following the existing category/order format) so operators see the new defaults in release notes?
| # Results in errors like | ||
| # RATE_LIMITED: ingestion rate limit (15000000 bytes) exceeded while | ||
| # RATE_LIMITED: ingestion rate limit (30000000 bytes) exceeded while | ||
| # adding 10 bytes | ||
| [rate_limit_bytes: <int> | default = 15000000 (15MB) ] | ||
| [rate_limit_bytes: <int> | default = 30000000 (30MB) ] | ||
|
|
||
| # Burst size (bytes) used in ingestion. | ||
| # Results in errors like | ||
| # RATE_LIMITED: ingestion rate limit (20000000 bytes) exceeded while | ||
| # RATE_LIMITED: ingestion rate limit (30000000 bytes) exceeded while | ||
| # adding 10 bytes | ||
| # Ignores rate strategy and is always local. | ||
| [burst_size_bytes: <int> | default = 20000000 (20MB) ] | ||
| [burst_size_bytes: <int> | default = 30000000 (30MB) ] |
There was a problem hiding this comment.
MEDIUM: The RATE_LIMITED error examples here don't match the current distributor error text. In modules/distributor/distributor.go the message includes local/global bytes/s and burst (and user), e.g. ingestion rate limit (local: ... bytes/s, global: ... bytes/s, burst: ... bytes) .... Since this section is being updated anyway, could we align the example to the actual message (same issue also appears in the troubleshooting page that shows the log line)?
What this PR does:
it bumps the ingestion limits to make more straighforward testing Tempo without running into limits
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]