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

rewrite plugin-contextualize without domain #1924

Merged
merged 8 commits into from
Mar 8, 2023
Merged

Conversation

djskinner
Copy link
Contributor

@djskinner djskinner commented Feb 10, 2023

Goal

Changeset

  • rewrite plugin-contextualize to not use the deprecated node domain API.
    • The usage API remains the same (contextualize: (fn: () => void, onError: OnErrorCallback) => void) but there are changes in behaviour.
    • Before, the plugin would capture unhandled errors regardless of the config values autoDetectErrors and enabledErrorTypes.unhandledExceptions. Now, the plugin will only capture unhandled exceptions if both autoDetectErrors and enabledErrorTypes.unhandledExceptions are enabled.
    • Before, the plugin was unable to add context to unhandled promise rejections. Now, (since node 16) unhandled promise rejections are captured with context if autoDetectErrors and enabledErrorTypes.unhandledRejections are enabled.

Testing

  • e2e coverage covering both uncaught exceptions and unhandled rejections
  • no unit test coverage - it appears impossible to cover this in a meaningful way with jest

@djskinner djskinner force-pushed the contextualize branch 2 times, most recently from d69e437 to 9c49d75 Compare February 10, 2023 15:58
@github-actions
Copy link

github-actions bot commented Feb 10, 2023

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 44.63 kB 13.64 kB
After 44.63 kB 13.64 kB
± No change No change

code coverage diff

<temporarily disabled>

Generated by 🚫 dangerJS against a18b793

@djskinner djskinner force-pushed the contextualize branch 11 times, most recently from 9b06144 to 3874407 Compare February 14, 2023 12:13
@djskinner djskinner marked this pull request as ready for review February 15, 2023 17:35
@djskinner djskinner force-pushed the node-remove-domain branch 5 times, most recently from 103df37 to b72bcd5 Compare February 23, 2023 14:15
Base automatically changed from node-remove-domain to integration/v8 March 6, 2023 11:17
@@ -42,7 +42,7 @@ Scenario Outline: unhandled promise rejections are not reported when autoDetectE
When I invoke the "<lambda>" lambda in "features/fixtures/simple-app" with the "events/<type>/promise-rejection.json" event
Then the lambda response "errorMessage" equals "Error: yikes"
And the lambda response "errorType" equals "Runtime.UnhandledPromiseRejection"
And the lambda response "trace" is an array with 6 elements
And the lambda response "trace" is an array with 5 elements
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change here is that the line " at process.EventEmitter.emit (domain.js:483:12)" is no longer present. This frame was being added due to the mere presence of require('domain') in the contextualize plugin, which is part of the node notifier and therefore also instantiated in aws lambda.

@djskinner djskinner requested a review from imjoehaines March 6, 2023 17:22
@djskinner djskinner merged commit 630d9ca into integration/v8 Mar 8, 2023
@djskinner djskinner deleted the contextualize branch March 8, 2023 08:53
@gingerbenw gingerbenw mentioned this pull request Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants