-
Notifications
You must be signed in to change notification settings - Fork 569
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
Conversation
a6bad1a
to
1ec8491
Compare
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Co-authored-by: Oleg Zaytsev <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
83bc649
to
55f3944
Compare
Signed-off-by: Yuri Nikolic <[email protected]>
metric := mimirpb.FromLabelAdaptersToMetric(series).String() | ||
ellipsis := "" | ||
|
||
if utf8.RuneCountInString(metric) > 200 { | ||
ellipsis = "\u2026" | ||
} | ||
|
||
return []any{len(series), limit, metric, ellipsis} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Problems I'd like to see fixed in subsequent PRs:
|
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()
anddistributor.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
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]