-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node): Fix local variables capturing for out-of-app frames #17545
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
base: develop
Are you sure you want to change the base?
Changes from 7 commits
d92b0a0
7ca795c
5323928
7fb4542
662ab6d
367ef5c
76893c5
6ff90dc
c81dc55
d2a40cf
651d3f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* eslint-disable no-unused-vars */ | ||
|
||
const Sentry = require('@sentry/node'); | ||
// const { loggingTransport } = require('@sentry-internal/node-integration-tests'); is throwing error that package not found, so using relative path | ||
const { loggingTransport } = require('../../../src/index.ts'); | ||
|
||
// make sure to create the following file with the following content: | ||
// function out_of_app_function() { | ||
// const outOfAppVar = 'out of app value'; | ||
// throw new Error('out-of-app error'); | ||
// } | ||
|
||
// module.exports = { out_of_app_function }; | ||
Comment on lines
3
to
12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is far too brittle, but not sure how to best do this. Maybe @timfish you have some ideas? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can either do this or use |
||
|
||
const { out_of_app_function } = require('./node_modules/test-module/out-of-app-function.js'); | ||
|
||
function in_app_function() { | ||
const inAppVar = 'in app value'; | ||
out_of_app_function(); | ||
} | ||
|
||
Sentry.init({ | ||
dsn: 'https://[email protected]/1337', | ||
transport: loggingTransport, | ||
includeLocalVariables: true, | ||
// either set each frame's in_app flag manually or import the `out_of_app_function` from a node_module directory | ||
// beforeSend: (event) => { | ||
// event.exception?.values?.[0]?.stacktrace?.frames?.forEach(frame => { | ||
// if (frame.function === 'out_of_app_function') { | ||
// frame.in_app = false; | ||
// } | ||
// }); | ||
// return event; | ||
// }, | ||
}); | ||
|
||
setTimeout(async () => { | ||
try { | ||
in_app_function(); | ||
} catch (e) { | ||
Sentry.captureException(e); | ||
await Sentry.flush(); | ||
|
||
return null; | ||
} | ||
}, 1000); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* eslint-disable no-unused-vars */ | ||
|
||
const Sentry = require('@sentry/node'); | ||
// const { loggingTransport } = require('@sentry-internal/node-integration-tests'); is throwing error that package not found, so using relative path | ||
const { loggingTransport } = require('../../../src/index.ts'); | ||
|
||
const { out_of_app_function } = require('./node_modules/test-module/out-of-app-function.js'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Missing Dependency in Test SetupThe new local variables test files depend on Additional Locations (1) |
||
|
||
Sentry.init({ | ||
dsn: 'https://[email protected]/1337', | ||
transport: loggingTransport, | ||
includeLocalVariables: true, | ||
integrations: [ | ||
Sentry.localVariablesIntegration({ | ||
includeOutOfAppFrames: true, | ||
}), | ||
], | ||
// either set each frame's in_app flag manually or import the `out_of_app_function` from a node_module directory | ||
// beforeSend: (event) => { | ||
// event.exception?.values?.[0]?.stacktrace?.frames?.forEach(frame => { | ||
// if (frame.function === 'out_of_app_function') { | ||
// frame.in_app = false; | ||
// } | ||
// }); | ||
// return event; | ||
// }, | ||
}); | ||
|
||
function in_app_function() { | ||
const inAppVar = 'in app value'; | ||
out_of_app_function(); | ||
} | ||
|
||
setTimeout(async () => { | ||
try { | ||
in_app_function(); | ||
} catch (e) { | ||
Sentry.captureException(e); | ||
await Sentry.flush(); | ||
return null; | ||
} | ||
}, 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be happening, are you running
yarn build
in the root of the repo? You might have to runyarn clean
and delete node modules to get stuff in a good state.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, unfortunately, it's happening even after clean installation and building.
The steps I took:
yarn
yarn clean
yarn build
loggingTransport
from@sentry-internal/node-integration-tests
DEBUG=true yarn workspace @sentry-internal/node-integration-tests test -- LocalVariables
You will get this error:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the issue is
yarn workspace
, if youcd
into the directory and run the tests there directly do you get the same issues? I'm unfortunately not able to reproduce.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright yeah. My bad. I was building in the root directory. I should've run the build command in the
node-integration-tests
directory as well. Working fine now. Let's wait for @timfish for their reply to the last comment now.