enable breadcrumbs and context-scoped calls for node #1927
Conversation
djskinner
force-pushed
the
node-breadcrumbs
branch
6 times, most recently
from
20015fd
to
550caa4
Compare
djskinner
force-pushed
the
node-breadcrumbs
branch
from
550caa4
to
000c454
Compare
There was a problem hiding this comment.
Changes look good, but I think we need to test the APIs we're changing here — adding some unit tests to notifier.test.ts for Bugsnag.addMetadata & friends should be fine
the previous version suffered from loss of AsyncLocalStorage context so was updated to show the Bugsnag.addFeatureFlag capabilities working inside a request context. Further coverage will be considered separately for the context-loss cases.
| const runInContext = (req, res, next) => { | ||
| client._clientContext.run(req.bugsnag, next) | ||
| } |
There was a problem hiding this comment.
This is a new express middleware that can be used to restore async local storage context if it is lost. See E2E tes showing usage
| bugsnag.leaveBreadcrumb = function () { | ||
| bugsnag._logger.warn('Breadcrumbs are not supported in Node.js yet') | ||
| } | ||
|
|
||
| bugsnag._config.enabledBreadcrumbTypes = [] |
There was a problem hiding this comment.
remove the patches that were preventing breadcrumbs from being used with the node notifier
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "bugsnag-test", | |||
| "dependencies": { | |||
| "body-parser": "^1.19.0", | |||
| "body-parser": "^1.20.2", | |||
There was a problem hiding this comment.
Bumping this to fix the context-loss present in the older versions
| Bugsnag.addFeatureFlags(featureFlags) | ||
| Bugsnag.clearFeatureFlag('from config 3') |
There was a problem hiding this comment.
This was failing due to the context loss from doing bodyParser.urlencoded() with the older version of the dependency. Bumping fixes it. Could also have used middleware.runInContext to restore it
| @@ -55,50 +55,6 @@ Scenario: not reporting unhandledRejections when autoDetectErrors is off | |||
| And I wait for 1 second | |||
| Then I should receive no requests | |||
|
|
|||
| Scenario: using contextualize to add context to an error | |||
There was a problem hiding this comment.
moved to the new contextualize suite
Goal
Design
leaveBreadcrumbthat prevented usage of breadcrumbs on the main client instance. Note cloned instances such as those inside a request handler didn't have this limitation though such usage was undocumented.Bugsnag.leaveBreadrumborBugsnag.addFeatureFlagfrom within an AsyncLocalStorage context (e.g. when using the express plugin or plugin-contextualize) forward the method onto the cloned client instance thereby achieving scoped context without having to pass the cloned instance of the client (e.g.req.bugsnag) around.runInContextmiddleware for express that can be used to re-establish the AsyncLocalStorage context which can be lost in some scenarios (where such context loss prevents the correct operation ofBugsnag.calls, such asBugsnag.leaveBreadcrumb)Testing