fix: do not ignore the error as it might be useful for upstream middl…

…ewares. #2209 (#3656)

* fix do not ignore the error as it might be useful for upstream middlewares (#2209)

* Update CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* fix import-shadowing

* remove redundant assertions and use `assert.AnError`

* Remove stale test

* Remove unnecessary options

---------

Co-authored-by: Robert Pająk <pellared@hotmail.com>
Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

CHANGELOG.md

@@ -20,6 +20,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Fixed
 
+- The error received by `otelecho` middleware is then passed back to upstream middleware instead of being swallowed. (#3656)
 - Prevent taking from reservoir in AWS XRay Remote Sampler when there is zero capacity in `go.opentelemetry.io/contrib/samplers/aws/xray`. (#3684)
 
 ## [1.16.0-rc.2/0.41.0-rc.2/0.10.0-rc.2] - 2023-03-23

instrumentation/github.com/labstack/echo/otelecho/echo.go

@@ -102,7 +102,7 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc {
 				span.SetAttributes(semconv.HTTPStatusCode(status))
 			}
 
-			return nil
+			return err
 		}
 	}
 }

instrumentation/github.com/labstack/echo/otelecho/echo_test.go

@@ -18,7 +18,6 @@ package otelecho
 
 import (
 	"context"
-	"errors"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -33,23 +32,6 @@ import (
 	b3prop "go.opentelemetry.io/contrib/propagators/b3"
 )
 
-func TestErrorOnlyHandledOnce(t *testing.T) {
-	router := echo.New()
-	timesHandlingError := 0
-	router.HTTPErrorHandler = func(e error, c echo.Context) {
-		timesHandlingError++
-	}
-	router.Use(Middleware("test-service"))
-	router.GET("/", func(c echo.Context) error {
-		return errors.New("mock error")
-	})
-	r := httptest.NewRequest(http.MethodGet, "/", nil)
-	w := httptest.NewRecorder()
-	router.ServeHTTP(w, r)
-
-	assert.Equal(t, 1, timesHandlingError)
-}
-
 func TestGetSpanNotInstrumented(t *testing.T) {
 	router := echo.New()
 	router.GET("/ping", func(c echo.Context) error {

instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go

@@ -137,3 +137,79 @@ func TestError(t *testing.T) {
 	// server errors set the status
 	assert.Equal(t, codes.Error, span.Status().Code)
 }
+
+func TestStatusError(t *testing.T) {
+	for _, tc := range []struct {
+		name       string
+		echoError  string
+		statusCode int
+		spanCode   codes.Code
+		handler    func(c echo.Context) error
+	}{
+		{
+			name:       "StandardError",
+			echoError:  "oh no",
+			statusCode: http.StatusInternalServerError,
+			spanCode:   codes.Error,
+			handler: func(c echo.Context) error {
+				return errors.New("oh no")
+			},
+		},
+		{
+			name:       "EchoHTTPServerError",
+			echoError:  "code=500, message=my error message",
+			statusCode: http.StatusInternalServerError,
+			spanCode:   codes.Error,
+			handler: func(c echo.Context) error {
+				return echo.NewHTTPError(http.StatusInternalServerError, "my error message")
+			},
+		},
+		{
+			name:       "EchoHTTPClientError",
+			echoError:  "code=400, message=my error message",
+			statusCode: http.StatusBadRequest,
+			spanCode:   codes.Unset,
+			handler: func(c echo.Context) error {
+				return echo.NewHTTPError(http.StatusBadRequest, "my error message")
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			sr := tracetest.NewSpanRecorder()
+			provider := trace.NewTracerProvider(trace.WithSpanProcessor(sr))
+
+			router := echo.New()
+			router.Use(otelecho.Middleware("foobar", otelecho.WithTracerProvider(provider)))
+			router.GET("/err", tc.handler)
+			r := httptest.NewRequest("GET", "/err", nil)
+			w := httptest.NewRecorder()
+			router.ServeHTTP(w, r)
+
+			spans := sr.Ended()
+			require.Len(t, spans, 1)
+			span := spans[0]
+			assert.Equal(t, "/err", span.Name())
+			assert.Equal(t, tc.spanCode, span.Status().Code)
+
+			attrs := span.Attributes()
+			assert.Contains(t, attrs, attribute.String("net.host.name", "foobar"))
+			assert.Contains(t, attrs, attribute.String("http.route", "/err"))
+			assert.Contains(t, attrs, attribute.String("http.method", "GET"))
+			assert.Contains(t, attrs, attribute.Int("http.status_code", tc.statusCode))
+			assert.Contains(t, attrs, attribute.String("echo.error", tc.echoError))
+		})
+	}
+}
+
+func TestErrorNotSwallowedByMiddleware(t *testing.T) {
+	e := echo.New()
+	r := httptest.NewRequest(http.MethodGet, "/err", nil)
+	w := httptest.NewRecorder()
+	c := e.NewContext(r, w)
+	h := otelecho.Middleware("foobar")(echo.HandlerFunc(func(c echo.Context) error {
+		return assert.AnError
+	}))
+
+	err := h(c)
+	assert.Equal(t, assert.AnError, err)
+}