[receiver/otlp] Refactor http error handling

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)
+}

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)
+		})
+	}
+}

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,16 +149,18 @@ 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())
 }
 
 // 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 {