Distributor: make OTLP endpoint return marshalled proto bytes as response body for errors #8227

ying-jeanne · 2024-05-30T20:09:23Z

What this PR does

Which issue(s) this PR fixes or relates to

Fixes #8185 , as discussed there are 2 possible ways to solve this:

pass a error handling function to handler, have 2 different implementation for otlp and normal push request
replicate the handler function for otlp and normal push request.

This PR is using the 2nd solution since it allows us to have more different logics for otlp endpoint in the future.

Checklist

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

jesusvazquez

Requesting small change other than that looks good to me.

I do think that this will solve the issue but we still have to work more on the errors. For example returning this:

the incoming push request has been rejected because its message size is larger than the allowed limit of 140 bytes (err-mimir-distributor-max-write-message-size). To adjust the related limit, configure -distributor.max-recv-msg-size, or contact your service administrator.

To the user explains the error in Mimir but it doesnt explain how to fix it in the collector. This is the bridge that we have to fix. But lets do that in future PRs not on this one 👍

pkg/distributor/push.go

ying-jeanne · 2024-05-31T15:57:42Z

Requesting small change other than that looks good to me.

I do think that this will solve the issue but we still have to work more on the errors. For example returning this:
the incoming push request has been rejected because its message size is larger than the allowed limit of 140 bytes (err-mimir-distributor-max-write-message-size). To adjust the related limit, configure -distributor.max-recv-msg-size, or contact your service administrator.
To the user explains the error in Mimir but it doesnt explain how to fix it in the collector. This is the bridge that we have to fix. But lets do that in future PRs not on this one 👍

Thanks @jesusvazquez for the review, I have this ticket to track all the remainings we needs in order to support otlp endpoint, attached another ticket with your description in it.

jesusvazquez

LGTM left a small note but is no blocker

jesusvazquez · 2024-05-31T17:48:59Z

pkg/distributor/push.go

