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

@sentry/node async contexts play poorly with http response events #13452

Closed
3 tasks done
theo-m opened this issue Aug 23, 2024 · 2 comments
Closed
3 tasks done

@sentry/node async contexts play poorly with http response events #13452

theo-m opened this issue Aug 23, 2024 · 2 comments
Labels
Package: node Issues related to the Sentry Node SDK

Comments

@theo-m
Copy link

theo-m commented Aug 23, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.26.0

Framework Version

No response

Link to Sentry event

https://mra-30.sentry.io/issues/5747145444

Reproduction Example/SDK Setup

Here is reproduction repository: https://github.com/theo-m/repro-sentry-logging

It seems like sentry's async footwork plays poorly with pino-http. I've opened the issue here because I don't think pino-http is at fault, it's just using node's response events.

Steps to Reproduce

# 1. run 
pnpm start

# 2. run 
pnpm test

You'll observe that express does send the responses appropriately with the right errors - but, what's logged by Pino is entirely different.

Expected Result

see repo https://github.com/theo-m/repro-sentry-logging

Actual Result

see repo https://github.com/theo-m/repro-sentry-logging

@github-actions github-actions bot added the Package: node Issues related to the Sentry Node SDK label Aug 23, 2024
@Lms24
Copy link
Member

Lms24 commented Aug 26, 2024

Hi @theo-m, thanks for writing in and thanks for providing a reproduction!

I gave it a try and what I can see is that the error that's sent to Sentry is the "not happy" error you're throwing in your test route. This is what you'd expect, right? Likewise, this is the error that I get returned on the client side.

Now, pino logs the "failed with status code 500" error, but it does so regardless of if I remove all Sentry imports or leave them in place.
The only difference I notice is that there are a couple more stack frames from opentelemetry but this shouldn't be a problem. It's just expected behaviour from wrapping some node modules.

Can you clarify what's the expected behaviour/what exactly is going wrong?

@theo-m
Copy link
Author

theo-m commented Aug 26, 2024

I gave it a try and what I can see is that the error that's sent to Sentry is the "not happy" error you're throwing in your test route. This is what you'd expect, right? Likewise, this is the error that I get returned on the client side.

Yes indeed! My problem is what's logged by pino, which is confusing when reading server logs - and the heart of the problem.

Now, pino logs the "failed with status code 500" error, but it does so regardless of if I remove all Sentry imports or leave them in place.
The only difference I notice is that there are a couple more stack frames from opentelemetry but this shouldn't be a problem. It's just expected behaviour from wrapping some node modules.

You're right! I've tried a couple of ablations and permutations and got things mixed up, I'm observing the issue when removing Sentry still - my bad for opening a ticket on a Friday haha

Will close, sorry for the waste of time and thanks for looking at this 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK
Projects
Archived in project
Development

No branches or pull requests

2 participants