Skip to content
This repository has been archived by the owner on Dec 4, 2018. It is now read-only.

Cawllec/fix shouldnt notify logging #113

Merged
merged 9 commits into from
Sep 1, 2017
Merged

Cawllec/fix shouldnt notify logging #113

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
41b201c
Added default disabling of notification errors if not shouldNotify an…
Cawllec Aug 31, 2017
8871360
Added default disabling of notification errors if not shouldNotify an…
Cawllec Aug 31, 2017
6ca0fa6
Merge branch 'cawllec/fix-shouldnt-notify-logging' of github.com:bugs…
Cawllec Aug 31, 2017
e302522
Updated ts interface
Cawllec Aug 31, 2017
b094305
Removed extra config
Cawllec Aug 31, 2017
1f9c0df
Added original error logging if not shouldnotify
Cawllec Aug 31, 2017
a82fbb2
Removed config opt from ts
Cawllec Aug 31, 2017
b136ad8
Better error logging
Cawllec Sep 1, 2017
cbc9e51
Updated text, better error information logging
Cawllec Sep 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/bugsnag.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ declare namespace bugsnag {
projectRoot?: string;
packageJSON?: string;
sendCode?: boolean;
logAllDeliveryErrors?: boolean;
}

interface NotifyOptions {
Expand Down
13 changes: 12 additions & 1 deletion lib/bugsnag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,17 @@ Bugsnag.register = function(apiKey, options) {
Bugsnag.configure = function(options) {
Configuration.configure(options);

// If notify release stages is set and we're not going to be notifying bugsnag we need to log
// this once and then prevent it from being logged in the future
if (!Bugsnag.shouldNotify()) {
Configuration.logger.warn(Configuration.apiKey ?
"Current release stage not permitted to send events to Bugsnag." :
"Bugsnag has not been configured with an api key!");
Configuration.logger.warn(Configuration.logAllDeliveryErrors ?
"Delivery errors will still be reported" :
"Delivery errors will not be reported. Configure logAllDeliveryErrors to change this behaviour");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these log messages are confusing and could be worded better…

Something along the lines of:
"Calls to bugsnag.notify() will silently fail"
or
"Nothing will be sent to Bugsnag. Any bugsnag.notify() calls are essentially NOOPs."

}

// If we should auto notify we also configure the uncaught exception handler, we can't do this
// by default as it changes the way the app response by removing the default handler.
if (Configuration.autoNotifyUncaught && !unCaughtErrorHandlerAdded) {
Expand All @@ -92,7 +103,7 @@ Bugsnag.notify = function(error, options, cb) {
options = {};
}
if (!Bugsnag.shouldNotify()) {
if (cb) {
if (cb && Configuration.logAllDeliveryErrors) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this branch could be in a more appropriate place, as the the option mentions log but no logging is done here (an Error object is created and passed to the cb()).

A better place would be in the definition of autoNotifyCallback.

if (!Configuration.apiKey) {
cb(new Error("Bugsnag has not been configured with an api key!"));
} else {
Expand Down
2 changes: 2 additions & 0 deletions lib/configuration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var Configuration = {
metaData: {},
logger: new Logger(),
sendCode: true,
logAllDeliveryErrors: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this option might want changing to be consistent with the log message.


beforeNotifyCallbacks: [],

Expand Down Expand Up @@ -61,6 +62,7 @@ var Configuration = {
Configuration.metaData = options.metaData || Configuration.metaData;
Configuration.onUncaughtError = options.onUncaughtError || Configuration.onUncaughtError;
Configuration.hostname = options.hostname || Configuration.hostname;
Configuration.logAllDeliveryErrors = options.logAllDeliveryErrors || Configuration.logAllDeliveryErrors;
Configuration.proxy = options.proxy;
Configuration.headers = options.headers;
if (options.projectRoot != null) {
Expand Down