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

fix(errors): Better define schema, align with python #1460

Merged
merged 10 commits into from
Oct 15, 2024
7 changes: 2 additions & 5 deletions cypress/e2e/error-tracking.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('Exception capture', () => {
expect(captures[2].properties.extra_prop).to.be.eql(2)
expect(captures[2].properties.$exception_source).to.eql(undefined)
expect(captures[2].properties.$exception_personURL).to.eql(undefined)
expect(captures[2].properties.$exception_stack_trace_raw).not.to.exist
expect(captures[2].properties.$exception_list).not.to.exist
})
})

Expand Down Expand Up @@ -53,9 +53,6 @@ describe('Exception capture', () => {
expect(captures[2].event).to.be.eql('$exception')
expect(captures[2].properties.$exception_message).to.be.eql('This is an error')
expect(captures[2].properties.$exception_type).to.be.eql('Error')
expect(captures[2].properties.$exception_source).to.match(
/http:\/\/localhost:\d+\/playground\/cypress\//
)
expect(captures[2].properties.$exception_personURL).to.match(
/http:\/\/localhost:\d+\/project\/test_token\/person\/[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}/
)
Expand All @@ -70,7 +67,7 @@ describe('Exception capture', () => {

cy.phCaptures({ full: true }).then((captures) => {
expect(captures[2].properties.$exception_message).to.be.eql('wat even am I')
expect(captures[2].properties.$exception_stack_trace_raw).to.exist
expect(captures[2].properties.$exception_list).to.exist
})
})
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ describe('Error conversion', () => {
$exception_message: 'but somehow still a string',
$exception_is_synthetic: true,
$exception_level: 'error',
$exception_language: 'javascript',
}
expect(errorToProperties(['Uncaught exception: InternalError: but somehow still a string'])).toEqual(expected)
})
Expand All @@ -48,6 +49,7 @@ describe('Error conversion', () => {
$exception_message: 'Non-Error exception captured with keys: foo, string',
$exception_is_synthetic: true,
$exception_level: 'error',
$exception_language: 'javascript',
}
expect(errorToProperties([{ string: 'candidate', foo: 'bar' } as unknown as Event])).toEqual(expected)
})
Expand All @@ -58,6 +60,7 @@ describe('Error conversion', () => {
$exception_message: 'Non-Error exception captured with keys: isTrusted',
$exception_is_synthetic: true,
$exception_level: 'error',
$exception_language: 'javascript',
}
const event = new MouseEvent('click', { bubbles: true, cancelable: true, composed: true })
expect(errorToProperties([event])).toEqual(expected)
Expand All @@ -71,13 +74,14 @@ describe('Error conversion', () => {
throw new Error("this mustn't be null")
}

expect(Object.keys(errorProperties)).toHaveLength(4)
expect(Object.keys(errorProperties)).toHaveLength(5)
expect(errorProperties.$exception_type).toEqual('Error')
expect(errorProperties.$exception_message).toEqual('oh no an error has happened')
expect(errorProperties.$exception_level).toEqual('error')
// the stack trace changes between runs, so we just check that it's there
expect(errorProperties.$exception_stack_trace_raw).toBeDefined()
expect(errorProperties.$exception_stack_trace_raw).toContain('{"filename')
expect(errorProperties.$exception_list).toBeDefined()
expect(errorProperties.$exception_list[0].stacktrace.frames[0].in_app).toEqual(true)
expect(errorProperties.$exception_list[0].stacktrace.frames[0].filename).toBeDefined()
})

