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

Load locals as module, with enable/disable and uncaughtOnly options #925

Merged
merged 6 commits into from
Feb 16, 2021

Conversation

waltjones
Copy link
Contributor

@waltjones waltjones commented Feb 12, 2021

Description of the change

  • Loads the Locals class as a separate module, so that build environments like vercel/pkg that don't have Inspector can build successfully. (See notes below)
  • Adds an enable flag that can be toggled dynamically at runtime.
  • Adds an uncaughtOnly flag that can be toggled dynamically at runtime.

Using the enable and uncaughtOnly flags at runtime can allow capturing locals for specific code paths or time intervals, while avoiding perf impact at other times.

Usage example:

const Rollbar = require('rollbar');
const Locals = require('rollbar/src/server/locals');

const rollbar = Rollbar.init({
  accessToken: 'server-token',
  captureUncaught: true,
  captureUnhandledRejections: true,
  locals: { module: Locals, enable: true, uncaughtOnly: true }
});

Notes:

To support non-Node build environments (e.g. vercel/pkg), a try/catch around the require('inspector') was considered.

try {
  require('inspector);
} catch { /* ... */ }

This would allow these environments to build successfully, without needing a separate Locals module. But since the require('inspector) might lead to other possible conflicts, even when the locals feature isn't enabled, loading the module separately looked like the most reliably safe option.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Fixes: #921

ch79594

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

@mrunalk mrunalk self-requested a review February 12, 2021 22:39
Copy link
Contributor

@bxsx bxsx left a comment

Choose a reason for hiding this comment

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

Looks good!
Few minor comments from my side :)

test/server.locals.test.js Outdated Show resolved Hide resolved
test/server.locals.test.js Outdated Show resolved Hide resolved
test/server.locals.test.js Outdated Show resolved Hide resolved
test/server.locals.test.js Outdated Show resolved Hide resolved
src/server/locals.js Outdated Show resolved Hide resolved
Copy link
Contributor

@bxsx bxsx left a comment

Choose a reason for hiding this comment

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

No further comments at the moment. Thanks for all changes (and sorry for one buggy suggestion, good you caught it!).

Looks good to me now.

@waltjones waltjones merged commit bf178f7 into master Feb 16, 2021
mudetroit pushed a commit that referenced this pull request Mar 14, 2024
…925)

* feat: load locals as module, with enable/disable and uncaughtOnly options

* fix: rename setPauseState -> pauseStateChanged and add conditional around log

* Update test/server.locals.test.js

Co-authored-by: Bart Skowron <[email protected]>

* Update test/server.locals.test.js

Co-authored-by: Bart Skowron <[email protected]>

* Update test/server.locals.test.js

Co-authored-by: Bart Skowron <[email protected]>

* test: reverse conditional

Co-authored-by: Bart Skowron <[email protected]>
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.

Inspector is not available
2 participants