feat: Exception Autocapture #649
Conversation
|
Size Change: +32.4 kB (+1%) Total Size: 2.35 MB
ℹ️ View Unchanged
|
benjackwhite
left a comment
benjackwhite
left a comment
There was a problem hiding this comment.
Some first pass comments. Will take a proper look at the exception code in a bit
| if (this.instance.sessionRecordingStarted()) { | ||
| errorProperties.$exception_sessionRecordingURL = | ||
| posthogHost + | ||
| '/recordings/' + | ||
| this.instance.sessionManager.checkAndGetSessionAndWindowId(true).sessionId | ||
| } |
There was a problem hiding this comment.
We have get_session_replay_url() now which should be used here.
One thought though - why do we want this 🤔 is it not redundant given that we can rebuild it on the other end?
src/posthog-core.ts
Outdated
| token: '', | ||
| autocapture: true, | ||
| // TODO: change to undefined when we release this so that remote config can turn it on even if unconfigured by the user | ||
| exception_autocapture: false, |
There was a problem hiding this comment.
Feels a little weird that we have exception_autocapture locally and autocaptureExceptions remotely... Not sure if that means it should be different but it feels confusing...
benjackwhite
left a comment
benjackwhite
left a comment
There was a problem hiding this comment.
Some first pass comments. Will take a proper look at the exception code in a bit
benjackwhite
left a comment
benjackwhite
left a comment
There was a problem hiding this comment.
Small changes but overall this looks great. Didn't review the "inspired" code too deeply but it all looks very clever ;)
src/posthog-core.ts
Outdated
| * Where there is a key in both generated exception and passed properties, | ||
| * the generated exception property takes precedence. | ||
| * | ||
| * If exception autocapture is not enabled this method is a no-op |
There was a problem hiding this comment.
I don't think this makes sense. Why is autocapturing of exceptions a necessity to manually capture an exception?
It's like saying " capture("$pageview") only works if you have automatic pageview capturing turned on" 😅
Happy to be challenged here but my exception would be that this always works regardless of the autocapture setting
There was a problem hiding this comment.
No, you've got tunnel vision 🙈
| captureException(args: ErrorEventArgs, properties?: Properties) { | ||
| if (this.isCapturing()) { | ||
| const errorProperties = errorToProperties(args) | ||
| const propertiesToSend = { ...properties, ...errorProperties } | ||
|
|
||
| const posthogHost = this.instance.config.ui_host || this.instance.config.api_host | ||
| errorProperties.$exception_personURL = posthogHost + '/person/' + this.instance.get_distinct_id() | ||
|
|
||
| this.sendExceptionEvent(propertiesToSend) | ||
| } else { | ||
| console.warn('PostHog exception autocapture is not enabled') | ||
| } | ||
| } |
There was a problem hiding this comment.
Personally I think we should remove the isCapturing check and have that in the caller instead. That way we can still call this for non-autocapture use cases
Changes
adds
session recording URL