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.

Sign up for GitHub

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

Jump to bottom

Add validation.RateLimited and TooManyHAClusters to errors catalogue #2009

Merged
merged 7 commits into from
Jun 8, 2022
Merged

Add validation.RateLimited and TooManyHAClusters to errors catalogue #2009

merged 7 commits into from
Jun 8, 2022

Conversation

zenador
Copy link
Contributor

@zenador zenador commented Jun 3, 2022 •
edited
Loading

What this PR does

Following what we've done in #1939, this PR adds "rate_limited" and "too_many_ha_clusters" to the errors catalogue.

Which issue(s) this PR fixes or relates to

N/A

Checklist

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

@zenador zenador force-pushed the zenador/err-cat-valid-rate-limited branch 2 times, most recently from 0fae871 to fb00149 Compare June 3, 2022 08:24
@zenador zenador changed the title Add validation.RateLimited to errors catalogue Add validation.RateLimited and TooManyHAClusters to errors catalogue Jun 3, 2022
@zenador zenador requested a review from pracucci June 3, 2022 10:56
@zenador zenador force-pushed the zenador/err-cat-valid-rate-limited branch 2 times, most recently from fc6fc62 to 9501ea2 Compare June 3, 2022 11:07
pracucci
pracucci reviewed Jun 3, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci 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! The direction is good. I left few comments. Looking forward for the runbooks. Thanks!

pkg/distributor/ha_tracker.go Outdated Show resolved Hide resolved
pkg/distributor/distributor.go Outdated Show resolved Hide resolved
pkg/distributor/distributor.go Outdated Show resolved Hide resolved
pkg/util/globalerror/errors.go Outdated
@@ -57,3 +60,7 @@ func (id ID) Message(msg string) string {
func (id ID) MessageWithLimitConfig(flag, msg string) string {
return fmt.Sprintf("%s (%s%s). You can adjust the related per-tenant limit by configuring -%s, or by contacting your service administrator.", msg, errPrefix, id, flag)
}

func (id ID) MessageWith2LimitConfig(flag, flag2, msg string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a big fan of "With2" version. A cleaner alternative, from the caller perspective, may be reversing MessageWithLimitConfig() arguments order and making flags a variable argument, like this:

func (id ID) MessageWithLimitConfig(msg string, flags ...string) string {

Then, you could dynamically generate the error message to include all flags.

I would do this change in a separate PR, to keep the change isolated from this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the With2 to what you suggested, but kept flag as a separate string to ensure there's a compile error if there isn't at least 1 flag. After this is merged, I can open a separate PR to change all previous uses of the old one to use the new one and then remove the old one. Is this okay?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The new function version LGTM!

I can open a separate PR to change all previous uses of the old one to use the new one and then remove the old one.

LGTM

@zenador zenador force-pushed the zenador/err-cat-valid-rate-limited branch from 9501ea2 to b39c533 Compare June 6, 2022 07:15
pracucci
pracucci reviewed Jun 7, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci 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! I left few more comments. Definitely heading to the right direction. I would like to see runbooks now :)

pkg/util/globalerror/errors.go Show resolved Hide resolved
pkg/util/globalerror/errors.go Outdated
@@ -57,3 +60,7 @@ func (id ID) Message(msg string) string {
func (id ID) MessageWithLimitConfig(flag, msg string) string {
return fmt.Sprintf("%s (%s%s). You can adjust the related per-tenant limit by configuring -%s, or by contacting your service administrator.", msg, errPrefix, id, flag)
}

func (id ID) MessageWith2LimitConfig(flag, flag2, msg string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The new function version LGTM!

I can open a separate PR to change all previous uses of the old one to use the new one and then remove the old one.

LGTM

pkg/distributor/ha_tracker.go Outdated Show resolved Hide resolved
pkg/util/validation/errors.go Outdated Show resolved Hide resolved
pkg/util/validation/errors.go Outdated Show resolved Hide resolved
@pracucci pracucci requested a review from osg-grafana June 8, 2022 07:52
pracucci
pracucci reviewed Jun 8, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci 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! We're super close to the finish line!

docs/sources/operators-guide/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
docs/sources/operators-guide/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
docs/sources/operators-guide/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
docs/sources/operators-guide/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
docs/sources/operators-guide/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
docs/sources/operators-guide/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
docs/sources/operators-guide/mimir-runbooks/_index.md Outdated Show resolved Hide resolved
pkg/util/globalerror/errors.go Outdated Show resolved Hide resolved
@zenador @pracucci
Apply suggestions from code review
c2583f7 
Co-authored-by: Marco Pracucci <marco@pracucci.com>
osg-grafana
osg-grafana approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@osg-grafana osg-grafana left a comment

Choose a reason for hiding this comment

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

Very nice, thank you! :)

@zenador zenador force-pushed the zenador/err-cat-valid-rate-limited branch from 3c093e3 to fd95e66 Compare June 8, 2022 09:23
pracucci
pracucci approved these changes Jun 8, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM!

@pracucci pracucci merged commit eb812fd into main Jun 8, 2022
@pracucci pracucci deleted the zenador/err-cat-valid-rate-limited branch June 8, 2022 12:48
@zenador zenador mentioned this pull request Jun 8, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pracucci pracucci pracucci approved these changes

@osg-grafana osg-grafana osg-grafana approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zenador @pracucci @osg-grafana