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 swallows the error and does not pass it to other middlewares #2209

Closed
hcelaloner opened this issue Apr 21, 2022 · 2 comments · Fixed by #3656
Closed

OtelEcho swallows the error and does not pass it to other middlewares #2209

hcelaloner opened this issue Apr 21, 2022 · 2 comments · Fixed by #3656
Labels
bug Something isn't working instrumentation: otelecho
Projects

Comments

@hcelaloner
Copy link
Contributor

Background

Hi, we were trying to use OpenTelemetry echo middleware provided in this repository in our project. However, using it causes misfunction in other middlewares. For instance, while trying we were using another middleware to send errors to our current apm tool and it stopped recording errors.

It seems that the current implementation of echo middleware swallows the error and does not pass it to other middleware (https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/github.com/labstack/echo/otelecho/echo.go#L101).

Proposed Solution

  • Return the error received from the next middleware call.
  • Also don't call the registered HTTP error handler since it is automatically called if the error received from the next middleware call is returned.
@hcelaloner
Copy link
Contributor Author

Submitted a PR with the proposed solution but I guess with this solution middleware is not able to detect the correct HTTP status code returned since this time HTTP error handler is called later. So the only way to handle is to leave it as it is? If that is the case adding test cases may be a good idea? Or should we register it as the last middleware if we want to see capture in other middleware?

@MrAlias MrAlias added bug Something isn't working instrumentation: otelecho labels Nov 2, 2022
@MrAlias MrAlias added this to Needs triage in Bugs via automation Nov 2, 2022
@markhildreth-gravity
Copy link

markhildreth-gravity commented Mar 23, 2023

FYI, the Echo project had a similar issue with their Recover middleware. They decided to make a flag to allow the user to let the error drop through.

Edit: Although in hindsight, they don't need to know about the status code, so they don't need to call the error handler...

The way we handle this is just to ensure this middleware is first. Not an ideal solution though :/

pendolf added a commit to pendolf/opentelemetry-go-contrib that referenced this issue Mar 27, 2023
pendolf added a commit to pendolf/opentelemetry-go-contrib that referenced this issue Mar 27, 2023
MrAlias added a commit that referenced this issue Apr 26, 2023
…ewares. #2209 (#3656)

* fix do not ignore the error as it might be useful for upstream middlewares (#2209)

* Update CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* fix import-shadowing

* remove redundant assertions and use `assert.AnError`

* Remove stale test

* Remove unnecessary options

---------

Co-authored-by: Robert Pająk <pellared@hotmail.com>
Co-authored-by: Chester Cheung <cheung.zhy.csu@gmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Bugs automation moved this from Needs triage to Closed Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment