[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 #4420 (comment) 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 :-)

[shonigbaum]

Hey,
any news on this? We are facing the same issue. Will this be fixed soon? Or is there a workaround recommended?

[dmathieu]

otelecho is lacking an owner, is deprecated and will be removed in a couple months.
#5550

Resolving this would be a good way for someone to step up before becoming an owner.

[shonigbaum]

Oh, thank you for mentioning. Hopefully there will be an owner soon. I'll take this topic into my team.

[rattuscz]

That is unfortunate. Didn't realize there is a risk of that.

This issue is still not resolved mainly because of -> #4420 (comment)

I don't really know how to move forward without breaking it for someone, plus changing the behavior now would mean i would have problem to integrate it even in our own projects spending hours and writing tests for not much of a win.

So personally i would still prefer to remain backwards compatible by providing the config option that allows gradual adoption without suddenly breaking in minor/patch version change