@@ -169,7 +173,8 @@ func handler(
 			if resp, ok := httpgrpc.HTTPResponseFromError(err); ok {
 				code, msg = int(resp.Code), string(resp.Body)
 			} else {
-				code, msg = toHTTPStatus(ctx, err, limits), err.Error()
+				_, code = toGRPCHTTPStatus(ctx, err, limits)


You might want the grpc error code for the logline below.

this is not used by normal mimir handler, normally we shouldn't need the grpc status code, i would just leave it as right now.

johannaratliff

LGTM I'll defer to @jesusvazquez 's expertise on the primary functionality here being good

56quarters

OK to merge but there are a few changes I'd like to see made or ticketed as follow up work for next week.

pkg/distributor/push.go

56quarters · 2024-05-31T21:01:38Z

pkg/distributor/push_test.go

@@ -1178,7 +1186,7 @@ func TestHandler_ToHTTPStatus(t *testing.T) {
 			)
 			require.NoError(t, err)

-			status := toHTTPStatus(ctx, tc.err, limits)
+			_, status := toGRPCHTTPStatus(ctx, tc.err, limits)


We should be asserting the gRPC code too if we're using the same method to generate them and HTTP codes. Either that or we should split the error translation methods.

56quarters · 2024-05-31T21:02:47Z

pkg/distributor/push.go

 // http.StatusInternalServerError is returned.
-func toHTTPStatus(ctx context.Context, pushErr error, limits *validation.Overrides) int {
+func toGRPCHTTPStatus(ctx context.Context, pushErr error, limits *validation.Overrides) (codes.Code, int) {
 	if errors.Is(pushErr, context.DeadlineExceeded) {


nit: this conditional isn't really needed since this is the default return at the bottom of the function.

let me do it in a separate PR, since this is not related to the otlp change, don't want to break the code in push.

pstibrany · 2024-06-06T10:37:32Z

pkg/distributor/push.go

+// See doc https://opentelemetry.io/docs/specs/otlp/#failures-1
+func writeErrorToHTTPResponseBody(w http.ResponseWriter, httpCode int, grpcCode codes.Code, msg string, logger log.Logger) {
+	// writeResponseFailedError would be returned when writeErrorToHTTPResponseBody fails to write the error to the response body.
+	writeResponseFailedBody, _ := proto.Marshal(grpcstatus.New(codes.Internal, "write error to response failed").Proto())


We always marshal this message even if it's not used. We could actually just marshal it once and reuse for every call.

I've changed this in #8294.

pstibrany · 2024-06-06T10:38:35Z

pkg/distributor/push.go

+			if httpCode != 202 {
+				level.Error(logger).Log("msg", "push error", "err", err)
+			}
+			addHeaders(w, err, r, httpCode, retryCfg)


Adding headers here is too late -- headers were already written to client by this time. It means responses will not have correct Retry-After headers.

Fixing this in #8294.

pstibrany · 2024-06-06T10:43:41Z

pkg/distributor/push.go

+			if resp, ok := httpgrpc.HTTPResponseFromError(err); ok {
+				// here the error would always be nil, since it is already checked in httpgrpc.HTTPResponseFromError
+				s, _ := grpcutil.ErrorToStatus(err)
+				writeErrorToHTTPResponseBody(w, int(resp.Code), s.Code(), string(resp.Body), logger)


This means we're passing HTTP status code as grpc code. Can OTLP clients handle that? (Abusing http codes as grpc codes is something we do in our code, but I don't think we should send it to clients expecting grpc response)

…onse body for errors (grafana#8227) * OTLP: proto marshal response for otlp push request * add changelog * fix go.mod * Address comments

ying-jeanne force-pushed the marshall-proto-otlp-response branch 3 times, most recently from 2e0b692 to b368560 Compare May 30, 2024 20:50

ying-jeanne requested a review from jesusvazquez May 30, 2024 20:52

aknuds1 self-requested a review May 31, 2024 04:50

jesusvazquez requested changes May 31, 2024

View reviewed changes

pkg/distributor/push.go Outdated Show resolved Hide resolved

ying-jeanne mentioned this pull request May 31, 2024

otlp: improve mimir error message for otlp endpoint, in order to give better instruction on collector config #8240

Open

ying-jeanne force-pushed the marshall-proto-otlp-response branch from b368560 to 48174f0 Compare May 31, 2024 16:50

ying-jeanne added 2 commits May 31, 2024 11:02

OTLP: proto marshal response for otlp push request

1058ea3

add changelog

06bc06c

ying-jeanne force-pushed the marshall-proto-otlp-response branch 2 times, most recently from 6536385 to aeca033 Compare May 31, 2024 17:12

ying-jeanne requested a review from jesusvazquez May 31, 2024 17:22

fix go.mod

566819d

ying-jeanne force-pushed the marshall-proto-otlp-response branch from aeca033 to 566819d Compare May 31, 2024 17:29

ying-jeanne changed the title ~~[WIP] OTLP: proto marshal response for otlp push request~~ OTLP: proto marshal response for otlp push request May 31, 2024

ying-jeanne changed the title ~~OTLP: proto marshal response for otlp push request~~ Distributor: make OTLP endpoint return marshalled proto bytes as response body for 4xx/5xx errors May 31, 2024

ying-jeanne changed the title ~~Distributor: make OTLP endpoint return marshalled proto bytes as response body for 4xx/5xx errors~~ Distributor: make OTLP endpoint return marshalled proto bytes as response body for errors May 31, 2024

ying-jeanne marked this pull request as ready for review May 31, 2024 17:31

ying-jeanne requested review from grafanabot and a team as code owners May 31, 2024 17:31

jesusvazquez approved these changes May 31, 2024

View reviewed changes

johannaratliff approved these changes May 31, 2024

View reviewed changes

56quarters approved these changes May 31, 2024

View reviewed changes

Address comments

7f9d101

ying-jeanne enabled auto-merge (squash) May 31, 2024 23:49

ying-jeanne merged commit 8de824e into main May 31, 2024
29 checks passed

ying-jeanne deleted the marshall-proto-otlp-response branch May 31, 2024 23:58

pstibrany reviewed Jun 6, 2024

View reviewed changes

pstibrany mentioned this pull request Jun 6, 2024

Add insight=true to push handlers #8294

Merged

3 tasks

ying-jeanne mentioned this pull request Jun 10, 2024

Distributor: Map Non-Retryable Errors to Retryable in OTLP Endpoint #8324

Merged

4 tasks

pstibrany mentioned this pull request Jun 13, 2024

Fix reporting of HTTP error messages with binary body content #8363

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distributor: make OTLP endpoint return marshalled proto bytes as response body for errors #8227

Distributor: make OTLP endpoint return marshalled proto bytes as response body for errors #8227

ying-jeanne commented May 30, 2024 •

edited

Loading

jesusvazquez left a comment

ying-jeanne commented May 31, 2024 •

edited

Loading

jesusvazquez left a comment

jesusvazquez May 31, 2024

ying-jeanne May 31, 2024 •

edited

Loading

johannaratliff left a comment

56quarters left a comment

56quarters May 31, 2024

56quarters May 31, 2024 •

edited

Loading

ying-jeanne May 31, 2024

pstibrany Jun 6, 2024 •

edited

Loading

pstibrany Jun 6, 2024

pstibrany Jun 6, 2024

pstibrany Jun 6, 2024

pstibrany Jun 6, 2024

Distributor: make OTLP endpoint return marshalled proto bytes as response body for errors #8227

Distributor: make OTLP endpoint return marshalled proto bytes as response body for errors #8227

Conversation

ying-jeanne commented May 30, 2024 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

jesusvazquez left a comment

Choose a reason for hiding this comment

ying-jeanne commented May 31, 2024 • edited Loading

jesusvazquez left a comment

Choose a reason for hiding this comment

jesusvazquez May 31, 2024

Choose a reason for hiding this comment

ying-jeanne May 31, 2024 • edited Loading

Choose a reason for hiding this comment

johannaratliff left a comment

Choose a reason for hiding this comment

56quarters left a comment

Choose a reason for hiding this comment

56quarters May 31, 2024

Choose a reason for hiding this comment

56quarters May 31, 2024 • edited Loading

Choose a reason for hiding this comment

ying-jeanne May 31, 2024

Choose a reason for hiding this comment

pstibrany Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

pstibrany Jun 6, 2024

Choose a reason for hiding this comment

pstibrany Jun 6, 2024

Choose a reason for hiding this comment

pstibrany Jun 6, 2024

Choose a reason for hiding this comment

pstibrany Jun 6, 2024

Choose a reason for hiding this comment

ying-jeanne commented May 30, 2024 •

edited

Loading

ying-jeanne commented May 31, 2024 •

edited

Loading

ying-jeanne May 31, 2024 •

edited

Loading

56quarters May 31, 2024 •

edited

Loading

pstibrany Jun 6, 2024 •

edited

Loading