[otelecho] Add option to skip calling global handler

[rattuscz]

Problem Statement

I want to be able to control whether otelecho middleware calls the global error handler or not.

Using multiple middlewares that cannot be configured this way the global error handler gets called multiple times for the exact same error. Handling this multiple processing of same error on global error handler seems to me as wrong way to go.

The only way to prevent this is to eat the error before this middleware is run after request is handled, which then does not append the error to span as attribute.

I believe this also goes against best practice of either handling the error or propagating, not both.

Proposed Solution

Add new option to middleware config to control this behavior, with default value resulting in same behavior as it is now to be BC.

Prior Art

In echo request logger middleware there is the exact same setting that controls calling of the global handler: https://github.com/labstack/echo/blob/98a523756d875bc13475bcb6237f09e771cbe321/middleware/request_logger.go#L115

[erkanzileli]

I'm having the same issue. You're right. The middleware calls the error handler and returns the error. When the middleware returns an error, echo calls the error handler again. so in this way, the error handler gets called twice. This results in a duplicated response body like below.

{
    "message": "content"
}
{
    "message": "content"
}

[MrAlias]

Instead of modifying the instrumentation, I would recommend you register a rate-limiting error handler. You can customize the behavior of how errors are handle however you want by using your own error handler. Adding to the API surface error of the instrumentation does not seem like a prudent way to solve an issue that can already be solved with a custom error handler.

[rattuscz]

@MrAlias I disagree that it's issue that should be solved in error handler, that is a workaround at best, because the real issue is calling error handler multiple times for the same error. That is definitely not the correct behavior.

Accepting the BC break and simply removing the line calling the error handler altogether as @pellared suggested is also fine as the error is propagated in the middleware chain up to be handled.

[pellared]

@rattuscz See https://github.com/open-telemetry/opentelemetry-go-contrib/pull/4420#issuecomment-1813097682 by @MrAlias:

I'm not opposed to this.

Also we agree that it is acceptable (breaking) change as the Go module is not stable. I think that the users would like this change.

[rattuscz]

As long as it's addressed somehow I'm happy :-)