From ff622078476132518a52e8e67ce442a1ccab71e6 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Thu, 4 Apr 2024 13:14:06 -0600 Subject: [PATCH] Refactor http error handling --- .../otlpreceiver/internal/errors/errors.go | 29 +++++-- .../internal/errors/errors_test.go | 83 +++++++++++++++++-- receiver/otlpreceiver/otlphttp.go | 22 ++--- 3 files changed, 106 insertions(+), 28 deletions(-) diff --git a/receiver/otlpreceiver/internal/errors/errors.go b/receiver/otlpreceiver/internal/errors/errors.go index ae2127bf9ba..5035682950e 100644 --- a/receiver/otlpreceiver/internal/errors/errors.go +++ b/receiver/otlpreceiver/internal/errors/errors.go @@ -27,11 +27,7 @@ func GetStatusFromError(err error) error { return s.Err() } -func GetHTTPStatusCodeFromStatus(err error) int { - s, ok := status.FromError(err) - if !ok { - return http.StatusInternalServerError - } +func GetHTTPStatusCodeFromStatus(s *status.Status) int { // See https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#failures // to see if a code is retryable. // See https://github.com/open-telemetry/opentelemetry-proto/blob/main/docs/specification.md#failures-1 @@ -48,3 +44,26 @@ func GetHTTPStatusCodeFromStatus(err error) int { return http.StatusInternalServerError } } + +func NewStatusFromMsgAndHTTPCode(errMsg string, statusCode int) *status.Status { + var c codes.Code + // Mapping based on https://github.com/grpc/grpc/blob/master/doc/http-grpc-status-mapping.md + // 429 mapping to ResourceExhausted and 400 mapping to StatusBadRequest are exceptions. + switch statusCode { + case http.StatusBadRequest: + c = codes.InvalidArgument + case http.StatusUnauthorized: + c = codes.Unauthenticated + case http.StatusForbidden: + c = codes.PermissionDenied + case http.StatusNotFound: + c = codes.Unimplemented + case http.StatusTooManyRequests: + c = codes.ResourceExhausted + case http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout: + c = codes.Unavailable + default: + c = codes.Unknown + } + return status.New(c, errMsg) +} diff --git a/receiver/otlpreceiver/internal/errors/errors_test.go b/receiver/otlpreceiver/internal/errors/errors_test.go index 72db243353f..e27b364304a 100644 --- a/receiver/otlpreceiver/internal/errors/errors_test.go +++ b/receiver/otlpreceiver/internal/errors/errors_test.go @@ -48,27 +48,22 @@ func Test_GetStatusFromError(t *testing.T) { func Test_GetHTTPStatusCodeFromStatus(t *testing.T) { tests := []struct { name string - input error + input *status.Status expected int }{ - { - name: "Not a Status", - input: fmt.Errorf("not a status error"), - expected: http.StatusInternalServerError, - }, { name: "Retryable Status", - input: status.New(codes.Unavailable, "test").Err(), + input: status.New(codes.Unavailable, "test"), expected: http.StatusServiceUnavailable, }, { name: "Non-retryable Status", - input: status.New(codes.InvalidArgument, "test").Err(), + input: status.New(codes.InvalidArgument, "test"), expected: http.StatusInternalServerError, }, { name: "Specifically 429", - input: status.New(codes.ResourceExhausted, "test").Err(), + input: status.New(codes.ResourceExhausted, "test"), expected: http.StatusTooManyRequests, }, } @@ -79,3 +74,73 @@ func Test_GetHTTPStatusCodeFromStatus(t *testing.T) { }) } } + +func Test_ErrorMsgAndHTTPCodeToStatus(t *testing.T) { + tests := []struct { + name string + errMsg string + statusCode int + expected *status.Status + }{ + { + name: "Bad Request", + errMsg: "test", + statusCode: http.StatusBadRequest, + expected: status.New(codes.InvalidArgument, "test"), + }, + { + name: "Unauthorized", + errMsg: "test", + statusCode: http.StatusUnauthorized, + expected: status.New(codes.Unauthenticated, "test"), + }, + { + name: "Forbidden", + errMsg: "test", + statusCode: http.StatusForbidden, + expected: status.New(codes.PermissionDenied, "test"), + }, + { + name: "Not Found", + errMsg: "test", + statusCode: http.StatusNotFound, + expected: status.New(codes.Unimplemented, "test"), + }, + { + name: "Too Many Requests", + errMsg: "test", + statusCode: http.StatusTooManyRequests, + expected: status.New(codes.ResourceExhausted, "test"), + }, + { + name: "Bad Gateway", + errMsg: "test", + statusCode: http.StatusBadGateway, + expected: status.New(codes.Unavailable, "test"), + }, + { + name: "Service Unavailable", + errMsg: "test", + statusCode: http.StatusServiceUnavailable, + expected: status.New(codes.Unavailable, "test"), + }, + { + name: "Gateway Timeout", + errMsg: "test", + statusCode: http.StatusGatewayTimeout, + expected: status.New(codes.Unavailable, "test"), + }, + { + name: "Unsupported Media Type", + errMsg: "test", + statusCode: http.StatusUnsupportedMediaType, + expected: status.New(codes.Unknown, "test"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := NewStatusFromMsgAndHTTPCode(tt.errMsg, tt.statusCode) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/receiver/otlpreceiver/otlphttp.go b/receiver/otlpreceiver/otlphttp.go index ae60d8d37d2..431a7ba0ae0 100644 --- a/receiver/otlpreceiver/otlphttp.go +++ b/receiver/otlpreceiver/otlphttp.go @@ -10,7 +10,6 @@ import ( "net/http" spb "google.golang.org/genproto/googleapis/rpc/status" - "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "go.opentelemetry.io/collector/receiver/otlpreceiver/internal/errors" @@ -43,7 +42,7 @@ func handleTraces(resp http.ResponseWriter, req *http.Request, tracesReceiver *t otlpResp, err := tracesReceiver.Export(req.Context(), otlpReq) if err != nil { - writeError(resp, enc, err, errors.GetHTTPStatusCodeFromStatus(err)) + writeError(resp, enc, err, http.StatusInternalServerError) return } @@ -74,7 +73,7 @@ func handleMetrics(resp http.ResponseWriter, req *http.Request, metricsReceiver otlpResp, err := metricsReceiver.Export(req.Context(), otlpReq) if err != nil { - writeError(resp, enc, err, errors.GetHTTPStatusCodeFromStatus(err)) + writeError(resp, enc, err, http.StatusInternalServerError) return } @@ -105,7 +104,7 @@ func handleLogs(resp http.ResponseWriter, req *http.Request, logsReceiver *logs. otlpResp, err := logsReceiver.Export(req.Context(), otlpReq) if err != nil { - writeError(resp, enc, err, errors.GetHTTPStatusCodeFromStatus(err)) + writeError(resp, enc, err, http.StatusInternalServerError) return } @@ -150,8 +149,10 @@ func readAndCloseBody(resp http.ResponseWriter, req *http.Request, enc encoder) // writeError encodes the HTTP error inside a rpc.Status message as required by the OTLP protocol. func writeError(w http.ResponseWriter, encoder encoder, err error, statusCode int) { s, ok := status.FromError(err) - if !ok { - s = errorMsgToStatus(err.Error(), statusCode) + if ok { + statusCode = errors.GetHTTPStatusCodeFromStatus(s) + } else { + s = errors.NewStatusFromMsgAndHTTPCode(err.Error(), statusCode) } writeStatusResponse(w, encoder, statusCode, s.Proto()) } @@ -159,7 +160,7 @@ func writeError(w http.ResponseWriter, encoder encoder, err error, statusCode in // errorHandler encodes the HTTP error message inside a rpc.Status message as required // by the OTLP protocol. func errorHandler(w http.ResponseWriter, r *http.Request, errMsg string, statusCode int) { - s := errorMsgToStatus(errMsg, statusCode) + s := errors.NewStatusFromMsgAndHTTPCode(errMsg, statusCode) switch getMimeTypeFromContentType(r.Header.Get("Content-Type")) { case pbContentType: writeStatusResponse(w, pbEncoder, statusCode, s.Proto()) @@ -188,13 +189,6 @@ func writeResponse(w http.ResponseWriter, contentType string, statusCode int, ms _, _ = w.Write(msg) } -func errorMsgToStatus(errMsg string, statusCode int) *status.Status { - if statusCode == http.StatusBadRequest { - return status.New(codes.InvalidArgument, errMsg) - } - return status.New(codes.Unknown, errMsg) -} - func getMimeTypeFromContentType(contentType string) string { mediatype, _, err := mime.ParseMediaType(contentType) if err != nil {