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

✨ [RUMF-922] stack trace on handled calls #889

Merged
merged 12 commits into from
Jun 17, 2021
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/core/src/domain/automaticErrorCollection.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,28 @@ describe('console tracker', () => {
...CONSOLE_CONTEXT,
message: 'console error: foo bar',
stack: undefined,
handlingStack: jasmine.any(String),
startClocks: jasmine.any(Object),
handling: ErrorHandling.HANDLED,
})
})

it('should generate a handling stack', () => {
function triggerError() {
console.error('foo', 'bar')
}
triggerError()
const rawError = notifyError.calls.mostRecent().args[0] as RawError
expect(rawError.handlingStack).toMatch(/^Error:\s+at triggerError (.|\n)*$/)
})

it('should stringify object parameters', () => {
console.error('Hello', { foo: 'bar' })
expect(notifyError).toHaveBeenCalledWith({
...CONSOLE_CONTEXT,
message: 'console error: Hello {\n "foo": "bar"\n}',
stack: undefined,
handlingStack: jasmine.any(String),
startClocks: jasmine.any(Object),
handling: ErrorHandling.HANDLED,
})
Expand Down
27 changes: 17 additions & 10 deletions packages/core/src/domain/automaticErrorCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ import {
toStackTraceString,
formatErrorMessage,
ErrorHandling,
createHandlingStack,
} from '../tools/error'
import { Observable } from '../tools/observable'
import { clocksNow } from '../tools/timeUtils'
import { jsonStringify, RequestType, find } from '../tools/utils'
import { Configuration } from './configuration'
import { monitor } from './internalMonitoring'
import { callMonitored } from './internalMonitoring'
import { computeStackTrace, subscribe, unsubscribe, StackTrace } from './tracekit'

export type ErrorObservable = Observable<RawError>
Expand All @@ -33,27 +34,33 @@ let originalConsoleError: (...params: unknown[]) => void
/* eslint-disable no-console */
export function startConsoleTracking(errorObservable: ErrorObservable) {
originalConsoleError = console.error
console.error = monitor((...params: unknown[]) => {
originalConsoleError.apply(console, params)
errorObservable.notify({
...buildErrorFromParams(params),
source: ErrorSource.CONSOLE,
startClocks: clocksNow(),
handling: ErrorHandling.HANDLED,

console.error = (...params: unknown[]) => {
const handlingStack = createHandlingStack()
callMonitored(() => {
originalConsoleError.apply(console, params)
errorObservable.notify({
...buildErrorFromParams(params, handlingStack),
source: ErrorSource.CONSOLE,
startClocks: clocksNow(),
handling: ErrorHandling.HANDLED,
})
})
})
}
}

export function stopConsoleTracking() {
console.error = originalConsoleError
}
/* eslint-enable no-console */

function buildErrorFromParams(params: unknown[]) {
function buildErrorFromParams(params: unknown[], handlingStack: string) {
const firstErrorParam = find(params, (param: unknown): param is Error => param instanceof Error)

return {
message: ['console error:', ...params].map((param) => formatConsoleParameters(param)).join(' '),
stack: firstErrorParam ? toStackTraceString(computeStackTrace(firstErrorParam)) : undefined,
handlingStack,
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export {
BeforeSendCallback,
} from './domain/configuration'
export { startAutomaticErrorCollection, ErrorObservable } from './domain/automaticErrorCollection'
export { computeStackTrace } from './domain/tracekit'
export { computeStackTrace, StackTrace } from './domain/tracekit'
export {
BuildEnv,
BuildMode,
Expand Down Expand Up @@ -40,7 +40,7 @@ export * from './tools/urlPolyfill'
export * from './tools/timeUtils'
export * from './tools/utils'
export * from './tools/errorFilter'
export { ErrorSource, ErrorHandling, formatUnknownError, RawError } from './tools/error'
export { ErrorSource, ErrorHandling, formatUnknownError, createHandlingStack, RawError } from './tools/error'
export { combine, Context, ContextArray, ContextValue, deepClone } from './tools/context'
export { areCookiesAuthorized, getCookie, setCookie, COOKIE_ACCESS_DELAY } from './browser/cookie'
export { startXhrProxy, XhrCompleteContext, XhrStartContext, XhrProxy, resetXhrProxy } from './browser/xhrProxy'
Expand Down
23 changes: 22 additions & 1 deletion packages/core/src/tools/error.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { StackTrace } from '../domain/tracekit'
import { formatUnknownError } from './error'
import { createHandlingStack, formatUnknownError } from './error'

describe('formatUnknownError', () => {
const NOT_COMPUTED_STACK_TRACE: StackTrace = { name: undefined, message: undefined, stack: [] } as any
Expand Down Expand Up @@ -71,3 +71,24 @@ describe('formatUnknownError', () => {
expect(formatted.message).toEqual('Uncaught {"foo":"bar"}')
})
})

describe('createHandlingStack', () => {
let handlingStack: string
function internalCall() {
handlingStack = createHandlingStack()
}
function userCallTwo() {
internalCall()
}
function userCallOne() {
userCallTwo()
}

it('should create handling stack trace without internal calls', () => {
userCallOne()

expect(handlingStack).toMatch(`Error:
at userCallTwo @ (.*)
at userCallOne @ (.*)`)
})
})
51 changes: 48 additions & 3 deletions packages/core/src/tools/error.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { StackTrace } from '../domain/tracekit'
import { callMonitored } from '../domain/internalMonitoring'
import { computeStackTrace, StackTrace } from '../domain/tracekit'
import { ClocksState } from './timeUtils'
import { jsonStringify } from './utils'
import { jsonStringify, noop } from './utils'

export interface RawError {
startClocks: ClocksState
Expand All @@ -15,6 +16,7 @@ export interface RawError {
}
originalError?: unknown
handling?: ErrorHandling
handlingStack?: string
}

export const ErrorSource = {
Expand All @@ -34,18 +36,25 @@ export enum ErrorHandling {
// eslint-disable-next-line @typescript-eslint/no-redeclare
export type ErrorSource = typeof ErrorSource[keyof typeof ErrorSource]

export function formatUnknownError(stackTrace: StackTrace | undefined, errorObject: any, nonErrorPrefix: string) {
export function formatUnknownError(
stackTrace: StackTrace | undefined,
errorObject: any,
nonErrorPrefix: string,
handlingStack?: string
) {
if (!stackTrace || (stackTrace.message === undefined && !(errorObject instanceof Error))) {
return {
message: `${nonErrorPrefix} ${jsonStringify(errorObject)!}`,
stack: 'No stack, consider using an instance of Error',
handlingStack,
type: stackTrace && stackTrace.name,
}
}

return {
message: stackTrace.message || 'Empty message',
stack: toStackTraceString(stackTrace),
handlingStack,
type: stackTrace.name,
}
}
Expand All @@ -65,3 +74,39 @@ export function toStackTraceString(stack: StackTrace) {
export function formatErrorMessage(stack: StackTrace) {
return `${stack.name || 'Error'}: ${stack.message!}`
}

/**
Creates a stacktrace without SDK internal frames.

Constraints:
- Has to be called at the utmost position of the call stack.
- No internal monitoring should encapsulate the function, that is why we need to use callMonitored inside of it.
*/
export function createHandlingStack(): string {
/**
* Skip the two internal frames:
* - SDK API (console.error, ...)
* - this function
* in order to keep only the user calls
*/
const internalFramesToSkip = 2
const error = new Error()
let formattedStack: string

// IE needs to throw the error to fill in the stack trace
if (!error.stack) {
try {
throw error
} catch (e) {
noop()
}
}

callMonitored(() => {
const stackTrace = computeStackTrace(error)
stackTrace.stack = stackTrace.stack.slice(internalFramesToSkip)
formattedStack = toStackTraceString(stackTrace)
})

return formattedStack!
}
12 changes: 12 additions & 0 deletions packages/rum-core/src/boot/rumPublicApi.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ describe('rum public api', () => {
{
context: undefined,
error: new Error('foo'),
handlingStack: jasmine.any(String),
source: ErrorSource.CUSTOM,
startClocks: jasmine.any(Object),
},
Expand All @@ -278,6 +279,17 @@ describe('rum public api', () => {
expect(displaySpy).toHaveBeenCalledWith("DD_RUM.addError: Invalid source 'invalid'")
})

it('should generate a handling stack', () => {
rumPublicApi.init(DEFAULT_INIT_CONFIGURATION)
function triggerError() {
rumPublicApi.addError(new Error('message'))
}
triggerError()
expect(addErrorSpy).toHaveBeenCalledTimes(1)
const stacktrace = addErrorSpy.calls.argsFor(0)[0].handlingStack
expect(stacktrace).toMatch(/^Error:\s+at triggerError (.|\n)*$/)
})

describe('save context when capturing an error', () => {
it('saves the date', () => {
const { clock } = setupBuilder.withFakeClock().build()
Expand Down
34 changes: 20 additions & 14 deletions packages/rum-core/src/boot/rumPublicApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import {
commonInit,
Configuration,
InternalMonitoring,
callMonitored,
createHandlingStack,
} from '@datadog/browser-core'
import { ProvidedSource } from '../domain/rumEventsCollection/error/errorCollection'
import { CommonContext, User, ActionType, RumEventDomainContext } from '../rawRumEvent.types'
Expand Down Expand Up @@ -152,21 +154,25 @@ export function makeRumPublicApi<C extends RumUserConfiguration>(startRumImpl: S
rumPublicApi.addAction(name, context as Context)
},

addError: monitor((error: unknown, context?: object, source: ProvidedSource = ErrorSource.CUSTOM) => {
let checkedSource: ProvidedSource
if (source === ErrorSource.CUSTOM || source === ErrorSource.NETWORK || source === ErrorSource.SOURCE) {
checkedSource = source
} else {
display.error(`DD_RUM.addError: Invalid source '${source as string}'`)
checkedSource = ErrorSource.CUSTOM
}
addErrorStrategy({
error,
context: deepClone(context as Context),
source: checkedSource,
startClocks: clocksNow(),
addError: (error: unknown, context?: object, source: ProvidedSource = ErrorSource.CUSTOM) => {
const handlingStack = createHandlingStack()
callMonitored(() => {
let checkedSource: ProvidedSource
if (source === ErrorSource.CUSTOM || source === ErrorSource.NETWORK || source === ErrorSource.SOURCE) {
checkedSource = source
} else {
display.error(`DD_RUM.addError: Invalid source '${source as string}'`)
checkedSource = ErrorSource.CUSTOM
}
addErrorStrategy({
error,
handlingStack,
context: deepClone(context as Context),
source: checkedSource,
startClocks: clocksNow(),
})
})
}),
},

addTiming: monitor((name: string) => {
addTimingStrategy(name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@ describe('error collection', () => {
it('notifies a raw rum error event', () => {
const { rawRumEvents } = setupBuilder.build()
const error = new Error('foo')

addError({
error,
handlingStack: 'Error: handling foo',
source: ErrorSource.CUSTOM,
startClocks: { relative: 1234 as RelativeTime, timeStamp: 123456789 as TimeStamp },
})
Expand All @@ -46,6 +48,7 @@ describe('error collection', () => {
resource: undefined,
source: ErrorSource.CUSTOM,
stack: jasmine.stringMatching('Error: foo'),
handling_stack: 'Error: handling foo',
type: 'Error',
handling: ErrorHandling.HANDLED,
},
Expand All @@ -65,6 +68,7 @@ describe('error collection', () => {
addError({
context: { foo: 'bar' },
error: new Error('foo'),
handlingStack: 'Error: handling foo',
source: ErrorSource.CUSTOM,
startClocks: { relative: 1234 as RelativeTime, timeStamp: 123456789 as TimeStamp },
})
Expand All @@ -78,6 +82,7 @@ describe('error collection', () => {
addError(
{
error: new Error('foo'),
handlingStack: 'Error: handling foo',
source: ErrorSource.CUSTOM,
startClocks: { relative: 1234 as RelativeTime, timeStamp: 123456789 as TimeStamp },
},
Expand All @@ -93,6 +98,7 @@ describe('error collection', () => {
addError(
{
error: new Error('foo'),
handlingStack: 'Error: handling foo',
source: ErrorSource.CUSTOM,
startClocks: { relative: 1234 as RelativeTime, timeStamp: 123456789 as TimeStamp },
},
Expand All @@ -108,6 +114,7 @@ describe('error collection', () => {
addError({
error: { foo: 'bar' },
source: ErrorSource.CUSTOM,
handlingStack: 'Error: handling foo',
startClocks: { relative: 1234 as RelativeTime, timeStamp: 123456789 as TimeStamp },
})
expect(rawRumEvents[0].domainContext).toEqual({
Expand Down Expand Up @@ -149,6 +156,7 @@ describe('error collection', () => {
},
source: ErrorSource.NETWORK,
stack: 'bar',
handling_stack: undefined,
type: 'foo',
handling: undefined,
},
Expand Down
Loading