From a72509f0e270b6009226a442bc18a7efe7c218ef Mon Sep 17 00:00:00 2001 From: Dan Skinner Date: Fri, 10 Feb 2023 15:17:37 +0000 Subject: [PATCH 1/6] rewrite plugin-contextualize without domain --- CHANGELOG.md | 1 + .../plugin-contextualize/contextualize.js | 34 +++-- .../test/contextualize.test.ts | 119 ------------------ .../uncaught-exception.js | 6 + .../contextualize-unhandled-rejection.js | 15 +++ test/node/features/unhandled_errors.feature | 29 +++++ 6 files changed, 67 insertions(+), 137 deletions(-) delete mode 100644 packages/plugin-contextualize/test/contextualize.test.ts create mode 100644 test/node/features/fixtures/unhandled/scenarios/contextualize-unhandled-rejection.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 77f6682e6e..80c69b9f3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ### Changed - (plugin-navigation-breadcrumbs) calling `pushState` or `replaceState` no longer triggers a new session when `autoTrackSessions` is enabled [#1820](https://github.com/bugsnag/bugsnag-js/pull/1820) +- (plugin-contextualize) reimplement without relying on the deprecated node Domain API [#1924](https://github.com/bugsnag/bugsnag-js/pull/1924) ## TBD diff --git a/packages/plugin-contextualize/contextualize.js b/packages/plugin-contextualize/contextualize.js index 82c305bf1e..f9e2e3cc89 100644 --- a/packages/plugin-contextualize/contextualize.js +++ b/packages/plugin-contextualize/contextualize.js @@ -1,6 +1,5 @@ -/* eslint node/no-deprecated-api: [error, {ignoreModuleItems: ["domain"]}] */ -const domain = require('domain') -const { getStack, maybeUseFallbackStack } = require('@bugsnag/core/lib/node-fallback-stack') +const { getStack } = require('@bugsnag/core/lib/node-fallback-stack') +const clone = require('@bugsnag/core/lib/clone-client') module.exports = { name: 'contextualize', @@ -9,21 +8,20 @@ module.exports = { // capture a stacktrace in case a resulting error has nothing const fallbackStack = getStack() - const dom = domain.create() - dom.on('error', err => { - // check if the stacktrace has no context, if so, if so append the frames we created earlier - if (err.stack) maybeUseFallbackStack(err, fallbackStack) - const event = client.Event.create(err, true, { - severity: 'error', - unhandled: true, - severityReason: { type: 'unhandledException' } - }, 'contextualize()', 1) - client._notify(event, onError, (e, event) => { - if (e) client._logger.error('Failed to send event to Bugsnag') - client._config.onUncaughtException(err, event, client._logger) - }) - }) - process.nextTick(() => dom.run(fn)) + const clonedClient = clone(client) + + // add the stacktrace to the cloned client so it can be used later + // by the uncaught exception handler. Note the unhandled rejection + // handler does not need this because it gets a stacktrace + clonedClient.fallbackStack = fallbackStack + + clonedClient.addOnError(onError) + + if (client._clientContext) { + client._clientContext.run(clonedClient, fn) + } else { + fn() + } } return contextualize diff --git a/packages/plugin-contextualize/test/contextualize.test.ts b/packages/plugin-contextualize/test/contextualize.test.ts deleted file mode 100644 index 9010cd10b4..0000000000 --- a/packages/plugin-contextualize/test/contextualize.test.ts +++ /dev/null @@ -1,119 +0,0 @@ -import Client from '@bugsnag/core/client' -import { schema } from '@bugsnag/core/config' -import plugin from '../' -import fs from 'fs' -import Event from '@bugsnag/core/event' - -// mock an async resource - -const items = ['a', 'b', 'c'] - -// node-style error-first -function load (index: number, cb: (err: Error | null, res?: string) => void) { - process.nextTick(() => { - const item = items[index] - if (item) return cb(null, item) - cb(new Error('no item available')) - }) -} - -describe('plugin: contextualize', () => { - it('should call the onUnhandledException callback when an error is captured', done => { - const c = new Client({ - apiKey: 'api_key', - onUncaughtException: (err: Error) => { - expect(err.message).toBe('no item available') - done() - }, - plugins: [plugin] - }, { - ...schema, - onUncaughtException: { - validate: (val: unknown) => typeof val === 'function', - message: 'should be a function', - defaultValue: () => {} - } - }) - c._setDelivery(client => ({ - sendEvent: (payload, cb) => { - expect(payload.events[0].errors[0].errorMessage).toBe('no item available') - expect(payload.events[0].severity).toBe('warning') - expect(payload.events[0]._user).toEqual({ - id: '1a2c3cd4', - name: 'Ben Gourley', - email: 'ben.gourley@bugsnag.com' - }) - cb(null) - }, - sendSession: () => {} - })) - const contextualize = c.getPlugin('contextualize') - contextualize(() => { - load(8, (err) => { - if (err) throw err - }) - }, (event: Event) => { - event.setUser('1a2c3cd4', 'ben.gourley@bugsnag.com', 'Ben Gourley') - event.severity = 'warning' - }) - }) - - it('should add a stacktrace when missing', done => { - const c = new Client({ - apiKey: 'api_key', - onUncaughtException: () => { - done() - }, - plugins: [plugin] - }, { - ...schema, - onUncaughtException: { - validate: (val: unknown) => typeof val === 'function', - message: 'should be a function', - defaultValue: () => {} - } - }) - c._setDelivery(client => ({ - sendEvent: (payload, cb) => { - expect(payload.events[0].errors[0].errorMessage).toBe('ENOENT: no such file or directory, open \'does not exist\'') - expect(payload.events[0].errors[0].stacktrace[0].file).toBe(`${__filename}`) - cb(null) - }, - sendSession: () => {} - })) - const contextualize = c.getPlugin('contextualize') - contextualize(() => { - fs.createReadStream('does not exist') - }) - }) - - it('should tolerate a failed event', done => { - const c = new Client({ - apiKey: 'api_key', - onUncaughtException: (err: Error) => { - expect(err.message).toBe('no item available') - done() - }, - plugins: [plugin] - }, { - ...schema, - onUncaughtException: { - validate: (val: unknown) => typeof val === 'function', - message: 'should be a function', - defaultValue: () => {} - } - }) - c._setDelivery(client => ({ - sendEvent: (payload, cb) => { - cb(new Error('sending failed')) - }, - sendSession: () => {} - })) - const contextualize = c.getPlugin('contextualize') - contextualize(() => { - load(8, (err) => { - if (err) throw err - }) - }) - }) -}) diff --git a/packages/plugin-node-uncaught-exception/uncaught-exception.js b/packages/plugin-node-uncaught-exception/uncaught-exception.js index 8cd80d596d..27d535d541 100644 --- a/packages/plugin-node-uncaught-exception/uncaught-exception.js +++ b/packages/plugin-node-uncaught-exception/uncaught-exception.js @@ -1,3 +1,5 @@ +const { maybeUseFallbackStack } = require('@bugsnag/core/lib/node-fallback-stack') + let _handler module.exports = { load: client => { @@ -7,6 +9,10 @@ module.exports = { // if we are in an async context, use the client from that context const c = (client._clientContext && client._clientContext.getStore()) ? client._clientContext.getStore() : client + // check if the stacktrace has no context, if so, if so append the frames we created earlier + // see plugin-contextualize for where this is created + if (err.stack && c.fallbackStack) maybeUseFallbackStack(err, c.fallbackStack) + const event = c.Event.create(err, false, { severity: 'error', unhandled: true, diff --git a/test/node/features/fixtures/unhandled/scenarios/contextualize-unhandled-rejection.js b/test/node/features/fixtures/unhandled/scenarios/contextualize-unhandled-rejection.js new file mode 100644 index 0000000000..fa8ea3ca6a --- /dev/null +++ b/test/node/features/fixtures/unhandled/scenarios/contextualize-unhandled-rejection.js @@ -0,0 +1,15 @@ +var Bugsnag = require('@bugsnag/node') +Bugsnag.start({ + apiKey: process.env.BUGSNAG_API_KEY, + endpoints: { + notify: process.env.BUGSNAG_NOTIFY_ENDPOINT, + sessions: process.env.BUGSNAG_SESSIONS_ENDPOINT + } +}) + +var contextualize = Bugsnag.getPlugin('contextualize') +contextualize(function () { + Promise.reject(new Error('unhandled rejection')) +}, function (event) { + event.addMetadata('subsystem', { name: 'fs reader', widgetsAdded: 'cat,dog,mouse' }) +}) diff --git a/test/node/features/unhandled_errors.feature b/test/node/features/unhandled_errors.feature index 22aa6b1ea6..c323c55143 100644 --- a/test/node/features/unhandled_errors.feature +++ b/test/node/features/unhandled_errors.feature @@ -70,6 +70,35 @@ Scenario: using contextualize to add context to an error And the event "metaData.subsystem.name" equals "fs reader" And the event "metaData.subsystem.widgetsAdded" equals "cat,dog,mouse" +@skip_before_node_16 +Scenario: using contextualize with an unhandled rejection (with context added) + And I run the service "unhandled" with the command "node scenarios/contextualize-unhandled-rejection" + And I wait to receive an error + Then the error is valid for the error reporting API version "4" for the "Bugsnag Node" notifier + And the event "unhandled" is true + And the event "severity" equals "error" + And the event "severityReason.type" equals "unhandledPromiseRejection" + And the exception "errorClass" equals "Error" + And the exception "message" equals "unhandled rejection" + And the exception "type" equals "nodejs" + And the "file" of stack frame 0 equals "scenarios/contextualize-unhandled-rejection.js" + And the "lineNumber" of stack frame 0 equals 12 + And the event "metaData.subsystem.name" equals "fs reader" + And the event "metaData.subsystem.widgetsAdded" equals "cat,dog,mouse" + +Scenario: using contextualize with an unhandled rejection (no context added) + And I run the service "unhandled" with the command "node scenarios/contextualize-unhandled-rejection" + And I wait to receive an error + Then the error is valid for the error reporting API version "4" for the "Bugsnag Node" notifier + And the event "unhandled" is true + And the event "severity" equals "error" + And the event "severityReason.type" equals "unhandledPromiseRejection" + And the exception "errorClass" equals "Error" + And the exception "message" equals "unhandled rejection" + And the exception "type" equals "nodejs" + And the "file" of stack frame 0 equals "scenarios/contextualize-unhandled-rejection.js" + And the "lineNumber" of stack frame 0 equals 12 + Scenario: overridden handled state in a callback And I run the service "unhandled" with the command "node scenarios/modify-unhandled-callback" And I wait to receive an error From a572bd42291ff2529d0a64b03a5c41c64e4e09da Mon Sep 17 00:00:00 2001 From: Dan Skinner Date: Thu, 2 Mar 2023 15:41:45 +0000 Subject: [PATCH 2/6] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80c69b9f3f..69108a96b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ ### Changed - (plugin-navigation-breadcrumbs) calling `pushState` or `replaceState` no longer triggers a new session when `autoTrackSessions` is enabled [#1820](https://github.com/bugsnag/bugsnag-js/pull/1820) -- (plugin-contextualize) reimplement without relying on the deprecated node Domain API [#1924](https://github.com/bugsnag/bugsnag-js/pull/1924) +- (plugin-contextualize) reimplement without relying on the deprecated node Domain API. From Node 16+ unhandled promise rejections are also supported [#1924](https://github.com/bugsnag/bugsnag-js/pull/1924) ## TBD From 855c425cbf682a435fdb6e37b9f9a44967a86212 Mon Sep 17 00:00:00 2001 From: Dan Skinner Date: Thu, 2 Mar 2023 15:42:40 +0000 Subject: [PATCH 3/6] plugin-contextualize: always run in the async context --- packages/plugin-contextualize/contextualize.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/plugin-contextualize/contextualize.js b/packages/plugin-contextualize/contextualize.js index f9e2e3cc89..5a3671ef76 100644 --- a/packages/plugin-contextualize/contextualize.js +++ b/packages/plugin-contextualize/contextualize.js @@ -17,11 +17,7 @@ module.exports = { clonedClient.addOnError(onError) - if (client._clientContext) { - client._clientContext.run(clonedClient, fn) - } else { - fn() - } + client._clientContext.run(clonedClient, fn) } return contextualize From e6a91d3d516365625ed2291e5d7af08b993f8422 Mon Sep 17 00:00:00 2001 From: Dan Skinner Date: Thu, 2 Mar 2023 16:44:04 +0000 Subject: [PATCH 4/6] change expectations on aws-lambda test --- test/aws-lambda/features/promise-rejection.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/aws-lambda/features/promise-rejection.feature b/test/aws-lambda/features/promise-rejection.feature index ba35a91f8d..034f16466e 100644 --- a/test/aws-lambda/features/promise-rejection.feature +++ b/test/aws-lambda/features/promise-rejection.feature @@ -42,7 +42,7 @@ Scenario Outline: unhandled promise rejections are not reported when autoDetectE When I invoke the "" lambda in "features/fixtures/simple-app" with the "events//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 And the lambda response "trace.0" equals "Runtime.UnhandledPromiseRejection: Error: yikes" And the lambda response "body" is null And the lambda response "statusCode" is null From e87ecafb24c72f6ee0e8a3e5e5ed9cdb1f0aff9f Mon Sep 17 00:00:00 2001 From: Dan Skinner Date: Mon, 6 Mar 2023 11:22:30 +0000 Subject: [PATCH 5/6] change expectations on aws-lambda test --- test/aws-lambda/features/promise-rejection.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/aws-lambda/features/promise-rejection.feature b/test/aws-lambda/features/promise-rejection.feature index 034f16466e..ba35a91f8d 100644 --- a/test/aws-lambda/features/promise-rejection.feature +++ b/test/aws-lambda/features/promise-rejection.feature @@ -42,7 +42,7 @@ Scenario Outline: unhandled promise rejections are not reported when autoDetectE When I invoke the "" lambda in "features/fixtures/simple-app" with the "events//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 5 elements + And the lambda response "trace" is an array with 6 elements And the lambda response "trace.0" equals "Runtime.UnhandledPromiseRejection: Error: yikes" And the lambda response "body" is null And the lambda response "statusCode" is null From a18b793595fe202423cc4a04d99700c0b07afcb9 Mon Sep 17 00:00:00 2001 From: Dan Skinner Date: Mon, 6 Mar 2023 14:16:29 +0000 Subject: [PATCH 6/6] change expectations on aws-lambda test --- test/aws-lambda/features/promise-rejection.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/aws-lambda/features/promise-rejection.feature b/test/aws-lambda/features/promise-rejection.feature index ba35a91f8d..034f16466e 100644 --- a/test/aws-lambda/features/promise-rejection.feature +++ b/test/aws-lambda/features/promise-rejection.feature @@ -42,7 +42,7 @@ Scenario Outline: unhandled promise rejections are not reported when autoDetectE When I invoke the "" lambda in "features/fixtures/simple-app" with the "events//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 And the lambda response "trace.0" equals "Runtime.UnhandledPromiseRejection: Error: yikes" And the lambda response "body" is null And the lambda response "statusCode" is null