Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

otelecho: remove global error handler call #4420

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
otelecho: config option to skip global error handler call
  • Loading branch information
rattuscz committed Oct 12, 2023
commit 4e40bbb32a42cabf2fd850bffcb2f0ee23d10482
8 changes: 8 additions & 0 deletions instrumentation/github.com/labstack/echo/otelecho/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ type config struct {
TracerProvider oteltrace.TracerProvider
Propagators propagation.TextMapPropagator
Skipper middleware.Skipper
HandleError bool
}

// Option specifies instrumentation configuration options.
Expand Down Expand Up @@ -66,3 +67,10 @@ func WithSkipper(skipper middleware.Skipper) Option {
cfg.Skipper = skipper
})
}

// WithoutHandleError instructs middleware to not call global error handler when next middleware/handler returns an error.
func WithoutHandleError() Option {
return optionFunc(func(cfg *config) {
cfg.HandleError = false
})
}
10 changes: 7 additions & 3 deletions instrumentation/github.com/labstack/echo/otelecho/echo.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ const (

// Middleware returns echo middleware which will trace incoming requests.
func Middleware(service string, opts ...Option) echo.MiddlewareFunc {
cfg := config{}
cfg := config{
HandleError: true,
}
for _, opt := range opts {
opt.apply(&cfg)
}
Expand Down Expand Up @@ -92,8 +94,10 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc {
err := next(c)
if err != nil {
span.SetAttributes(attribute.String("echo.error", err.Error()))
// invokes the registered HTTP error handler
c.Error(err)
if cfg.HandleError {
// invokes the registered HTTP error handler
c.Error(err)
}
}

status := c.Response().Status
Expand Down
44 changes: 44 additions & 0 deletions instrumentation/github.com/labstack/echo/otelecho/echo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,47 @@ func TestSkipper(t *testing.T) {
router.ServeHTTP(w, r)
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'ping' handler")
}

func TestHandleErrorDefault(t *testing.T) {
r := httptest.NewRequest("GET", "/ping", nil)
w := httptest.NewRecorder()

router := echo.New()
router.Use(Middleware("foobar"))
router.GET("/ping", func(c echo.Context) error {
return assert.AnError
})

handlerCalled := 0
router.HTTPErrorHandler = func(err error, c echo.Context) {
handlerCalled++
assert.ErrorIs(t, err, assert.AnError, "test error is expected in error handler")
assert.NoError(t, c.NoContent(http.StatusTeapot))
}

router.ServeHTTP(w, r)
assert.Equal(t, http.StatusTeapot, w.Result().StatusCode, "should call the 'ping' handler")
assert.Equal(t, 2, handlerCalled, "global error handler is called twice")
}

func TestWithoutHandleError(t *testing.T) {
r := httptest.NewRequest("GET", "/ping", nil)
w := httptest.NewRecorder()

router := echo.New()
router.Use(Middleware("foobar", WithoutHandleError()))
router.GET("/ping", func(c echo.Context) error {
return assert.AnError
})

handlerCalled := 0
router.HTTPErrorHandler = func(err error, c echo.Context) {
handlerCalled++
assert.ErrorIs(t, err, assert.AnError, "test error is expected in error handler")
assert.NoError(t, c.NoContent(http.StatusTeapot))
}

router.ServeHTTP(w, r)
assert.Equal(t, http.StatusTeapot, w.Result().StatusCode, "should call the 'ping' handler")
assert.Equal(t, 1, handlerCalled, "global error handler is called once")
}