-
Notifications
You must be signed in to change notification settings - Fork 251
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
V7: Breadcrumb refactor #650
Changes from 4 commits
7eb6e3c
8689f42
4fab27e
49a4658
fa55906
b245c79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,19 +124,21 @@ class BugsnagClient { | |
return this._sessionDelegate.startSession(this) | ||
} | ||
|
||
leaveBreadcrumb (name, metaData, type, timestamp) { | ||
leaveBreadcrumb (message, metadata, type) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if (!this._configured) throw new Error('client not configured') | ||
|
||
// coerce bad values so that the defaults get set | ||
name = name || undefined | ||
type = typeof type === 'string' ? type : undefined | ||
timestamp = typeof timestamp === 'string' ? timestamp : undefined | ||
metaData = typeof metaData === 'object' && metaData !== null ? metaData : undefined | ||
message = typeof message === 'string' ? message : '' | ||
type = typeof type === 'string' ? type : 'manual' | ||
metadata = typeof metadata === 'object' && metadata !== null ? metadata : {} | ||
|
||
// if no name and no metaData, usefulness of this crumb is questionable at best so discard | ||
if (typeof name !== 'string' && !metaData) return | ||
// if no message, discard | ||
if (!message) return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The spec doesn't allow a breadcrumb without a message, so we are more strict about that now. |
||
|
||
const crumb = new BugsnagBreadcrumb(name, metaData, type, timestamp) | ||
const crumb = new BugsnagBreadcrumb(message, metadata, type) | ||
|
||
// check the breadcrumb is the list of enabled types | ||
if (!this.config.enabledBreadcrumbTypes || !includes(this.config.enabledBreadcrumbTypes, crumb.type)) return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worthwhile doing this check before instantiating a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah good point. We previously did it after because the default |
||
|
||
// push the valid crumb onto the queue and maintain the length | ||
this.breadcrumbs.push(crumb) | ||
|
@@ -205,13 +207,11 @@ class BugsnagClient { | |
} | ||
|
||
// only leave a crumb for the error if actually got sent | ||
if (this.config.autoBreadcrumbs) { | ||
this.leaveBreadcrumb(event.errorClass, { | ||
errorClass: event.errorClass, | ||
errorMessage: event.errorMessage, | ||
severity: event.severity | ||
}, 'error') | ||
} | ||
BugsnagClient.prototype.leaveBreadcrumb.call(this, event.errorClass, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Node's notifier monkey-patches the Calling the method via the prototype ensures "internal" calls to |
||
errorClass: event.errorClass, | ||
errorMessage: event.errorMessage, | ||
severity: event.severity | ||
}, 'error') | ||
|
||
if (originalSeverity !== event.severity) { | ||
event._handledState.severityReason = { type: 'userCallbackSetSeverity' } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
const { filter, reduce, keys, isArray, includes } = require('./lib/es-utils') | ||
const { intRange, stringWithLength } = require('./lib/validators') | ||
|
||
const BREADCRUMB_TYPES = ['navigation', 'request', 'process', 'log', 'user', 'state', 'error', 'manual'] | ||
|
||
module.exports.schema = { | ||
apiKey: { | ||
defaultValue: () => null, | ||
|
@@ -68,10 +70,13 @@ module.exports.schema = { | |
message: 'should be a number ≤40', | ||
validate: value => intRange(0, 40)(value) | ||
}, | ||
autoBreadcrumbs: { | ||
defaultValue: () => true, | ||
message: 'should be true|false', | ||
validate: (value) => typeof value === 'boolean' | ||
enabledBreadcrumbTypes: { | ||
defaultValue: () => BREADCRUMB_TYPES, | ||
message: `should be null or a list of available breadcrumb types (${BREADCRUMB_TYPES.join(',')})`, | ||
validate: value => value === null || (isArray(value) && reduce(BREADCRUMB_TYPES, (accum, maybeType) => { | ||
if (accum === false) return accum | ||
return includes(BREADCRUMB_TYPES, maybeType) | ||
}, true)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this validation logic is wrong. Consider the following failing test case:
Gives There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the failing test case 🙌 I added it to the codebase and fixed the bug (I was iterating over |
||
}, | ||
user: { | ||
defaultValue: () => null, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,8 @@ | ||
declare class Breadcrumb { | ||
public name: string; | ||
public metaData: object; | ||
public message: string; | ||
public metadata: object; | ||
public type: string; | ||
public timestamp: string; | ||
constructor(name: string, metaData?: object, type?: string, timestamp?: string); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Breadcrumb construction is not public, it can only be done via |
||
} | ||
|
||
export default Breadcrumb; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,12 @@ | ||
const { map, reduce, filter } = require('@bugsnag/core/lib/es-utils') | ||
const { map, reduce, filter, includes } = require('@bugsnag/core/lib/es-utils') | ||
|
||
/* | ||
* Leaves breadcrumbs when console log methods are called | ||
*/ | ||
exports.init = (client) => { | ||
const isDev = /^dev(elopment)?$/.test(client.config.releaseStage) | ||
|
||
const explicitlyDisabled = client.config.consoleBreadcrumbsEnabled === false | ||
const implicitlyDisabled = (client.config.autoBreadcrumbs === false || isDev) && client.config.consoleBreadcrumbsEnabled !== true | ||
if (explicitlyDisabled || implicitlyDisabled) return | ||
if (!client.config.enabledBreadcrumbTypes || !includes(client.config.enabledBreadcrumbTypes, 'log') || isDev) return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change has the known impact of meaning console log breadcrumbs can't be enabled in development. |
||
|
||
map(CONSOLE_LOG_METHODS, method => { | ||
const original = console[method] | ||
|
@@ -36,14 +34,6 @@ exports.init = (client) => { | |
}) | ||
} | ||
|
||
exports.configSchema = { | ||
consoleBreadcrumbsEnabled: { | ||
defaultValue: () => undefined, | ||
validate: (value) => value === true || value === false || value === undefined, | ||
message: 'should be true|false' | ||
} | ||
} | ||
|
||
if (process.env.NODE_ENV !== 'production') { | ||
exports.destroy = () => CONSOLE_LOG_METHODS.forEach(method => { | ||
if (typeof console[method]._restore === 'function') console[method]._restore() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These default values have been removed in favour of the validation/default values being done in the
client.leaveBreadcrumb()
method rather than being split between there and here.