Skip to content

Conversation

alvinlin123
Copy link
Contributor

What this PR does:
See title

Which issue(s) this PR fixes:
Fixes #7009

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

…user series error message

Signed-off-by: Alvin Lin <alvinlin123@gmail.com>
@alvinlin123 alvinlin123 force-pushed the issue-7009-label-series-limit-err-msg-fix branch from bf5147f to 904a106 Compare September 5, 2025 16:09
Copy link
Contributor

@justinjung04 justinjung04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

return fmt.Errorf("per-labelset series limit of %d exceeded (labelSet: %s, local limit: %d global limit: %d actual)",
minNonZero(err.globalLimit, err.localLimit), err.id, err.localLimit, err.globalLimit)
// per labelset limit does not have a configurable local limit like per user max series, so local limit is always zero.
return fmt.Errorf("per-labelset series limit of %d exceeded (labelSet: %s, local limit: 0 global limit: %d actual local limit: %d)",
Copy link
Contributor

@justinjung04 justinjung04 Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should we drop local limit: 0 string? I don't see much value in it if it's always 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alvinlin123!,
I think it would be better to expose only one of the two: actualLocalLimit or localLimit.

Copy link
Contributor Author

@alvinlin123 alvinlin123 Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup I think that's a good idea to drop the "localLimit" as we don't have local limit config now for per label set series limit. We can always add it back if we decided to add the local limit. Will update this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

Signed-off-by: Alvin Lin <alvinlin123@gmail.com>
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 9, 2025
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@yeya24 yeya24 merged commit 45b52d3 into cortexproject:master Sep 9, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ingester lgtm This PR has been approved by a maintainer size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message for max series per label set is incomplete
4 participants