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

feat: add stack to stacktraceless "exceptions" #1472

Merged
merged 4 commits into from
Oct 16, 2024
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
16 changes: 16 additions & 0 deletions cypress/e2e/error-tracking.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,22 @@ describe('Exception capture', () => {
cy.wait('@exception-autocapture-script')
})

it('adds stacktrace to captured strings', () => {
cy.get('[data-cy-exception-string-button]').click()

// ugh
cy.wait(1500)

cy.phCaptures({ full: true }).then((captures) => {
expect(captures.map((c) => c.event)).to.deep.equal(['$pageview', '$autocapture', '$exception'])
expect(captures[2].event).to.be.eql('$exception')
expect(captures[2].properties.$exception_list[0].stacktrace.frames.length).to.be.eq(1)
expect(captures[2].properties.$exception_list[0].stacktrace.frames[0].function).to.be.eq(
'HTMLButtonElement.onclick'
)
})
})

it('autocaptures exceptions', () => {
cy.get('[data-cy-button-throws-error]').click()

Expand Down
4 changes: 4 additions & 0 deletions playground/cypress-full/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@
Send an exception
</button>

<button data-cy-exception-string-button onclick="posthog.captureException('I am a plain old string', { extra_prop: 2 })">
Capture as string
</button>

<br />

<script>
Expand Down
4 changes: 4 additions & 0 deletions playground/cypress/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@
Capture an exception
</button>

<button data-cy-exception-string-button onclick="posthog.captureException('I am a plain old string', { extra_prop: 2 })">
Capture as string
</button>

<script>
!function(t,e){var o,n,p,r;e.__SV||(window.posthog=e,e._i=[],e.init=function(i,s,a){function g(t,e){var o=e.split(".");2==o.length&&(t=t[o[0]],e=o[1]),t[e]=function(){t.push([e].concat(Array.prototype.slice.call(arguments,0)))}}(p=t.createElement("script")).type="text/javascript",p.async=!0,p.src=s.api_host+"/static/array.js",(r=t.getElementsByTagName("script")[0]).parentNode.insertBefore(p,r);var u=e;for(void 0!==a?u=e[a]=[]:a="posthog",u.people=u.people||[],u.toString=function(t){var e="posthog";return"posthog"!==a&&(e+="."+a),t||(e+=" (stub)"),e},u.people.toString=function(){return u.toString(1)+".people (stub)"},o="capture identify alias people.set people.set_once set_config register register_once unregister opt_out_capturing has_opted_out_capturing opt_in_capturing reset isFeatureEnabled onFeatureFlags getSurveys getActiveMatchingSurveys captureException".split(" "),n=0;n<o.length;n++)g(u,o[n]);e._i.push([i,s,a])},e.__SV=1)}(document,window.posthog||[]);
</script>
Expand Down
66 changes: 43 additions & 23 deletions src/extensions/exception-autocapture/error-conversion.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export interface ErrorConversions {
const ERROR_TYPES_PATTERN =
/^(?:[Uu]ncaught (?:exception: )?)?(?:((?:Eval|Internal|Range|Reference|Syntax|Type|URI|)Error): )?(.*)$/i

export function parseStackFrames(ex: Error & { framesToPop?: number; stacktrace?: string }): StackFrame[] {
export function parseStackFrames(ex: Error & { stacktrace?: string }, framesToPop: number = 0): StackFrame[] {
// Access and store the stacktrace property before doing ANYTHING
// else to it because Opera is not very good at providing it
// reliably in other circumstances.
Expand All @@ -65,7 +65,9 @@ export function parseStackFrames(ex: Error & { framesToPop?: number; stacktrace?
const skipLines = getSkipFirstStackStringLines(ex)

try {
return defaultStackParser(stacktrace, skipLines)
const frames = defaultStackParser(stacktrace, skipLines)
// frames are reversed so we remove the from the back of the array
return frames.slice(0, frames.length - framesToPop)
} catch {
// no-empty
}
Expand Down Expand Up @@ -146,17 +148,26 @@ function errorPropertiesFromString(candidate: string, metadata?: ErrorMetadata):
? candidate
: metadata?.defaultExceptionMessage

const exception: Exception = {
type: exceptionType,
value: exceptionMessage,
mechanism: {
handled,
synthetic,
},
}

if (metadata?.syntheticException) {
// Kludge: strip the last frame from a synthetically created error
// so that it does not appear in a users stack trace
const frames = parseStackFrames(metadata.syntheticException, 1)
if (frames.length) {
exception.stacktrace = { frames }
}
}

return {
$exception_list: [
{
type: exceptionType,
value: exceptionMessage,
mechanism: {
handled,
synthetic,
},
},
],
$exception_list: [exception],
$exception_level: 'error',
}
}
Expand Down Expand Up @@ -206,17 +217,26 @@ function errorPropertiesFromObject(candidate: Record<string, unknown>, metadata?
? metadata.overrideExceptionMessage
: `Non-Error ${'exception'} captured with keys: ${extractExceptionKeysForMessage(candidate)}`

const exception: Exception = {
type: exceptionType,
value: exceptionMessage,
mechanism: {
handled,
synthetic,
},
}

if (metadata?.syntheticException) {
// Kludge: strip the last frame from a synthetically created error
// so that it does not appear in a users stack trace
const frames = parseStackFrames(metadata?.syntheticException, 1)
if (frames.length) {
exception.stacktrace = { frames }
}
}

return {
$exception_list: [
{
type: exceptionType,
value: exceptionMessage,
mechanism: {
handled,
synthetic,
},
},
],
$exception_list: [exception],
$exception_level: isSeverityLevel(candidate.level) ? candidate.level : 'error',
}
}
Expand Down Expand Up @@ -259,7 +279,7 @@ export function errorToProperties(
} else if (isPlainObject(candidate) || isEvent(candidate)) {
// group these by using the keys available on the object
const objectException = candidate as Record<string, unknown>
return errorPropertiesFromObject(objectException)
return errorPropertiesFromObject(objectException, metadata)
} else if (isUndefined(error) && isString(event)) {
let name = 'Error'
let message = event
Expand Down
9 changes: 0 additions & 9 deletions src/extensions/exception-autocapture/stack-trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ export interface StackFrame {
}

const WEBPACK_ERROR_REGEXP = /\(error: (.*)\)/
const STRIP_FRAME_REGEXP = /captureException/
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 is a superior approach but I couldn't get it to work. The function name I was seeing was t.value in the stack trace. Perhaps there's some Javascript function construction magic needed to ensure the function name gets caught in the stack. Open to any thoughts if people have them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably because array.js is minified? Or the loaded extensions are if we're using the npm version of main package..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably true - very meta given our current work 😂

const STACKTRACE_FRAME_LIMIT = 50

const UNKNOWN_FUNCTION = '?'
Expand Down Expand Up @@ -210,14 +209,6 @@ export function reverseAndStripFrames(stack: ReadonlyArray<StackFrame>): StackFr

localStack.reverse()

if (STRIP_FRAME_REGEXP.test(getLastStackFrame(localStack).function || '')) {
localStack.pop()

if (STRIP_FRAME_REGEXP.test(getLastStackFrame(localStack).function || '')) {
localStack.pop()
}
}

return localStack.slice(0, STACKTRACE_FRAME_LIMIT).map((frame) => ({
...frame,
filename: frame.filename || getLastStackFrame(localStack).filename,
Expand Down
1 change: 1 addition & 0 deletions src/extensions/exception-autocapture/type-checking.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export function isError(candidate: unknown): candidate is Error {
case '[object Error]':
case '[object Exception]':
case '[object DOMException]':
case '[object DOMError]':
return true
default:
return isInstanceOf(candidate, Error)
Expand Down
15 changes: 8 additions & 7 deletions src/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1826,14 +1826,15 @@ export class PostHog {

/** Capture a caught exception manually */
captureException(error: Error, additionalProperties?: Properties): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to update the type here as well now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was torn... technically I'd still like people to send us errors but it is a little misleading to ourselves to think that the input will always be an error. What I really want is to type it as unknown but for a user to be shown Error as the type

const syntheticException = new Error('PostHog syntheticException')
const properties: Properties = isFunction(assignableWindow.__PosthogExtensions__?.parseErrorAsProperties)
? assignableWindow.__PosthogExtensions__.parseErrorAsProperties([
error.message,
undefined,
undefined,
undefined,
error,
])
? assignableWindow.__PosthogExtensions__.parseErrorAsProperties(
[error.message, undefined, undefined, undefined, error],
// create synthetic error to get stack in cases where user input does not contain one
// creating the exceptionas soon into our code as possible means we should only have to
// remove a single frame (this 'captureException' method) from the resultant stack
{ syntheticException }
)
: {
$exception_level: 'error',
$exception_list: [
Expand Down
1 change: 1 addition & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ export type ErrorEventArgs = [
export type ErrorMetadata = {
handled?: boolean
synthetic?: boolean
syntheticException?: Error
overrideExceptionType?: string
overrideExceptionMessage?: string
defaultExceptionType?: string
Expand Down
Loading