Skip to content

[node] Stack locals #6369

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

Closed
smeubank opened this issue Dec 1, 2022 · 12 comments · Fixed by #6478
Closed

[node] Stack locals #6369

smeubank opened this issue Dec 1, 2022 · 12 comments · Fixed by #6478
Assignees
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK Package: node Issues related to the Sentry Node SDK Package: remix Issues related to the Sentry Remix SDK Type: Improvement

Comments

@smeubank
Copy link
Member

smeubank commented Dec 1, 2022

Problem Statement

Node js does not capture stack locals similar to a few other server side SDKs

Stack-locals → Some SDKs (Python and PHP) will pick up variable values within the stacktrace. These can be scrubbed, or this behavior can be disabled altogether if necessary.

comment from an unrelated NextJs issue: #6321 (comment)

Could this be easily applied to remix, next, and any other fromworks depending on Node?

Solution Brainstorm

There are possible concerns in Node that this would create some overhead (if it is possible at all)

If it was to be implemented could it be controlled by some user flag?

@smeubank smeubank added Type: Improvement Feature: Errors Package: nextjs Issues related to the Sentry Nextjs SDK Package: node Issues related to the Sentry Node SDK Package: remix Issues related to the Sentry Remix SDK labels Dec 1, 2022
@smeubank
Copy link
Member Author

smeubank commented Dec 1, 2022

summarizing some internal discussions on this issue in story form:

@lforst found out

  • Seems it actually should be possible to access debugger data at runtime with Node Inspector API
    then @AbhiPrasad said
  • users essentially must turn on debug mode
  • Since you can disable inspect in node
    then @kamilogorek did
  • local tests confirm it works on some level

image

image

@AbhiPrasad
Copy link
Member

@kamilogorek

1 similar comment
@kamilogorek
Copy link
Contributor

@kamilogorek

@AbhiPrasad
Copy link
Member

image

image

@smeubank
Copy link
Member Author

smeubank commented Dec 6, 2022

Image

Image

@smeubank
Copy link
Member Author

smeubank commented Dec 6, 2022

https://github.com/kamilogorek/sentry-node-local-variables

@kamilogorek example

Refactored everything so its easier to understand

https://github.com/kamilogorek/sentry-node-local-variables/blob/master/vars.js

@timfish
Copy link
Collaborator

timfish commented Dec 6, 2022

Oh this is really nice!

@timfish
Copy link
Collaborator

timfish commented Dec 7, 2022

How do you want this adding? Keep it as an integration?

Should I do some testing to see how long this suspends the event loop to determine if this can be enabled by default?

@AbhiPrasad
Copy link
Member

Keep it as an integration?

Yes please!

Should I do some testing to see how long this suspends the event loop to determine if this can be enabled by default?

This would be great, but we can also put this off. We can make this opt-in to start, and default it in v8. So we do it the following way.

  1. release the integration, gate it with a top level option
  2. update sentry docs to enable this option by default (so they copy past Sentry.init with the option set to true)
  3. set the option to be true by default in v8

@github-actions
Copy link
Contributor

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@therealarkin
Copy link

Question - is there anything we need to do/tell the customer to do on the issues details page to enable this if this is not enabled?

@smeubank
Copy link
Member Author

smeubank commented Feb 2, 2023

Question - is there anything we need to do/tell the customer to do on the issues details page to enable this if this is not enabled?

@therealarkin user need to configure like this

image

covered here blog post and SDK docs here

not sure how to detect if they are missing, in terms of the event payload

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK Package: node Issues related to the Sentry Node SDK Package: remix Issues related to the Sentry Remix SDK Type: Improvement
Projects
None yet
5 participants