Ingester write path: refactor error creation #6324

duricanikolic · 2023-10-10T19:18:31Z

What this PR does

Similarly to what has been done on distributor site (PR), this PR introduces the following error types, representing the problems on the ingester’s write path that need some special handling:

unavailableError,
tsdbUnavailableError,
sampleError,
exemplarError,
tsdbIngestExemplarError,
instanceLimitReachedError,
perUserSeriesLimitReachedError,
perUserMetadataLimitReachedError,
perMetricSeriesLimitReachedError,
perMetricMetadataLimitReachedError.
They all belong to the ingester package.

Moreover, this PR refactors the ingester's write path in such a way that all mappings between errors created by the ingester and the HTTP status or gRPC status codes have been moved to the topmost possible level, i.e., to ingester.Push().

The semantics of the ingester's write path has not been changed, i.e., the newly introduced errors are mapped to the same HTTP or gRPC 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

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

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

pkg/ingester/errors.go

pkg/ingester/ingester.go

pkg/mimir/grpc_push_check.go

pkg/ingester/errors.go

pstibrany · 2023-10-12T06:44:26Z

pkg/ingester/errors.go

+		message: fmt.Sprintf(
+			"%s. The affected sample has timestamp %s and is from series %s",
+			prefixMsg,
 			timestamp.Time().UTC().Format(time.RFC3339Nano),
 			mimirpb.FromLabelAdaptersToLabels(labels).String(),
 		),
-	)
+	}


We should do this formatting in Error() call, not in "new" call. It's wasteful if message is never used.

Thank you, I have re-organized the code accordingly.

This is not quite complete. There's still unnecessary string formatting done when calling globalerror.ID.Message(...) too early. This should also be delayed to Error calls.

Perhaps keeping globalerror.ID in errors could also help with deciding which sampling wrapper to use? 🤔 Then we can move sampling out of calls to updateFirstPartial, and let that function only work ith ingesterError.

I wouldn't touch error sampling now. This is a separate problem, and it would additionally complicate this PR and this task, which is basically to remove httpgrpc.Errorf() calls from ingester and distributor.

This is not quite complete. There's still unnecessary string formatting done when calling globalerror.ID.Message(...) too early. This should also be delayed to Error calls.

I was asked by @colega to do it this way here. If your opinion about formatting is strong, I will put the globalID and message back to the error as fields, and format them during Error().

Yes, I would suggest to put it back. While I see Oleg's point, creating additional strings in "new" call is premature, and can be avoided by putting formatting into Error call. If Error call is actually never called, then we just saved formatting completely.

I will fix this immediately.

pkg/ingester/errors.go

pkg/ingester/ingester.go

pstibrany · 2023-10-12T07:07:38Z

pkg/ingester/ingester.go

-		code := http.StatusBadRequest
-		errWithUser := wrapOrAnnotateWithUser(firstPartialErr, userID)
-		return newErrorWithHTTPStatus(errWithUser, code)
+		return wrapOrAnnotateWithUser(firstPartialErr, userID)


Previously we would always return HTTP Status code 400 for ANY error here, even if it wasn't "safe to wrap". After this change, any error that's "not safe to wrap" will be translated to 500. Is that correct?