class FakeDomError {
Expand All @@ -90,6 +94,7 @@ describe('Error conversion', () => {
$exception_type: 'DOMError',
$exception_message: 'click: foo',
$exception_level: 'error',
$exception_language: 'javascript',
}
const event = new FakeDomError('click', 'foo')
expect(errorToProperties([event as unknown as Event])).toEqual(expected)
Expand All @@ -103,13 +108,14 @@ describe('Error conversion', () => {
throw new Error("this mustn't be null")
}

expect(Object.keys(errorProperties)).toHaveLength(5)
expect(Object.keys(errorProperties)).toHaveLength(6)
expect(errorProperties.$exception_type).toEqual('dom-exception')
expect(errorProperties.$exception_message).toEqual('oh no disaster')
expect(errorProperties.$exception_level).toEqual('error')
// the stack trace changes between runs, so we just check that it's there
expect(errorProperties.$exception_stack_trace_raw).toBeDefined()
expect(errorProperties.$exception_stack_trace_raw).toContain('{"filename')
expect(errorProperties.$exception_list).toBeDefined()
expect(errorProperties.$exception_list[0].stacktrace.frames[0].in_app).toEqual(true)
expect(errorProperties.$exception_list[0].stacktrace.frames[0].filename).toBeDefined()
})

it('should convert an error event to an error', () => {
Expand All @@ -120,24 +126,23 @@ describe('Error conversion', () => {
throw new Error("this mustn't be null")
}

expect(Object.keys(errorProperties)).toHaveLength(4)
expect(Object.keys(errorProperties)).toHaveLength(5)
expect(errorProperties.$exception_type).toEqual('Error')
expect(errorProperties.$exception_message).toEqual('the real error is hidden inside')
expect(errorProperties.$exception_level).toEqual('error')
// the stack trace changes between runs, so we just check that it's there
expect(errorProperties.$exception_stack_trace_raw).toBeDefined()
expect(errorProperties.$exception_stack_trace_raw).toContain('{"filename')
expect(errorProperties.$exception_list).toBeDefined()
expect(errorProperties.$exception_list[0].stacktrace.frames[0].in_app).toEqual(true)
expect(errorProperties.$exception_list[0].stacktrace.frames[0].filename).toBeDefined()
})

it('can convert source, lineno, colno', () => {
const expected: ErrorProperties = {
$exception_colno: 200,
$exception_is_synthetic: true,
$exception_lineno: 12,
$exception_message: 'string candidate',
$exception_source: 'a source',
$exception_type: 'Error',
$exception_level: 'error',
$exception_language: 'javascript',
}
expect(errorToProperties(['string candidate', 'a source', 12, 200])).toEqual(expected)
})
Expand All @@ -152,14 +157,15 @@ describe('Error conversion', () => {
const errorProperties: ErrorProperties = unhandledRejectionToProperties([
ce as unknown as PromiseRejectionEvent,
])
expect(Object.keys(errorProperties)).toHaveLength(5)
expect(Object.keys(errorProperties)).toHaveLength(6)
expect(errorProperties.$exception_type).toEqual('UnhandledRejection')
expect(errorProperties.$exception_message).toEqual('a wrapped rejection event')
expect(errorProperties.$exception_handled).toEqual(false)
expect(errorProperties.$exception_level).toEqual('error')
// the stack trace changes between runs, so we just check that it's there
expect(errorProperties.$exception_stack_trace_raw).toBeDefined()
expect(errorProperties.$exception_stack_trace_raw).toContain('{"filename')
expect(errorProperties.$exception_list).toBeDefined()
expect(errorProperties.$exception_list[0].stacktrace.frames[0].in_app).toEqual(true)
expect(errorProperties.$exception_list[0].stacktrace.frames[0].filename).toBeDefined()
})

it('should convert unhandled promise rejection', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ describe('Exception Observer', () => {
$exception_message: 'test error',
$exception_type: 'Error',
$exception_personURL: expect.any(String),
$exception_stack_trace_raw: expect.any(String),
$exception_list: expect.any(Array),
},
})
expect(request.batchKey).toBe('exceptionEvent')
Expand Down
61 changes: 51 additions & 10 deletions src/extensions/exception-autocapture/error-conversion.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,45 @@ import {
import { defaultStackParser, StackFrame } from './stack-trace'

import { isEmptyString, isNumber, isString, isUndefined } from '../../utils/type-utils'
import { ErrorEventArgs, ErrorProperties, SeverityLevel, severityLevels } from '../../types'
import { ErrorEventArgs, SeverityLevel, severityLevels } from '../../types'

export interface ErrorProperties {
$exception_type: string
$exception_message: string
$exception_level: SeverityLevel
$exception_list?: Exception[]
// $exception_source?: string
// $exception_lineno?: number
// $exception_colno?: number
$exception_DOMException_code?: string
$exception_is_synthetic?: boolean
// $exception_stack_trace_raw?: string
$exception_handled?: boolean
$exception_personURL?: string
$exception_language?: string
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One for the future but would be cool to have a shared schema similar to what we have for TS <-> Python we have for queries

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will rm the commented out stuff 👀


export interface Exception {
type?: string
value?: string
mechanism?: {
handled?: boolean
type?: string
neilkakkar marked this conversation as resolved.
Show resolved Hide resolved
source?: string
synthetic?: boolean
}
module?: string
thread_id?: number
neilkakkar marked this conversation as resolved.
Show resolved Hide resolved
language?: string
stacktrace?: {
frames?: StackFrame[]
}
}

export interface ErrorConversions {
errorToProperties: (args: ErrorEventArgs) => ErrorProperties
unhandledRejectionToProperties: (args: [ev: PromiseRejectionEvent]) => ErrorProperties
}

/**
* based on the very wonderful MIT licensed Sentry SDK
Expand Down Expand Up @@ -59,7 +97,15 @@ function errorPropertiesFromError(error: Error): ErrorProperties {
return {
$exception_type: error.name,
$exception_message: error.message,
$exception_stack_trace_raw: JSON.stringify(frames),
$exception_list: [
{
type: error.name,
value: error.message,
stacktrace: {
frames,
},
},
],
$exception_level: 'error',
}
}
Expand Down Expand Up @@ -111,7 +157,8 @@ function errorPropertiesFromObject(candidate: Record<string, unknown>): ErrorPro
}
}

export function errorToProperties([event, source, lineno, colno, error]: ErrorEventArgs): ErrorProperties {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export function errorToProperties([event, _, __, ___, error]: ErrorEventArgs): ErrorProperties {
// some properties are not optional but, it's useful to start off without them enforced
let errorProperties: Omit<ErrorProperties, '$exception_type' | '$exception_message' | '$exception_level'> & {
$exception_type?: string
Expand Down Expand Up @@ -177,13 +224,7 @@ export function errorToProperties([event, source, lineno, colno, error]: ErrorEv
$exception_level: isSeverityLevel(errorProperties.$exception_level)
? errorProperties.$exception_level
: 'error',
...(source
? {
$exception_source: source, // TODO get this from URL if not present
Copy link
Contributor

Choose a reason for hiding this comment

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

I came here to ask if we should get rid of this... ahead of me :)

}
: {}),
...(lineno ? { $exception_lineno: lineno } : {}),
...(colno ? { $exception_colno: colno } : {}),
Comment on lines -185 to -186
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove these from the other places they're referenced in a follow up PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

they aren't referenced anywhere I think

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 they're mentioned in the new Rust service. They're also in the taxonomy descriptions but that's less important because arguably they should stick around there...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

uh oh, that shouldn't be the case, it should use the values from the frame instead 🤔 - let me check again

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh it's optional, all good, need it to stay there to support old versions that might send events 💀 , but yeah we don't need to use this at all imo

$exception_language: 'javascript',
}
}

Expand Down
19 changes: 0 additions & 19 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -575,22 +575,3 @@ export type ErrorEventArgs = [
// but provided as an array of literal types, so we can constrain the level below
export const severityLevels = ['fatal', 'error', 'warning', 'log', 'info', 'debug'] as const
export declare type SeverityLevel = typeof severityLevels[number]

export interface ErrorProperties {
$exception_type: string
$exception_message: string
$exception_level: SeverityLevel
$exception_source?: string
$exception_lineno?: number
$exception_colno?: number
$exception_DOMException_code?: string
$exception_is_synthetic?: boolean
$exception_stack_trace_raw?: string
$exception_handled?: boolean
$exception_personURL?: string
}

export interface ErrorConversions {
errorToProperties: (args: ErrorEventArgs) => ErrorProperties
unhandledRejectionToProperties: (args: [ev: PromiseRejectionEvent]) => ErrorProperties
}
3 changes: 2 additions & 1 deletion src/utils/globals.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { ErrorProperties } from '../extensions/exception-autocapture/error-conversion'
import type { PostHog } from '../posthog-core'
import { SessionIdManager } from '../sessionid'
import { ErrorEventArgs, ErrorProperties, Properties } from '../types'
import { ErrorEventArgs, Properties } from '../types'

/*
* Global helpers to protect access to browser globals in a way that is safer for different targets
Expand Down
Loading