Skip to content

Conversation

zenador
Copy link
Contributor

@zenador zenador commented Jun 8, 2022

What this PR does

Following what we've done in #2009, this PR changes MessageWithLimitConfig to accept multiple flags and adds 'tenant-' to one error ID.

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 requested a review from pracucci June 8, 2022 12:57
@56quarters
Copy link
Contributor

I'm not sure it makes sense to add tenant- to each of the error IDs. The limits don't necessarily correspond to user-specific limits, they can also come from the limits section of the global configuration file: https://grafana.com/docs/mimir/latest/operators-guide/configuring/reference-configuration-parameters/#limits

@zenador zenador force-pushed the zenador/err-cat-tidy-up branch from 8826b3a to e9ae7d7 Compare June 8, 2022 13:54
@zenador zenador force-pushed the zenador/err-cat-tidy-up branch from e9ae7d7 to 758ef72 Compare June 8, 2022 13:55
@pracucci
Copy link
Collaborator

pracucci commented Jun 8, 2022

I'm not sure it makes sense to add tenant- to each of the error IDs. The limits don't necessarily correspond to user-specific limits, they can also come from the limits section of the global configuration file: https://grafana.com/docs/mimir/latest/operators-guide/configuring/reference-configuration-parameters/#limits

The tenant- prefix originated by a feedback I gave to Jeanette. After looking these changes, I want to step back. I agree with Nick that looks "too much". I think the only one where we need to clarify it is TooManyHAClusters, so I would add the tenant- prefix just to that. I think other errors have no need to disambiguate between per-tenant and per-instance (or per-something-else).

@zenador zenador force-pushed the zenador/err-cat-tidy-up branch from 526a393 to 0c67d4a Compare June 9, 2022 06:51
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, LGTM!

@pracucci pracucci merged commit 4076f4a into main Jun 9, 2022
@pracucci pracucci deleted the zenador/err-cat-tidy-up branch June 9, 2022 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants