Skip to content
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 ingester flag to customize message in limiter error messages #5460

Merged
merged 6 commits into from
Jul 18, 2023

Conversation

kumanik
Copy link
Contributor

@kumanik kumanik commented Jul 15, 2023

What this PR does:
Adds an admin contact address flag to the ingester and uses it while printing error messages when hitting limits.

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

Checklist

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

Signed-off-by: Nikhil Kumar <nikhil.kumar.92219@gmail.com>
@kumanik
Copy link
Contributor Author

kumanik commented Jul 15, 2023

@friedrichg Have a look

I was thinking of adding tests for checking contact details at-

limiter := NewLimiter(limits, ring, util.ShardingStrategyDefault, true, 3, false)
actual := limiter.FormatError("user-1", errMaxSeriesPerUserLimitExceeded)
assert.EqualError(t, actual, "per-user series limit of 100 exceeded, please contact administrator to raise it (local limit: 0 global limit: 100 actual local limit: 100)")

Or should I create a completely new test?

Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

You don't need to add any additional tests, just keep the current tests working.

pkg/ingester/ingester.go Outdated Show resolved Hide resolved
pkg/ingester/ingester.go Outdated Show resolved Hide resolved
pkg/ingester/limiter.go Outdated Show resolved Hide resolved
pkg/ingester/limiter.go Outdated Show resolved Hide resolved
Signed-off-by: Nikhil Kumar <nikhil.kumar.92219@gmail.com>
@kumanik kumanik marked this pull request as ready for review July 16, 2023 05:16
Signed-off-by: Nikhil Kumar <nikhil.kumar.92219@gmail.com>
Signed-off-by: Nikhil Kumar <nikhil.kumar.92219@gmail.com>
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Good job @kumanik , the only thing missing is a FEATURE in the CHANGELOG.

Signed-off-by: Nikhil Kumar <nikhil.kumar.92219@gmail.com>
@kumanik kumanik force-pushed the limiter-admin-contact branch from 4c3061f to e17e98b Compare July 18, 2023 12:32
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Friedrich Gonzalez <friedrichg@gmail.com>
@friedrichg friedrichg changed the title Add ingester flag to add admin contact details in limiter error messages Add ingester flag to customize message in limiter error messages Jul 18, 2023
@kumanik
Copy link
Contributor Author

kumanik commented Jul 18, 2023

It is done I suppose

Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

Thank you!

@friedrichg friedrichg merged commit 025e723 into cortexproject:master Jul 18, 2023
@kumanik
Copy link
Contributor Author

kumanik commented Jul 18, 2023

No, thank you. Really glad I could contribute. If there is anything to work on I would like to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customize error message when hitting limits
2 participants