Hi @pstibrany,
Not exactly. All the errors returned here were safe, because they are the soft errors found by [pushSamplesToAppender()](https://github.com/grafana/mimir/blob/main/pkg/ingester/ingester.go#L874). These errors are always safe, and after the changes from this PR they are all defined in errors.go. This PR keeps the same semantics as the current master.

All the errors returned here were safe,

These errors are always safe

I don't think this is true. If it is, let's use type system to make sure. But I think there are possible errors that can be returned by pushSamplesToAppender that are not wrapped into our "safe" wrappers.

In the current implementation, updateFirstPartial (here) is responsible for assigning a value to variable firstPartialErr, that is then retrurned with the 400 HTTP status code (here).

The updateFirstPartial is passed as a parameter to pushSamplesToAppender(), and is called different times inside its implementation (e.g., here, here, etc...)

Related to this PR, all the calls to updateFirstPartial in pushSamplesToAppend() return what used to be called a safeToWrap error, and what now became an ingesterError, i.e., an error that wrapOrAnnotateWithUserId() would actually wrap with userID. Moreover, errorType() of all these ingesterErrors is ingesterErrorType.badData, i.e., the type of errors that is mapped to 400.

In the current implementation, updateFirstPartial (here) is responsible for assigning a value to variable firstPartialErr, that is then retrurned with the 400 HTTP status code (here).

The updateFirstPartial is passed as a parameter to pushSamplesToAppender(), and is called different times inside its implementation (e.g., here, here, etc...)

I see, and I think you're right. It would be great if we could codify that, for example by defining updateFirstPartial so that it only accepts ingesterErrors.

I don't understand which problem with SampledError wrapping you are referring to. Anyway, I don't think it is a good idea to pass a *log.Sampler to updateFirstPartial because there are some errors that don't need to be sampled, for example this.

I see, and I think you're right. It would be great if we could codify that, for example by defining updateFirstPartial so that it only accepts ingesterErrors.

I can try to do it, but what do we actually want to achieve? If the error returned by updateFirstPartial()'s parameter is something we consider a silent error, we go on, but if it is another type of error we want to fail immediately?
WDYT if we leave it like it is now (it doesn't change the previous behavior) and to improve this in a separate PR that will be dedicated only to that problem? Otherwise this PR will become very big.

but what do we actually want to achieve? If the error returned by updateFirstPartial()'s parameter is something we consider a silent error, we go on, but if it is another type of error we want to fail immediately?

What we want to achieve is to make sure that we don't "leak" any other types of errors via firstPartialErr (that can't be safely wrapped). As stated previously, even if that was the case before this PR, we would still end up returning 400, but after this PR that's not the case anymore -- instead call to wrapOrAnnotateWithUser with unwrappable error will "destroy" underlying error type and our mapping will not convert it to anything.

While we can verify that only "ingester errors" are returned firstPartialErr today by doing a code review, it would be better to actually use the type system to avoid any future "leaks" of non-ingester errors here.

I agree about doing this in separate PR.

I don't understand which problem with SampledError wrapping you are referring to.

I've tried to change updateFirstPartial to only accept ingesterError, but that wasn't working for errors wrapped by SampledError.

Anyway, I don't think it is a good idea to pass a *log.Sampler to updateFirstPartial because there are some errors that don't need to be sampled, for example this.

I don't think that's a problem -- we would pass nil in such case.

I don't think that's a problem -- we would pass nil in such case.

I have updated the updateFirstPartial signature accordingly. I will do additional changes in a separate PR.

pkg/ingester/ingester.go

pstibrany

Thank you, nice work! I've left some non-blocking comments.

pstibrany · 2023-10-12T15:18:41Z

pkg/ingester/errors.go

+
+// exemplarError is an ingesterError indicating a problem with an exemplar.
+type exemplarError struct {
+	originalErr    error


It looks somewhat strange that exemplarError has this optional originalErr. That almost looks like a separate type to me.

I have added another type, tsdbIngestExemplarError to handle that situation.

pstibrany · 2023-10-12T15:22:16Z

pkg/ingester/ingester.go

 			if firstPartialErr == nil {
 				firstPartialErr = errFn()
+				if sampler != nil {


This was suggested in combination of making errFn return only ingesterError, and changing firstPartialErr to that type as well. It may make more sense then, but we can keep it for now.

pstibrany · 2023-10-12T15:27:24Z

pkg/ingester/ingester_test.go

+		require.True(t, ok)
+		require.Equal(t, http.StatusServiceUnavailable, int(stat.Code()))
+		require.Equal(t, tooBusyErrorMsg, stat.Message())


Nit: These checks repeat at different places. We could extract them into a checkGrpcStatusError method.

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

duricanikolic · 2023-10-12T17:04:29Z

I have implemented requested findings and merged the PR.

pracucci · 2023-10-18T13:52:22Z

pkg/ingester/ingester.go

+						if err == nil {
+							return nil
+						}


Why is this if necessary?

duricanikolic self-assigned this Oct 10, 2023

duricanikolic force-pushed the yuri/handling-ingester-errors branch 2 times, most recently from 4a84994 to c6c126f Compare October 10, 2023 20:20

duricanikolic marked this pull request as ready for review October 10, 2023 20:22

duricanikolic requested a review from a team as a code owner October 10, 2023 20:22

duricanikolic requested review from pstibrany, colega and pracucci October 10, 2023 20:22

duricanikolic force-pushed the yuri/handling-ingester-errors branch from c6c126f to 176604b Compare October 11, 2023 13:44

colega reviewed Oct 11, 2023

View reviewed changes

duricanikolic requested a review from colega October 11, 2023 15:42

colega reviewed Oct 11, 2023

View reviewed changes

pkg/ingester/errors.go Outdated Show resolved Hide resolved

duricanikolic force-pushed the yuri/handling-ingester-errors branch 2 times, most recently from 7e93c86 to 5d5e118 Compare October 11, 2023 15:48

colega reviewed Oct 11, 2023

View reviewed changes

pkg/ingester/errors.go Outdated Show resolved Hide resolved

pstibrany reviewed Oct 12, 2023

View reviewed changes

duricanikolic requested a review from pstibrany October 12, 2023 09:33

duricanikolic force-pushed the yuri/handling-ingester-errors branch from 0707e49 to a10c540 Compare October 12, 2023 10:01

pstibrany approved these changes Oct 12, 2023

View reviewed changes

duricanikolic added 9 commits October 12, 2023 18:17

Refactoring ingester's errors - Part 1

185d6bd

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Refactoring ingester's errors - Part 2

2acf283

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Refactoring ingester's errors - Part 3

c09d6e5

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Refactoring ingester's errors - Part 4

d43a273

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Fixing review findings

42670c7

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Fixing review findings 2

31a9136

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Fixing review findings 3

f711849

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Fixing review findings 4

1380c89

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Adding tsdbIngestExemplarError

43680a3

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

duricanikolic force-pushed the yuri/handling-ingester-errors branch from bba50dc to 43680a3 Compare October 12, 2023 16:24

duricanikolic merged commit 847212d into main Oct 12, 2023
28 checks passed

duricanikolic deleted the yuri/handling-ingester-errors branch October 12, 2023 17:04

pracucci reviewed Oct 18, 2023

View reviewed changes

pkg/ingester/ingester.go

Comment on lines +1229 to +1231

if err == nil {

return nil

}

Copy link

Collaborator

pracucci Oct 18, 2023

Choose a reason for hiding this comment

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

Why is this if necessary?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingester write path: refactor error creation #6324

Ingester write path: refactor error creation #6324

duricanikolic commented Oct 10, 2023 •

edited

Loading

pstibrany Oct 12, 2023

duricanikolic Oct 12, 2023

pstibrany Oct 12, 2023

pstibrany Oct 12, 2023

duricanikolic Oct 12, 2023

duricanikolic Oct 12, 2023

pstibrany Oct 12, 2023

duricanikolic Oct 12, 2023

pstibrany Oct 12, 2023

duricanikolic Oct 12, 2023

pstibrany Oct 12, 2023

duricanikolic Oct 12, 2023

pstibrany Oct 12, 2023

duricanikolic Oct 12, 2023

duricanikolic Oct 12, 2023 •

edited

Loading

pstibrany Oct 12, 2023

pstibrany Oct 12, 2023

duricanikolic Oct 12, 2023

pstibrany left a comment

pstibrany Oct 12, 2023

duricanikolic Oct 12, 2023

pstibrany Oct 12, 2023

pstibrany Oct 12, 2023

duricanikolic commented Oct 12, 2023

pracucci Oct 18, 2023

Ingester write path: refactor error creation #6324

Ingester write path: refactor error creation #6324

Conversation

duricanikolic commented Oct 10, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Which issue(s) this PR fixes or relates to

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duricanikolic Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duricanikolic commented Oct 12, 2023

Choose a reason for hiding this comment

duricanikolic commented Oct 10, 2023 •

edited

Loading

duricanikolic Oct 12, 2023 •

edited

Loading