otelecho: remove global error handler call

[rattuscz]

First try at implementing #4419

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: rattuscz / name: Petr Vítek (4e40bbb, c3761c7, 08e7c3a, 0ae232b, 07184f6, 55bb621, e1b9126)
• ✅ login: hanyuancheung / name: Chester Cheung (d34410e)
• ✅ login: dmathieu / name: Damien Mathieu (55bb621)

[MrAlias]

#4419 (comment)

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.

[pellared]

To be honest, I do not understand why c.Error(err) is even called given that the err is returned by the middleware. Shouldn't we simply remove this line?

PS. I never used echo.

[MrAlias]

To be honest, I do not understand why c.Error(err) is even called given that the err is returned by the middleware. Shouldn't we simply remove this line?

PS. I never used echo.

I'm not opposed to this.

[hanyuancheung]

This PR SGTM

[hanyuancheung]

@rattuscz Please do not forget to add a changelog.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3cecdcf) 81.1% compared to head (d34410e) 80.8%.

❗ Current head d34410e differs from pull request most recent head e1b9126. Consider uploading reports for the commit e1b9126 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #4420     +/-   ##
=======================================
- Coverage   81.1%   80.8%   -0.3%     
=======================================
  Files        150     150             
  Lines      10753   10349    -404     
=======================================
- Hits        8725    8371    -354     
+ Misses      1872    1834     -38     
+ Partials     156     144     -12     

Files	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%> (ø)

... and 22 files with indirect coverage changes

[rattuscz]

To be honest, I do not understand why c.Error(err) is even called given that the err is returned by the middleware. Shouldn't we simply remove this line?

PS. I never used echo.

Yes it seems weird to me too, it's probably propagated by copy pasting some example from echo documentation.

Anyway it will be BC break if you simply remove it now as users of this package had to already solve it somehow on their end - either by ignoring some errors in the global error handler or by carefully stacking middlewares to get expected behavior.

[pellared]

Anyway it will be BC break

It is not a stable Go module so breaking change are acceptable if there are good reasons to do so.

[rattuscz]

Okie, changed this to implement the removal.
I've once again today run into issue with this so I hope this is merged & released soon :-)

[dmathieu]

Tests are failing.

[dmathieu]

This is also improperly placed in the changelog. It needs to be in the unreleased section.

[rattuscz]

Tests are failing.

Oh well it actually depends on the http status code being already set (for example by error handler.. 😞 ).

So this middleware would require some other middleware up in the chain calling the error handler or it needs to call the error handler itself.

Otherwise even though the server works properly - global error handler is called by echo if err comes out of middlewares - it will not record correct http status code and span ok/error status.

Any thoughts? 🤔

[dmathieu]

Well, it looks like the TestStatusError test needs to be updated to properly reflect those changes.

[rattuscz]

Well, it looks like the TestStatusError test needs to be updated to properly reflect those changes.

Sure I can fix the tests, if that's the way to go.
Not really sure if there is some better way how to avoid those problems, because it will still be quite easy to have it wrongly setup just because of order of middlewares/logger/error handler.

In our projects we tend to have logger as the last middleware and eating the error returned from handler functions so the response is already set, error logged/handled and all is great.
Others may have different order and suffer breaks when we change this.

Will leave it here for a few days before commiting

[asp3cto]

Still got this error, and well, it's quite difficult to catch because of middleware debugging issues. In echo style, the handlers and middlewares should not call c.Error explicitly, and refactoring whole project just to fit the handling logic in this middleware is not great. Any plans to fix?

[dmathieu]

This PR has had no response by its owner for 2 years. I think it's safe to say that you can take its commits, merge with the main branch and open a new one.

[asp3cto]

@dmathieu can this be done as a breaking change or just an option with current implementation by default?

[dmathieu]

The package isn't stable. So it can be a breaking change.

[asp3cto]

@dmathieu Decided to make it an option with current logic by default, see #8025