feat: manually capture errors #1374
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: +1.71 kB (+0.15%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
src/posthog-core.ts
Outdated
| @@ -1815,6 +1815,11 @@ export class PostHog { | |||
| return !!this.sessionRecording?.started | |||
| } | |||
|
|
|||
| /** Checks the feature flags associated with this Survey to see if the survey can be rendered. */ | |||
| captureException(error: Error, additionalProperties?: Properties): void { | |||
| this.exceptionObserver?.captureException(error, additionalProperties) | |||
There was a problem hiding this comment.
hmmmm....
exception capture is ~10% of the bundle if it wasn't lazily loaded.
if someone doesn't have exception capture enabled posthog will still advertise this method as part of its contract, what should it do?
just log?
There was a problem hiding this comment.
exception capture enabled
this is mostly about the auto-capture, right? people could still do capture(thisError) manually, if we allow that, it should always be part of the bundle.
Maybe we need a different config for auto-capture and manual capture, so people that don't use either, don't pay the bundle price.
| captureException(error: Error, additionalProperties: Properties = {}) { | ||
| const errorEventArgs: ErrorEventArgs = [error.message, undefined, undefined, undefined, error] | ||
| const errorProperties = errorToProperties(errorEventArgs) | ||
| this._captureException({ ...errorProperties, ...additionalProperties }) | ||
| } |
There was a problem hiding this comment.
yep, I think this is the 3% bump in bundle... we're pulling in errorToProperties which would otherwise not be imported unless exception capture is configured
I think this should check if error stuff is on the window and log if not
and we give people a local config to turn it on (if it's not already possible)
if lots of people use it we can make it the default or have remote config to allow manual but not auto capture???
or we add this and stand the 3% bump for everyone but that feels wrong IMHO open to challenge
There was a problem hiding this comment.
Certainly wasn't my intention to increase the default bundle size... I actually thought I'd avoid it by moving errorToProperties into this file rather than src/posthog-core.ts.
I pushed another commit that hopefully moves things in the right direction but not sure how to fully evaluate this. Is there any prior art for how we do this with something similar?
Do you think there should be a local config to enable manual capture if someone doesn't have autocapture enabled? It feels like a bit of a gotcha to me but get that it might be necessary if we want to ensure the bundle size stays small for folks not using it. I wonder what the flow looks like for manually showing surveys, we should make sure that both function the same way at the very least
There was a problem hiding this comment.
the core capture in surveys is part of the main SDK - this is always available and anyone can use it. The extension is all the code for showing surveys.
What imo would be ideal here (and what we did for surveys as well) is to figure out how the manual exception capture API would look like, and re-use that inside the extension as well (instead of the manual _captureException function) - I think the interface you have already looks reasonable, and the personURL can be just another part of additionalProperties.
The contention here is that the errorToProperties function (+dependents) is pretty big, while the surveys core code is pretty small, so taking the same approach might not make the most sense.
As such, I think there are two three options:
- Keep basic manual exception capture as part of main SDK (which does not do stack trace parsing). And when they enable autocapture/stack trace parsing - we do this parsing to get all the details of the error.
- Since autocapture is 10% of bundle and stack trace parsing is 3%, we can make two extensions: One for manual capture, one for autocapture. Enabling autocapture also enables manual capture.
- Make stack trace parsing part of the core SDK, 3% isn't that big of a hit (what you already currently have).
I'm currently leaning towards (1), specially because least effort currently, and gives us enough information (by users having to turn on autocapture to get the stack trace details) to make a decision whether (2) or (3) is worth it. Also doesn't affect current bundle size for existing users 👌 . My hypothesis is that we'll find out people who want to do manual capture would be very happy with having autocapture as well, so would anyways turn it on.
There was a problem hiding this comment.
re-reading, looks like Paul said the exact same thing but in fewer words 🙈 🙈
There was a problem hiding this comment.
it is a rare event that I'm the most concise person 🤣
| captureException(errorProperties: Properties) { | ||
| captureException(error: Error, additionalProperties: Properties = {}) { | ||
| if (!window || !this.isEnabled || this.isCapturing) { | ||
| logger.error(LOGGER_PREFIX + ' error tracking is not enabled - cannot capture exception') |
There was a problem hiding this comment.
I think the !window has to be a different logging message so as not to mislead customers or us when debugging things
|
@pauldambra @neilkakkar I think I followed what you both suggested, hopefully I haven't overcooked things and made it overly complicated. There's still tests to be fixed but wanted to make sure I was on the right path first. Size change looks to be down but I'm surprised it's still ~1.5%. Does that seem reasonable? |
|
yeah looks reasonable! I think the only thing missing is upgrading to have the stack if autocapture is enabled, when capturing custom exceptions. (and you can't do this via import, this needs to be set on the window when autocapture is enabled) The 2.5kB still seems a bit surprising, that's effectively one page of text. Could you check if this goes down if you don't import |
|
@neilkakkar yep, inlining is a big win. Down to 329 B (from 2.5kB) |
| 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}/ | ||
| ) | ||
| expect(captures[2].properties.$exception_source).to.eql(undefined) |
There was a problem hiding this comment.
minor nit: check extra_prop is set too?
| cy.wait(1500) | ||
|
|
||
| cy.phCaptures({ full: true }).then((captures) => { | ||
| expect(captures[2].properties.$exception_stack_trace_raw).to.exist |
There was a problem hiding this comment.
nit: confirm this is also wat even am i and not some other autocaptured exception (since all of those will have stack traces too)
Changes
Add the manual
captureExceptionmethod to the SDK