Add global label length limits#83
Open
bboreham wants to merge 1 commit intoprometheus-community:mainfrom
Open
Conversation
1. config/config.go — Default limits of 1 MiB - Added LabelNameLengthLimit: 1 << 20 and LabelValueLengthLimit: 1 << 20 to DefaultGlobalConfig - Added fallback logic in GlobalConfig.UnmarshalYAML so a partial global config (one that doesn't set these fields) still gets the defaults, matching the pattern already used for ScrapeInterval, EvaluationInterval, etc. 2. scrape/scrape.go — Pre-construction panic prevention Added a check before p.Labels(&lset) that rejects metrics whose raw text exceeds 16 MiB. Since the encoding format panics for any individual string > 16 MiB, and no single label value can be longer than the total metric string, len(met) > 1<<24 is a safe guard. The configured 1 MiB limit then catches the normal cases in verifyLabelLimits. 3. storage/remote/write_handler.go — Configurable limits in RW receiver - Added labelNameLengthLimit and labelValueLengthLimit fields to writeHandler - Added two int parameters to NewWriteHandler - Added checkLabelLengths() helper that enforces the limits - Wired the check into both v1 write() and v2 appendV2() paths 4. web/api/v1/api.go — Wires global config limits to RW handler Passes cfg.GlobalConfig.LabelNameLengthLimit and LabelValueLengthLimit from the config to NewWriteHandler at construction. 5. promql/functions.go — Guards in label_replace and label_join Added explicit checks for the 16 MiB encoding limit before the lb.Labels() call. The evaluator's existing defer ev.recover() already converts panics to errors, but these checks give a clear, explicit error message instead. Tests - Updated config/config_test.go to expect the new 1 MiB defaults in TestGetScrapeConfigs - Updated all ~16 NewWriteHandler call sites in write_handler_test.go to pass 0, 0 (no limit) for the new parameters
bboreham
requested review from
bwplotka,
cstyan,
roidelapluie and
tomwilkie
as code owners
|
I see there is a mismatch between the description and what is checked in for LabelValueLengthLimit:
LabelValueLengthLimit: 1 << 21, |
|
Please can you ensure you sign the DCO with your commit? Thanks! |
jdheyburn
reviewed
Mar 25, 2026
| if gc.LabelNameLengthLimit == 0 { | ||
| gc.LabelNameLengthLimit = DefaultGlobalConfig.LabelNameLengthLimit | ||
| } | ||
|
|
There was a problem hiding this comment.
gc.LabelValueLengthLimit is not initialised to DefaultGlobalConfig.LabelValueLengthLimit
jdheyburn
reviewed
Mar 25, 2026
| } | ||
|
|
||
| // checkLabelLengths returns an error if any label name or value in lset exceeds | ||
| // the configured limits. A limit of 0 means no limit. |
There was a problem hiding this comment.
If the user wishes to set no limit, and uint has a default value of 0, how can we differentiate between omission vs no limit? Perhaps we can use -1 instead to specify no limit?
jdheyburn
reviewed
Mar 25, 2026
| MetricNameEscapingScheme: model.AllowUTF8, | ||
| // Default to 1 MiB to avoid crashes from the 16 MiB encoding limit. | ||
| LabelNameLengthLimit: 1 << 20, | ||
| LabelValueLengthLimit: 1 << 21, |
There was a problem hiding this comment.
For consistency, could you omit the spaces here to match how the syntax is done later on in the code?
jdheyburn
reviewed
Mar 25, 2026
| indexes := regex.FindStringSubmatchIndex(srcVal) | ||
| if indexes != nil { // Only replace when regexp matches. | ||
| res := regex.ExpandString([]byte{}, repl, srcVal, indexes) | ||
| if len(res) > 1<<24 { |
There was a problem hiding this comment.
Could we set 1<<24 to a variable and referenced wherelse it is being used?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Note: this PR is an example for use in the KubeCon EU ContribFest session. Please leave if you are not in that session.
Added a check before p.Labels(&lset) that rejects metrics whose raw text exceeds 16 MiB. Since the encoding format panics for any individual string >
16 MiB, and no single label value can be longer than the total metric string, len(met) > 1<<24 is a safe guard. The configured 1 MiB limit then
catches the normal cases in verifyLabelLimits.
Passes cfg.GlobalConfig.LabelNameLengthLimit and LabelValueLengthLimit from the config to NewWriteHandler at construction.
Added explicit checks for the 16 MiB encoding limit before the lb.Labels() call. The evaluator's existing defer ev.recover() already converts panics
to errors, but these checks give a clear, explicit error message instead.
Tests
Which issue(s) does the PR fix:
Does this PR introduce a user-facing change?