mimir icon indicating copy to clipboard operation
mimir copied to clipboard

Add messageSizeLargerErrFmt error to errors catalog

Open zenador opened this issue 3 years ago • 3 comments
trafficstars

What this PR does

Add messageSizeLargerErrFmt error to errors catalog.

Which issue(s) this PR fixes or relates to

Fixes #2432

Checklist

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

zenador avatar Jul 19 '22 16:07 zenador

A few questions:

  1. the util package previously didn't import anything from within mimir, but now it has to as it needs globalerrors at least. it also needs to import the config flag name from distributors, but that would cause an import cycle, so i put that in globalerrors to minimise the imports. is this okay?
  2. the limit uses int most of the time, but there's a part of the code that uses int64 in mimir/pkg/util/push/otel.go. this is the same error right? for simplicity i kept it as int and casted int64 to int when displaying the error from that code, so the numbers might not make sense in that case. do we expect the number to be that big? should we be casting all the ints to int64 instead?
  3. this config isn't per-tenant right? there seems to be other new errors related to configs that are not per-tenant like DistributorMaxInflightPushRequestsBytes but they use MessageWithLimitConfig which calls the limit 'per-tenant'. should we add a boolean argument to that to decide whether we add the 'per-tenant' string or not?

zenador avatar Jul 19 '22 16:07 zenador

A few questions:

1. the util package previously didn't import anything from within mimir, but now it has to as it needs globalerrors at least. it also needs to import the config flag name from distributors, but that would cause an import cycle, so i put that in globalerrors to minimise the imports. is this okay?

I think a bit of copy/paste or removing the flag name from the error message is a better choice that moving a distributor specific flag outside the distributor package.

2. the limit uses int most of the time, but there's a part of the code that uses int64 in `mimir/pkg/util/push/otel.go`. this is the same error right? for simplicity i kept it as int and casted int64 to int when displaying the error from that code, so the numbers might not make sense in that case. do we expect the number to be that big? should we be casting all the ints to int64 instead?

I've noticed this too :sob:. Most of the things we use int or even int64 for, shouldn't ever be negative. I think your approach is fine for now.

3. this config isn't per-tenant right? there seems to be other new errors related to configs that are not per-tenant like DistributorMaxInflightPushRequestsBytes but they use MessageWithLimitConfig which calls the limit 'per-tenant'. should we add a boolean argument to that to decide whether we add the 'per-tenant' string or not?

I think having two different methods for the different types of limits would be a better choice than a bool that switches between two behaviors but that's personal preference.

56quarters avatar Jul 19 '22 18:07 56quarters

Thanks! Switched to copy/paste and 2 different methods

zenador avatar Jul 20 '22 17:07 zenador