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

Distributor write path: put httpgrpc.Errorf() calls at the topmost level #6191

Merged
merged 24 commits into from
Oct 6, 2023

Conversation

duricanikolic
Copy link
Contributor

@duricanikolic duricanikolic commented Oct 2, 2023

What this PR does

This PR introduces the following error types, reppresenting the problems on the distributor’s write path that need some special handling:

  • ReplicasNotMatch,
  • TooManyClusters,
  • ValidationError,
  • IngestionRateLimited
  • RequestRateLimited.
    They all belong to the newly introduced package distributorerror.

Moreover, this PR refactors the distributor's write path in such a way that all the calls to httpgrpc.Errorf() have been moved to the topmost possible level,i.e., to:

  • push.handler() and
  • distributor.Push().
    The semantics of the distributor's write path has not been changed, i.e., the newly introduced errors are mapped to the same HTTP status codes their corresponding problems used to be assigned before this PR.

This refactoring is just a preliminary change needed for a bigger refactoring and improving of intester's and distributor's write path error handling.

Which issue(s) this PR fixes or relates to

Related to #6008

Checklist

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

@duricanikolic duricanikolic requested a review from a team as a code owner October 2, 2023 16:14
@duricanikolic duricanikolic self-assigned this Oct 2, 2023
@duricanikolic duricanikolic force-pushed the yuri/handling-errors-new branch 6 times, most recently from a6bad1a to 1ec8491 Compare October 2, 2023 17:22
pkg/util/push/push.go Outdated Show resolved Hide resolved
pkg/util/push/push.go Outdated Show resolved Hide resolved
pkg/util/push/push.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/distributor/distributor.go Outdated Show resolved Hide resolved
pkg/distributor/distributor.go Outdated Show resolved Hide resolved
pkg/distributor/distributorerror/error.go Outdated Show resolved Hide resolved
pkg/distributor/distributorerror/error.go Outdated Show resolved Hide resolved
pkg/distributor/distributorerror/error.go Outdated Show resolved Hide resolved
pkg/util/push/push.go Outdated Show resolved Hide resolved
pkg/util/push/push.go Outdated Show resolved Hide resolved
pkg/util/push/push.go Outdated Show resolved Hide resolved
duricanikolic and others added 19 commits October 5, 2023 11:18
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Co-authored-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
pkg/distributor/distributor.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/distributor/distributorerror/errors.go Outdated Show resolved Hide resolved
pkg/distributor/distributorerror/errors.go Outdated Show resolved Hide resolved
pkg/distributor/distributorerror/errors.go Outdated Show resolved Hide resolved
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
Comment on lines +536 to +543
metric := mimirpb.FromLabelAdaptersToMetric(series).String()
ellipsis := ""

if utf8.RuneCountInString(metric) > 200 {
ellipsis = "\u2026"
}

return []any{len(series), limit, metric, ellipsis}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we shorten the metric if it's too long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same implementation we are having on master right now (here)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hum, okay, I'd say that's a bug.

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

It would be good to see the final result where this is heading, because I fail to see how this PR improves anything really.

@pstibrany
Copy link
Member

Problems I'd like to see fixed in subsequent PRs:

  • dependency from util/push to distributorerror. Push handler is currently generic and it should not call error translation from distributor (sub)package directly. To solve this we could pass "error translation function" to push handler. Later when ingester errors are supported too, this error translation function should handle both kinds of errors.

  • distributorerror package -- all errors introduced here belong to distributor package. Furthermore they don't need to be exported. The only thing that needs to be exported is "distributor's error translation function".

  • design of error types in distributorerror package. They should not be just wrappers around 'error' or 'format`.

@pstibrany pstibrany merged commit 86ef0a0 into main Oct 6, 2023
28 checks passed
@pstibrany pstibrany deleted the yuri/handling-errors-new branch October 6, 2023 06:48
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.

4 participants