-
Notifications
You must be signed in to change notification settings - Fork 810
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
discarded samples per labelset metrics for throttle by labelset #6492
discarded samples per labelset metrics for throttle by labelset #6492
Conversation
Signed-off-by: Ben Ye <[email protected]>
pkg/distributor/distributor.go
Outdated
@@ -1048,7 +1064,7 @@ func (d *Distributor) prepareSeriesKeys(ctx context.Context, req *cortexpb.Write | |||
matchedLabelSetLimits := validation.LimitsPerLabelSetsForSeries(limitsPerLabelSet, cortexpb.FromLabelAdaptersToLabels(validatedSeries.Labels)) | |||
if len(matchedLabelSetLimits) > 0 && labelSetCounters == nil { | |||
// TODO: use pool. | |||
labelSetCounters = make(map[uint64]*samplesLabelSetEntry, len(matchedLabelSetLimits)) | |||
labelSetCounters = make(map[uint64]*samplesLabelSetEntry, len(limitsPerLabelSet)) |
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.
Is there a reason why now we creating with the limitsPerLabelSet
size?
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.
Umm, I copied over the code I had from last PR and I am a bit lost here.
I think you are right the size should be at max matchedLabelSetLimits. I will update the code.
@@ -1201,6 +1229,8 @@ func (i *Ingester) Push(ctx context.Context, req *cortexpb.WriteRequest) (*corte | |||
|
|||
case errors.As(cause, &errMaxSeriesPerLabelSetLimitExceeded{}): | |||
perLabelSetSeriesLimitCount++ | |||
// We only track per labelset discarded samples for throttling by labelset limit. | |||
reasonCounter.increment(matchedLabelSetLimits, perLabelsetSeriesLimit) |
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.
Even though we are only tracking perLabelsetSeriesLimit
, we still wanna have this reasonCounter struct so we can easily add other reasons in the future, right?
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 LGTM! Thanks
Signed-off-by: Ben Ye <[email protected]>
What this PR does:
Similar to #6464, but we limited the reason to throttle by max series per labelset only to reduce complexity.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]