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

[otelgin]: Catch and record panics in Middleware #5088

Open
asecondo opened this issue Feb 14, 2024 · 2 comments
Open

[otelgin]: Catch and record panics in Middleware #5088

asecondo opened this issue Feb 14, 2024 · 2 comments
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request instrumentation: otelgin

Comments

@asecondo
Copy link

Problem Statement

Today, the otelgin middleware does not handle panics. In the event that there's some bug in some service code or lower middleware layer that panics, this information is not present in the resulting span. It would be incredibly useful to leverage this information to help troubleshooting in development and production environments.

Proposed Solution

Catch, record, and "rethrow" any panics. This solution would likely look very similar to the otelgin.HTML(...) handler here.

Alternatives

  • Do nothing, as is currently done today
  • Add panic recovery handling to this middleware. This feels incorrect, as we'd be going beyond the expected responsibilities of this middleware.

Prior Art

As mentioned above, we would likely leverage a similar solution used in the same package here.

Additional Context

N/A.

@pellared
Copy link
Member

pellared commented Nov 26, 2024

None of our instrumentation libraries handles panics. Exception is (as you mentioned) otelgin.HTML, which does not look right to me. I would propose removing panic handling from otelgin.HTML.

The SDK is responsible for handling panics. span.End recovers in
https://github.com/open-telemetry/opentelemetry-go/blob/8b510c36ed26fafe151df35a251f4589f5959802/sdk/trace/span.go#L420

Therefore, any panic happening should already be caught by the defer span.End().

We could consider setting error span status in span.End when a panic occurs. However, this should be a separate issue in https://github.com/open-telemetry/opentelemetry-go. @MrAlias, was it ever proposed or discussed?

Similar comments:

@flc1125
Copy link
Member

flc1125 commented Nov 26, 2024

From the code, although it handles panic, there is no situation where Status is set to Error. However, in actual application, it is very necessary.

open-telemetry/opentelemetry-go@8b510c3/sdk/trace/span.go#L420

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request instrumentation: otelgin
Projects
None yet
Development

No branches or pull requests

3 participants