fix(otelecho): add config option to skip global error handler call

[asp3cto]

Add WithSkipErrorHandler configuration option to support Echo's native error handling pattern where handlers return errors instead of calling c.Error() explicitly.

This prevents double responses on errors propagated to the otelecho middleware.

Fixes #4419, related to #4420

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.7%. Comparing base (aea87ec) to head (96e4f5d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #8025   +/-   ##
=====================================
  Coverage   79.7%   79.7%           
=====================================
  Files        190     190           
  Lines      11912   11919    +7     
=====================================
+ Hits        9495    9502    +7     
  Misses      2062    2062           
  Partials     355     355           

Files with missing lines	Coverage Δ
...tation/github.com/labstack/echo/otelecho/config.go	100.0% <100.0%> (ø)
...entation/github.com/labstack/echo/otelecho/echo.go	100.0% <100.0%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dmathieu]

I'm not super fond of negative options (WithoutHandleError).
The option may make sense though (@scorpionknifes, as code owner, do you have an opinion?)

I wonder if we could make the option more powerful by having a WithOnError(func (c echo.Context, err error))
It means to keep the same behavior, folks would need to do this:

r.Use(otelecho.Middleware("my-server", WithOnError(func(c echo.Context, err error) {
  c.Error(err)
}))

And at the same time, as I write this, it seems a bit overfluous. If the child middleware returns and error, it's not for us to call c.Error(). We do telemetry, not a full app.
So entirely removing this may make more sense.

[asp3cto]

Well, the reason i didn't remove c.Error completely is to keep backwards compatibility, even for an unstable package.
Waiting for @scorpionknifes.

[dmathieu]

Unstable packages don't need to keep backwards compatibility, they are unstable :)

[asp3cto]

@dmathieu i understand) @scorpionknifes has no activity since February, should we wait for some other review or i can fix to remove the call completely?

[dmathieu]

I would remove the option (no push force please. Also, a new commit is more easily reverted).
I'll bring this to the SIG meeting later today. Feel free to join (https://github.com/open-telemetry/community/?tab=readme-ov-file#implementation-sigs)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: asp3cto / name: asp3cto (893c4b6)
• ✅ login: dmathieu / name: Damien Mathieu (849baae, 96e4f5d, d87a4cd, e834961)

[asp3cto]

I see some tests are failing. The issue is that the response status code should be read by this middleware, but it can be set in the error handler, which runs after all middlewares. Looking for a workaround.

The only way i currently see is to do like in echo request logger (https://github.com/labstack/echo/blob/e644ff8f7bb01c694cacec3ad22a7471609ea106/middleware/request_logger.go#L337), but by doing so we will loose the status code.

[dmathieu]

Ah! That would explain why c.Error() was called.

[asp3cto]

I need some advice here :)
In this example:

r := httptest.NewRequest("GET", "/ping", http.NoBody)
	w := httptest.NewRecorder()

	router := echo.New()
	router.Logger = log.New("echo")
	router.Use(middleware.RequestLoggerWithConfig(middleware.RequestLoggerConfig{
		LogValuesFunc: func(c echo.Context, v middleware.RequestLoggerValues) error {
			fmt.Println("Request:", c.Request().Method, c.Request().URL.Path, "->", v.Status)
			return nil
		},
		HandleError: false,
		LogStatus:   true,
	}))
	router.Use(otelecho.Middleware("foobar"))
	router.GET("/ping", func(_ echo.Context) error {
		return assert.AnError
	})

	handlerCalled := 0
	router.HTTPErrorHandler = func(err error, c echo.Context) {
		handlerCalled++
		fmt.Println("Error handler called:", err, "should be:", assert.AnError)
		err = c.NoContent(http.StatusTeapot)
		if err != nil {
			fmt.Println("Failed to write response:", err)
		}
	}

	router.ServeHTTP(w, r)
	fmt.Println("Response code:", w.Result().StatusCode, "should be:", http.StatusTeapot)
	fmt.Println("Handler called times:", handlerCalled, "should be: 1")

i tested the behaviour of the echo request logger. I found out that it logs http.StatusOk if HandleError=true, and http. StatusTeapot if false. So the issue exists because of echo error handling design, and there is a tradeoff in such middlewares: get valid response by calling c.Error two times (and process this in custom handler) or skip double call and loose the actual response data after the handler.

[asp3cto]

labstack/echo#1948
As i said, echo design issue) Based on the last message there, maybe it's a good idea to at least add a warning to README to use the Commited filter in custom error handlers.

[flc1125]

I wonder if we could make the option more powerful by having a WithOnError(func (c echo.Context, err error))

I agree with this approach; we only need to provide an interface for users to manage how errors are handled, or even choose not to handle errors at all.

Additionally, we may also need to clarify the purpose of the echo.Error() method, and determine whether otel should support it or if it should be left to echo to manage on its own.

[asp3cto]

@flc1125 the main issue is that if the middleware doesn't call c.Error explicitly, we don't get the right response status code if it's overrided by the custom error handler.

[dmathieu]

And if we call it, folks have issues as it's being called multiple times.
I think going to WithOnError seems the most logical path forward. We should also have a comment stating explicitly that if folks call WithOnError, their error endpoint may be called multiple times, or their span may have an invalid response status.

With this method, I think it makes sense to keep the default behavior similar to what we have today.

[asp3cto]

So we need to have default cfg.OnError = func(c echo.Context, err error) { c.Error(err) } and add a comment like "If this function doesn't call c.Error, response status code will be invalid if it is overriden in HTTPErrorHandler, and if it does, your custom error handler should check the commitment status of the response"? Just to make it clear

[asp3cto]

I have an idea to add a wrapper for HTTPErrorHandler to make sure telemetry is handled correctly on error. It means removing all recording of telemetry if we get an error and do it in the error handler, like e.HTTPErrorHandler=otelecho.HTTPErrorHandler(customHandler)

[asp3cto]

@dmathieu any thoughts?

[flc1125]

I can try to research it to figure out the function of c.Error(), but it might take some time.

[asp3cto]

@flc1125 we've already figured it out, the call is needed to set the actual status code in the span if it was modified in custom HTTPErrorHandler

[dmathieu]

What's the need to make this public?

[asp3cto]

no real need actually :)

[asp3cto]

currently waiting for owner review?

[dmathieu]

Waiting for a second approver/maintainer review.

[flc1125]

I have completed my research and here are my conclusions: I lean towards removing the c.Error() logic.

The reasons are:

I believe c.Error is used to handle errors caused by uncontrollable underlying system factors, similar to Go's panic mechanism.

Because of this, when we do not use c.Error(echo.NewHTTPError(...)), it defaults to returning a 500 status code error—isn't this precisely the typical underlying system error?

Furthermore, I think the role of middleware, unless it triggers errors from uncontrollable underlying factors, should be to pass the error returned by the handler unchanged, and this error will ultimately affect the final response status code.

Impact of adjusting according to this method: (and the impact is significant)

For projects relying on this component, when an error occurs, the status code will change from 500 to 200, unless we return an error via echo.NewHTTPError(...).

If we decide to proceed with this, we need to document this change in the CHANGELOG.

[asp3cto]

@flc1125 c.Error is also used for setting the correct status code in case we change it in the HTTPErrorHandler, see #8025 (comment)
Here out error handler always sets StatusTeapot if triggered, and if any middleware doesn't call c.Error, it can't read the actual response status code, because error handler always runs last.
That's why we decided to keep the default logic and also to give the user an opportunity to make a tradeoff with WithOnError.

[flc1125]

@asp3cto I believe that https://github.com/labstack/echo/blob/e644ff8f7bb01c694cacec3ad22a7471609ea106/echo.go#L887 must always be used in conjunction with the logic at https://github.com/labstack/echo/blob/e644ff8f7bb01c694cacec3ad22a7471609ea106/echo.go#L424-L430. Any scenario where either is unconfigured or fails to implement similar logic will result in an inability to fully manage status codes.

[asp3cto]

I understand, but the main issue is custom HTTPErrorHandler. Builtin middlewares do the same: https://github.com/labstack/echo/blob/e644ff8f7bb01c694cacec3ad22a7471609ea106/middleware/request_logger.go#L287

They also check if HandleError is set and call c.Error if it is true to get the actual status.

[flc1125]

They also check if HandleError is set and call c.Error if it is true to get the actual status.

If there is a precedent, I think this logic makes sense.

[dmathieu]

I'm using the merge of this PR to call out that otelecho is lacking a code owner, and will be deprecated/removed in a few weeks if nobody steps up.
#8056