@sentry/node with includeLocalVariables is not showing anything about local variables on any errors on the Sentry dashboard

[ruohola]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

7.64.0 7.73.0

Framework Version

Node 18.15.0

Link to Sentry event

[private]

SDK Setup

import * as Sentry from '@sentry/node'

Sentry.init({
  environment: ...,
  dsn: ...,
  tracesSampleRate: 1.0,
  includeLocalVariables: true,
  integrations: [
    new Sentry.Integrations.LocalVariables({
      captureAllExceptions: true,
    }),
  ],
  debug: true,
})

Steps to Reproduce

I'm trying to use @sentry/node's new includeLocalVariables (https://blog.sentry.io/local-variables-for-nodejs-in-sentry/).
I'm using Sentry SDK 7.73.0 with Node 18.15.0 (TypeScript, without ESM modules), which should work fine. But my caught errors do not show any local variables in the Sentry Dashboard.

Expected Result

Each caught error would the values of show local variables as such on the Sentry dashboard (picture from the blog post):

Actual Result

The errors show up fine as before, but no local variables can be seen anywhere from the dashboard:

┆Issue is synchronized with this Jira Improvement by Unito

[martson77]

+1

[ruohola]

The LocalVariables integration only includes local variables for uncaught exceptions by default.

Interesting. And what about should/could local variables be included in exceptions caught by the Sentry Express integration, i.e. app.use(Sentry.Handlers.errorHandler())? That's my use case, and I am not seeing any local variables on exceptions handled by the integration.

[lforst]

@ruohola Hi, what node version are you using? Did you turn on the captureAllExceptions flag?

[ruohola]

@ruohola Hi, what node version are you using? Did you turn on the captureAllExceptions flag?

I'm using Node 18.15.0 as pointed out in opening. No, I did not yet enable captureAllExceptions. The performance hit does not entice me. My immediate question now is that should the exceptions caught by the Express integration include local variables? To me it would make sense if they did, since those are basically uncaught exceptions from my application's point of view and exceptions that propagate to Sentry.Handlers.errorHandler() are rare enough for any performance hit to be marginal.

[timfish]

since those are basically uncaught exceptions from my application's point of view

When talking about uncaught exceptions we are referring to those that would trigger uncaughtException or unhandledRejection events. These cause newer versions of node to terminate.

Caught exceptions are caused by any throw that is subsequentially caught by a try/catch. They may be caught in your code or they may be caught in some library code but they are still considered "caught" from the debuggers point of view.

The Chrome debugger has seperate options to pause on caught/uncaught:

exceptions that propagate to Sentry.Handlers.errorHandler() are rare enough for any performance hit to be marginal

In most cases, enabling captureAllExceptions should have negligible performance impact and we may even enable it by default in the future. However, I would still encourage you to test and profile it thouroughly before using it in a heavily loaded production app.

captureAllExceptions will pause execution and capture local variables every time a throw is encountered in code. For example, if you code uses something like throw new NotFoundError() to handle 404 status, every 404 will result in capturing local variables.

[ruohola]

since those are basically uncaught exceptions from my application's point of view

When talking about uncaught exceptions we are referring to those that would trigger uncaughtException or unhandledRejection events. These cause newer versions of node to terminate.

Caught exceptions are caused by any throw that is subsequentially caught by a try/catch. They may be caught in your code or they may be caught in some library code but they are still considered "caught" from the debuggers point of view.

The Chrome debugger has seperate options to pause on caught/uncaught:

Yes, I very much understand the difference between caught and uncaught exceptions. My question was that would it make sense for @sentry/node's captureException() (which is used inside Sentry.Handlers.errorHandler()) be altered so that one could include local variables on the error on a case-by-case basis. This would allow the Express integration to conveniently include local variables on top-level errors without the possible performance hit of enabling Sentry.Integrations.LocalVariables({ captureAllExceptions: true }).

 

captureAllExceptions will pause execution and capture local variables every time a throw is encountered in code. For example, if you code uses something like throw new NotFoundError() to handle 404 status, every 404 will result in capturing local variables.

Which does not sound at all something like I would want to enable.

[lforst]

My question was that would it make sense for @sentry/node's captureException() (which is used inside Sentry.Handlers.errorHandler()) be altered so that one could include local variables on the error on a case-by-case basis.

If it is possible, yes that would make sense, however, it is likely not. The idea is not bad.

Node does not provide a dedicated API to grab local variables in the current scope, but it is possible with Node.js' debugger/inspector API, which we are leveraging here. If you think it through, you can't retroactively tell the debugger to grab the local variable state as it was when an error was thrown. However, that's exactly what we would need to do, in order to make what you suggest happen. I scanned the debugger API and couldn't find a way (maybe I missed something though).

For the above reason, we need this all-or-nothing option. Either we can grab the variables in time, or we don't at all.

Which does not sound at all something like I would want to enable.

We thought people might not wanna have this on by default since this obviously impacts performance, so we added this option. Makes sense I think. This is addon functionality, not essential for the SDK to work. Feel free to leave it turned off.

In case you care about the technical details feel free to take a look at our code for this functionality. It is all within one file!

sentry-javascript/packages/node/src/integrations/localvariables.ts

Line 262 in 9dc0f12

export class LocalVariables implements Integration {

[AbhiPrasad]

Which does not sound at all something like I would want to enable.

The overhead caused by this really does vary from app to app, I would advise profiling in more detail if you're really interested in using this! Perhaps even with extra throwing the overhead is worthwhile to get extra debugging context.

[timfish]

I've opened a PR to add rate limiting for capturing caught exceptions. This should make it safe to enable for all:
#9102

[AbhiPrasad]

We've released changes to make capturing local variables for all errors (handled and unhandled) the default in 7.73.0 of the Node SDK. Tim's also done the great work of making sure we optimize the overhead in these scenarios so that impact is minimal. Please give it a try and let us know what you think!

https://github.com/getsentry/sentry-javascript/releases/tag/7.73.0

[ruohola]

After upgrading to 7.73.0 (and even explicitly enabling integrations: [new Sentry.Integrations.LocalVariables({ captureAllExceptions: true })]), I'm still not seeing anything about local variables in the stack traces shown for errors on my Sentry dashboard. Here are some screenshots from parts of my dashboard that don't show sensitive information:

[maximlopin2]

Maybe someone has an example of code that works with includeLocalVariables?

[timfish]

Maybe someone has an example of code that works with includeLocalVariables?

It works with all the integration tests. Do you have a simplified example of it not working?

[maximlopin2]

Maybe someone has an example of code that works with includeLocalVariables?

It works with all the integration tests. Do you have a simplified example of it not working?

"@sentry/node": "^7.101.1"

Example

import * as Sentry from '@sentry/node'

Sentry.init({
  dsn: '...',
  tracesSampleRate: 1.0,
  includeLocalVariables: true
})

const someVariable1 = 1

try {
  const someVariable2 = 2
  unknownFunction()
} catch (e) {
  Sentry.captureException(e)
}

Environment

[AbhiPrasad]

I suspect this might be an issue with the top level scope. @maximlopin2 if you put all the logic into a function, does local variables show up?

function run() {
  const someVariable1 = 1
  
  try {
    const someVariable2 = 2
    unknownFunction()
  } catch (e) {
    Sentry.captureException(e)
  }
}

run();

[maximlopin2]

I suspect this might be an issue with the top level scope. @maximlopin2 if you put all the logic into a function, does local variables show up?

function run() {
  const someVariable1 = 1
  
  try {
    const someVariable2 = 2
    unknownFunction()
  } catch (e) {
    Sentry.captureException(e)
  }
}

run();

Yes, now it at least shows one of the variables 🤔

[lforst]

@maximlopin2 I think that is as good as it gets for now 🤔 @timfish we need a stack frame to match stuff right? Maybe there's an issue with top level calls.

Also @maximlopin2 are just doing plain Node.js code or ts-node or something like that. I am thinking that the runtime optimized someVariable2 away when the exception was recorded...

[timfish]

I'll add some tests for top level capture and see if we can add any special handling for that.

[getsantry]

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 remove the label Waiting for: Community, I will leave it alone ... forever!

---

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