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

V7: Breadcrumb refactor #650

Merged
merged 6 commits into from
Nov 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- Rename `autoCaptureSessions` -> `autoTrackSessions` and simplify validation logic [#647](https://github.com/bugsnag/bugsnag-js/pull/647)
- Rename `report` to `event` [#646](https://github.com/bugsnag/bugsnag-js/pull/646)
- Rename `notifyReleaseStages` -> `enabledReleaseStages` [#649](https://github.com/bugsnag/bugsnag-js/pull/649)
- Remove individual breadcrumb flags in favour of `enabledBreadcrumbTypes`, rename `breadcrumb.{ name -> message, metaData -> metadata }`, and update `leaveBreadcrumb()` type signature to be more explicit [#650](https://github.com/bugsnag/bugsnag-js/pull/649)

## 6.4.3 (2019-10-21)

Expand Down
6 changes: 1 addition & 5 deletions packages/browser/types/bugsnag.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ declare module "@bugsnag/core" {
interface Config {
apiKey: string;
beforeSend?: BugsnagCore.BeforeSend | BugsnagCore.BeforeSend[];
autoBreadcrumbs?: boolean;
autoDetectErrors?: boolean;
autoDetectUnhandledRejections?: boolean;
enabledBreadcrumbTypes?: BugsnagCore.BreadcrumbType[] | null;
appVersion?: string;
appType?: string;
endpoints?: { notify: string; sessions?: string };
Expand All @@ -23,10 +23,6 @@ declare module "@bugsnag/core" {
[key: string]: any;
// options for all bundled browser plugins
maxEvents?: number;
consoleBreadcrumbsEnabled?: boolean;
networkBreadcrumbsEnabled?: boolean;
navigationBreadcrumbsEnabled?: boolean;
interactionBreadcrumbsEnabled?: boolean;
collectUserIp?: boolean;
}
}
Expand Down
6 changes: 1 addition & 5 deletions packages/browser/types/test/fixtures/all-options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,11 @@ bugsnag({
enabledReleaseStages: [],
releaseStage: "production",
maxBreadcrumbs: 20,
autoBreadcrumbs: true,
enabledBreadcrumbTypes: ['manual','log','request'],
user: null,
metaData: null,
logger: undefined,
filters: ["foo",/bar/],
collectUserIp: true,
consoleBreadcrumbsEnabled: undefined,
interactionBreadcrumbsEnabled: undefined,
navigationBreadcrumbsEnabled: undefined,
networkBreadcrumbsEnabled: undefined,
maxEvents: 10
})
4 changes: 2 additions & 2 deletions packages/browser/types/test/fixtures/breadcrumb-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ const bugsnagClient = bugsnag({
beforeSend: (event) => {
event.breadcrumbs.map(breadcrumb => {
console.log(breadcrumb.type)
console.log(breadcrumb.name)
console.log(breadcrumb.metaData)
console.log(breadcrumb.message)
console.log(breadcrumb.metadata)
console.log(breadcrumb.timestamp)
})
}
Expand Down
10 changes: 5 additions & 5 deletions packages/core/breadcrumb.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
const { isoDate } = require('./lib/es-utils')

class BugsnagBreadcrumb {
constructor (name = '[anonymous]', metaData = {}, type = 'manual', timestamp = isoDate()) {
constructor (message, metadata, type, timestamp = isoDate()) {
Copy link
Contributor Author

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.

this.type = type
this.name = name
this.metaData = metaData
this.message = message
this.metadata = metadata
this.timestamp = timestamp
}

toJSON () {
return {
type: this.type,
name: this.name,
name: this.message,
timestamp: this.timestamp,
metaData: this.metaData
metaData: this.metadata
}
}
}
Expand Down
30 changes: 15 additions & 15 deletions packages/core/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,19 +124,21 @@ class BugsnagClient {
return this._sessionDelegate.startSession(this)
}

leaveBreadcrumb (name, metaData, type, timestamp) {
leaveBreadcrumb (message, metadata, type) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

timestamp is not in the public method signature according to the new spec

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
// check the breadcrumb is the list of enabled types
if (!this.config.enabledBreadcrumbTypes || !includes(this.config.enabledBreadcrumbTypes, type)) return

const crumb = new BugsnagBreadcrumb(message, metadata, type)

// push the valid crumb onto the queue and maintain the length
this.breadcrumbs.push(crumb)
Expand Down Expand Up @@ -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, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Node's notifier monkey-patches the leaveBreadcrumb() method to log a warning that breadcrumbs aren't supported.

Calling the method via the prototype ensures "internal" calls to leaveBreadcrumb() don't trigger the log. I can explain more if this doesn't make sense!

errorClass: event.errorClass,
errorMessage: event.errorMessage,
severity: event.severity
}, 'error')

if (originalSeverity !== event.severity) {
event._handledState.severityReason = { type: 'userCallbackSetSeverity' }
Expand Down
13 changes: 9 additions & 4 deletions packages/core/config.js
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,
Expand Down Expand Up @@ -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(value, (accum, maybeType) => {
if (accum === false) return accum
return includes(BREADCRUMB_TYPES, maybeType)
}, true))
},
user: {
defaultValue: () => null,
Expand Down
19 changes: 7 additions & 12 deletions packages/core/test/client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,8 +321,8 @@ describe('@bugsnag/core/client', () => {
client.notify(new Error('foobar'))
expect(client.breadcrumbs.length).toBe(1)
expect(client.breadcrumbs[0].type).toBe('error')
expect(client.breadcrumbs[0].name).toBe('Error')
expect(client.breadcrumbs[0].metaData.stacktrace).toBe(undefined)
expect(client.breadcrumbs[0].message).toBe('Error')
expect(client.breadcrumbs[0].metadata.stacktrace).toBe(undefined)
// the error shouldn't appear as a breadcrumb for itself
expect(payloads[0].events[0].breadcrumbs.length).toBe(0)
})
Expand Down Expand Up @@ -431,8 +431,8 @@ describe('@bugsnag/core/client', () => {
client.leaveBreadcrumb('french stick')
expect(client.breadcrumbs.length).toBe(1)
expect(client.breadcrumbs[0].type).toBe('manual')
expect(client.breadcrumbs[0].name).toBe('french stick')
expect(client.breadcrumbs[0].metaData).toEqual({})
expect(client.breadcrumbs[0].message).toBe('french stick')
expect(client.breadcrumbs[0].metadata).toEqual({})
})

it('caps the length of breadcrumbs at the configured limit', () => {
Expand All @@ -447,27 +447,22 @@ describe('@bugsnag/core/client', () => {
expect(client.breadcrumbs.length).toBe(3)
client.leaveBreadcrumb('seedy farmhouse')
expect(client.breadcrumbs.length).toBe(3)
expect(client.breadcrumbs.map(b => b.name)).toEqual([
expect(client.breadcrumbs.map(b => b.message)).toEqual([
'medium sliced white hovis',
'pumperninkel',
'seedy farmhouse'
])
})

it('doesn’t add the breadcrumb if it didn’t contain anything useful', () => {
it('doesn’t add the breadcrumb if it didn’t contain a message', () => {
const client = new Client(VALID_NOTIFIER)
client.setOptions({ apiKey: 'API_KEY_YEAH' })
client.configure()
client.leaveBreadcrumb(undefined)
client.leaveBreadcrumb(null, { data: 'is useful' })
client.leaveBreadcrumb(null, {}, null)
client.leaveBreadcrumb(null, { t: 10 }, null, 4)
expect(client.breadcrumbs.length).toBe(3)
expect(client.breadcrumbs[0].type).toBe('manual')
expect(client.breadcrumbs[0].name).toBe('[anonymous]')
expect(client.breadcrumbs[0].metaData).toEqual({ data: 'is useful' })
expect(client.breadcrumbs[1].type).toBe('manual')
expect(typeof client.breadcrumbs[2].timestamp).toBe('string')
expect(client.breadcrumbs.length).toBe(0)
})

it('allows maxBreadcrumbs to be set to 0', () => {
Expand Down
7 changes: 7 additions & 0 deletions packages/core/test/config.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,11 @@ describe('@bugsnag/core/config', () => {
})
})
})

describe('enabledBreadcrumbTypes', () => {
it('fails when a supplied value is not a valid breadcrumb type', () => {
const enabledBreadcrumbTypesValidator = config.schema.enabledBreadcrumbTypes.validate
expect(enabledBreadcrumbTypesValidator(['UNKNOWN_BREADCRUMB_TYPE'])).toBe(false)
})
})
})
5 changes: 2 additions & 3 deletions packages/core/types/breadcrumb.d.ts
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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breadcrumb construction is not public, it can only be done via leaveBreadcrumb().

}

export default Breadcrumb;
2 changes: 1 addition & 1 deletion packages/core/types/client.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ declare class Client {
opts?: common.NotifyOpts,
cb?: (err: any, event: Event) => void,
): void;
public leaveBreadcrumb(name: string, metaData?: any, type?: string, timestamp?: string): Client;
public leaveBreadcrumb(message: string, metadata?: { [key: string]: common.BreadcrumbMetadataValue }, type?: string): Client;
public startSession(): Client;
}

Expand Down
7 changes: 6 additions & 1 deletion packages/core/types/common.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import Event from "./event";
export interface Config {
apiKey: string;
beforeSend?: BeforeSend | BeforeSend[];
autoBreadcrumbs?: boolean;
enabledBreadcrumbTypes?: BreadcrumbType[];
autoDetectErrors?: boolean;
autoDetectUnhandledRejections?: boolean;
appVersion?: string;
Expand Down Expand Up @@ -108,3 +108,8 @@ export type NotifiableError = Error
| { errorClass: string; errorMessage: string }
| { name: string; message: string }
| any;

type Primitive = boolean | string | number | undefined | null;
export type BreadcrumbMetadataValue = Primitive | Array<Primitive>;

export type BreadcrumbType = "error" | "log" | "manual" | "navigation" | "process" | "request" | "state" | "user";
9 changes: 1 addition & 8 deletions packages/expo/types/bugsnag.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ declare module "@bugsnag/core" {
interface Config {
apiKey?: string;
beforeSend?: BugsnagCore.BeforeSend | BugsnagCore.BeforeSend[];
autoBreadcrumbs?: boolean;
enabledBreadcrumbTypes?: BugsnagCore.BreadcrumbType[];
autoDetectErrors?: boolean;
autoDetectUnhandledRejections?: boolean;
appVersion?: string;
Expand All @@ -21,13 +21,6 @@ declare module "@bugsnag/core" {
filters?: Array<string | RegExp>;
// catch-all for any missing options
[key: string]: any;
// options for all bundled expo plugins
appStateBreadcrumbsEnabled?: boolean;
consoleBreadcrumbsEnabled?: boolean;
networkBreadcrumbsEnabled?: boolean;
navigationBreadcrumbsEnabled?: boolean;
connectivityBreadcrumbsEnabled?: boolean;
orientationBreadcrumbsEnabled?: boolean;
}
}

Expand Down
6 changes: 1 addition & 5 deletions packages/expo/types/test/fixtures/all-options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,11 @@ bugsnag({
enabledReleaseStages: ['zzz'],
releaseStage: "production",
maxBreadcrumbs: 20,
autoBreadcrumbs: true,
enabledBreadcrumbTypes: ['manual','log','request'],
user: null,
metaData: null,
logger: undefined,
filters: ["foo",/bar/],
collectUserIp: true,
consoleBreadcrumbsEnabled: undefined,
interactionBreadcrumbsEnabled: undefined,
navigationBreadcrumbsEnabled: undefined,
networkBreadcrumbsEnabled: undefined,
maxEvents: 10
})
4 changes: 2 additions & 2 deletions packages/expo/types/test/fixtures/breadcrumb-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ const bugsnagClient = bugsnag({
beforeSend: (event) => {
event.breadcrumbs.map(breadcrumb => {
console.log(breadcrumb.type)
console.log(breadcrumb.name)
console.log(breadcrumb.metaData)
console.log(breadcrumb.message)
console.log(breadcrumb.metadata)
console.log(breadcrumb.timestamp)
})
}
Expand Down
4 changes: 2 additions & 2 deletions packages/node/src/notifier.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ const delivery = require('@bugsnag/delivery-node')
// extend the base config schema with some node-specific options
const schema = { ...require('@bugsnag/core/config').schema, ...require('./config') }

// remove autoBreadcrumbs from the config schema
delete schema.autoBreadcrumbs
// remove enabledBreadcrumbTypes from the config schema
delete schema.enabledBreadcrumbTypes

const pluginSurroundingCode = require('@bugsnag/plugin-node-surrounding-code')
const pluginInProject = require('@bugsnag/plugin-node-in-project')
Expand Down
4 changes: 2 additions & 2 deletions packages/node/types/bugsnag.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ declare module "@bugsnag/core" {
interface Config {
apiKey: string;
beforeSend?: BugsnagCore.BeforeSend | BugsnagCore.BeforeSend[];
// autoBreadcrumbs?: boolean; // this option is disabled in node, see below
autoDetectErrors?: boolean;
autoDetectUnhandledRejections?: boolean;
appVersion?: string;
Expand All @@ -30,7 +29,8 @@ declare module "@bugsnag/core" {
agent?: any;
projectRoot?: string;
sendCode?: boolean;
autoBreadcrumbs?: void;
// breadcrumbs are disabled in Node
enabledBreadcrumbTypes?: void;
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/plugin-console-breadcrumbs/README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# @bugsnag/plugin-console-breadcrumbs

This plugin adds the ability to record console method calls as breadcrumbs by monkey patching them. It defines a configuration option `consoleBreadcrumbsEnabled` which can be used to disable the functionality. It is included in the browser notifier.
This plugin adds the ability to record console method calls as breadcrumbs by monkey patching them. It is included in the browser notifier.

**Note:** enabling this plugin means that the line/col origin of console logs appear to come from within Bugsnag's code, so it is not recommended for use in dev environments.

Expand Down
14 changes: 2 additions & 12 deletions packages/plugin-console-breadcrumbs/console-breadcrumbs.js
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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]
Expand Down Expand Up @@ -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()
Expand Down