Cawllec/fix shouldnt notify logging

[Cawllec]

Disables delivery errors in the case that shouldNotify is false, adds configuration option to overwrite this behaviour if necessary

[bengourley]

In addition to the inline comments I'm also concerned about the following issues:

• The default log level is "error" so calls logger.warn are likely to be missed, unless the user has set their own log level. We could change the default log level to be "warn" or call logger.error with this message. Both of which seem ugly but would solve the problem.

• I'm not sure if this PR fixes the user's actual issue which was that errors were masked in development (if I interpreted it correctly). This is because the errors are still caught by the process.uncaughtException handler but then nothing is ever logged out. In short I think they still want a log message in development, but they want the log message of the caught error, not the refusal to deliver to Bugsnag due to configuration.

[bengourley]

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.

[bengourley]

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."

[bengourley]

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

[Cawllec]

Based on discussion with @bengourley I've removed the changes based around the configuration options, and instead ensured that when an error occurs and is correctly not-notified the original error is logged instead of the notification error.

This still leaves the issue with the initial log level however.

[bengourley]

This is better!

Here is how the user's repro repo now behaves:

I think it would be good if the error stack got into the log and looked more like how uncaught errors get logged out:

appending e.stack to the log message should do the trick?

[Cawllec]

Appended that stack as suggested and removed the Bugsnag: repetition.

I notice that it's logging each issue twice, any idea why?

[bengourley]

notifiedError will be coerced into a string, which will contain the Error: <message> bit, but then the string notifiedError.stack also contains Error: <message> followed by each stack line.

In summary you only need .stack and not both. This goes for line 23 and 24.

I've had a tinker and coming at it from the user's perspective, I think the best output would be something like this…

Bugsnag:  configuration prevents the following error from being sent to Bugsnag.
https://docs.bugsnag.com/platforms/nodejs/other/configuration-options/#notifyreleasestages

Error: Some error occured
    at app.use (/Users/bengourley/Development/bugsnag-bug-repro/index.js:13:11)
    at dispatch (/Users/bengourley/Development/bugsnag-bug-repro/node_modules/koa-compose/index.js:42:32)
    at /Users/bengourley/Development/bugsnag-bug-repro/node_modules/koa-compose/index.js:34:12
    at Server.handleRequest (/Users/bengourley/Development/bugsnag-bug-repro/node_modules/koa/lib/application.js:136:14)
    at emitTwo (events.js:125:13)
    at Server.emit (events.js:213:7)
    at parserOnIncoming (_http_server.js:602:12)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:116:23)

Which you should be able to do with something like…

[
  "configuration prevents the following error from being sent to Bugsnag.",
  "https://docs.bugsnag.com/platforms/nodejs/other/configuration-options/#notifyreleasestages",
  "",
  err.stack
].join('\n')

[bengourley]

LGTM 👍