-
Notifications
You must be signed in to change notification settings - Fork 823
Add active series limit for native histogram samples #6796
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
Add active series limit for native histogram samples #6796
Conversation
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.
Overall changes looks good. Please add tests.
CHANGELOG.md
Outdated
@@ -1,6 +1,7 @@ | |||
# Changelog | |||
|
|||
## master / unreleased | |||
* [ENHANCEMENT] Ingester: Add activeSeries limit specifically for NativeHistograms. #6796 |
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.
This changelog item needs to be grouped with other enhancement items
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.
Thanks. Updated.
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.
Please remove this line since you already added it below
024c1e9
to
ab7970d
Compare
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.
LGTM! Thank you.
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.
Thanks. I think this looks good. Just some nits
pkg/ingester/limiter.go
Outdated
localLimit := l.limits.MaxLocalNativeHistogramSeriesPerUser(userID) | ||
globalLimit := l.limits.MaxGlobalNativeHistogramSeriesPerUser(userID) | ||
|
||
return fmt.Errorf("per-user nativeHistograms series limit of %d exceeded, %s (local limit: %d global limit: %d actual local limit: %d)", |
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.
Let's be consistent with other messages. per-user native histogram series limit...
Signed-off-by: Paurush Garg <[email protected]>
0d0a007
to
5e35219
Compare
What this PR does:
In addition to the current active series limit on float+histogram samples, this PR adds an active series limit for specifically for native histogram
samples.
This is done, b/c the ingestion of native histogram samples is much more CPU intensive than that of float samples - adding native histogram samples specific ingestion limit to protect the service and to allow clients to adjust the NH series ingestion.
Currently, Prometheus doesn't provide a way to count the specific active native histogram series in the head and hence this PR uses the activeNativeHistogram series count from Cortex - which differs in the way active series is counted.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]