-
Couldn't load subscription status.
- Fork 52
feat: autocapture unhandled exceptions #214
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
base: feat/capture-exception
Are you sure you want to change the base?
Conversation
|
Still need to address some feedback on #212, so just looking for a general review on the autocapture approach/config for now 🙏 |
| /// Enable automatic capture of unhandled exceptions | ||
| /// | ||
| /// When enabled, PostHog will automatically capture: | ||
| /// - Flutter framework errors (FlutterError.onError) | ||
| /// - Dart runtime errors (PlatformDispatcher.onError) | ||
| /// | ||
| /// Default: false | ||
| var captureUnhandledExceptions = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its confusing having this one and the other 2 more granular configs
i'd prefer to keep just the 2 granular ones
| /// Controls whether `PlatformDispatcher.onError errors` are captured. | ||
| /// | ||
| /// Default: true | ||
| var captureDartErrors = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should call this one something closer to the listener, eg capturePlatformDispatcherErrors and mention its the Dart errors in the comment docs
| /// Callback function to determine if an exception should be captured | ||
| /// | ||
| /// Return true to capture the exception, false to ignore it. | ||
| /// If null, all exceptions will be captured (default behavior). | ||
| /// | ||
| /// Example: | ||
| /// ```dart | ||
| /// shouldCaptureException: (error) { | ||
| /// // Don't capture test exceptions | ||
| /// if (error is TestException) return false; | ||
| /// // Don't capture StateError exceptions | ||
| /// if (error is StateError) return false; | ||
| /// // Capture all other exceptions | ||
| /// return true; | ||
| /// } | ||
| /// ``` | ||
| bool Function(dynamic error)? shouldCaptureException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should implement #155 instead of having another callback for the very same thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agreed. This was more off your comment in one of our calls that it would be good idea to check directly on error type before capturing an exception. Will remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should still do that, and we can do it via the before send as well, its not a blocker for this PR tho.
if we add this one now, its hard to remove later
| /// ``` | ||
| bool Function(dynamic error)? shouldCaptureException; | ||
|
|
||
| // We could skip for now, but we will need this on the native layer soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd remove this comment, adding to toMap wont hurt
| /// - All other packages are inApp unless in inAppExcludes | ||
| var inAppByDefault = true; | ||
|
|
||
| /// Enable automatic capture of unhandled exceptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could add one more config here for android autocapture https://github.com/PostHog/posthog-android/blob/3b565baef8b46510ca515a13f211e245edf1b4c6/posthog/src/main/java/com/posthog/errortracking/PostHogErrorTrackingConfig.kt#L16
| import '../posthog_flutter_platform_interface.dart'; | ||
|
|
||
| /// Handles automatic capture of Flutter and Dart exceptions | ||
| class PostHogErrorTrackingAutoCaptureIntegration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could create the Integration interface here and do it similar to android and ios, with the install, uninstall and setup methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipped since this is now just 1 integration, but I agree that the next integration to be added should follow that pattern?
| ); | ||
|
|
||
| if (config.captureUnhandledExceptions) { | ||
| _instance!.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid the use of !
| // Restore original handlers | ||
| if (_originalFlutterErrorHandler != null) { | ||
| FlutterError.onError = _originalFlutterErrorHandler; | ||
| _originalFlutterErrorHandler = null; | ||
| } | ||
|
|
||
| if (_originalPlatformErrorHandler != null) { | ||
| PlatformDispatcher.instance.onError = _originalPlatformErrorHandler; | ||
| _originalPlatformErrorHandler = null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should restore to the default value regardless if they are null or not
https://github.com/PostHog/posthog-android/blob/3b565baef8b46510ca515a13f211e245edf1b4c6/posthog/src/main/java/com/posthog/errortracking/PostHogErrorTrackingAutoCaptureIntegration.kt#L65
otherwise, if _originalPlatformErrorHandler is null, we will never unhook our own handlers
| _captureException( | ||
| error: details.exception, | ||
| stackTrace: details.stack, | ||
| handled: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can create a posthogexception wrapper similar to android to pass those values
https://github.com/PostHog/posthog-android/blob/main/posthog/src/main/java/com/posthog/internal/errortracking/PostHogThrowable.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was the plan from our discussion in #212. Will do
| } | ||
|
|
||
| void _posthogFlutterErrorHandler(FlutterErrorDetails details) { | ||
| // don't capture silent errors (could maybe be a config?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
| void _posthogFlutterErrorHandler(FlutterErrorDetails details) { | ||
| // don't capture silent errors (could maybe be a config?) | ||
| if (!details.silent) { | ||
| _captureException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| error: details.exception, | ||
| stackTrace: details.stack, | ||
| handled: false, | ||
| context: details.context?.toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see https://github.com/PostHog/posthog-flutter/pull/214/files#r2459538837
we should not just toString, check how we stringify the metadata, etc
| // Call the original handler | ||
| if (_originalFlutterErrorHandler != null) { | ||
| try { | ||
| _originalFlutterErrorHandler!(details); | ||
| } catch (e) { | ||
| // Pretty sure we should be doing this to avoid infinite loops | ||
| debugPrint( | ||
| 'PostHog: Error in original FlutterError.onError handler: $e'); | ||
| } | ||
| } else { | ||
| // If no original handler, use the default behavior (default is to dump to console) | ||
| FlutterError.presentError(details); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will swallow the error and modify the apps behaviour, we should just call the _originalFlutterErrorHandler and le do it what it should do
we should wrap in try catch calling our own handler if needed
| /// Platform error handler for Dart runtime errors | ||
| void _setupPlatformErrorHandler() { | ||
| // prevent circular calls | ||
| if (PlatformDispatcher.instance.onError == _posthogPlatformErrorHandler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was added not a long ago so i am not sure this is available from our min version which is now 3.22
if this was added after 3.22, we have 2 options
either increase the min version or do this https://github.com/getsentry/sentry-dart/blob/a69a51fd1695dd93024be80a50ad05dd990b2b82/packages/flutter/lib/src/utils/platform_dispatcher_wrapper.dart#L49
which is figure out at runtime if its available or not and only set the callback if its available at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc PlatformDispatcher.instance.onError does not work on flutter web, so it should be a no op
flutter/flutter#100277
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the alternative for web and older versions before having PlatformDispatcher.instance.onError is https://api.flutter.dev/flutter/dart-async/runZonedGuarded.html
https://github.com/getsentry/sentry-dart/blob/a69a51fd1695dd93024be80a50ad05dd990b2b82/packages/dart/lib/src/sentry_run_zoned_guarded.dart#L12
i'd avoid using runZonedGuarded if possible and only support the other things going forward
| // Call the original handler | ||
| if (_originalPlatformErrorHandler != null) { | ||
| try { | ||
| return _originalPlatformErrorHandler!(error, stackTrace); | ||
| } catch (e) { | ||
| debugPrint( | ||
| 'PostHog: Error in original PlatformDispatcher.onError handler: $e'); | ||
| return true; // Consider the error handled | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| Future<void> _captureException({ | ||
| required dynamic error, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always use Object or Object? if nullable, check the whole PR about this
| }) { | ||
| return _posthog.captureException( | ||
| error: error, | ||
| stackTrace: stackTrace ?? StackTrace.current, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not use StackTrace.current here, you need to figure this out inside of the captureException so you can set syntheetic or not
| @@ -1,5 +1,6 @@ | |||
| import 'package:meta/meta.dart'; | |||
|
|
|||
| import 'exceptions/posthog_error_tracking_autocapture_integration.dart'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets make the folder called errortracking which is the name of the product, and focus on this name for naming our things instead of just exceptions
| return _posthog.setup(config); | ||
| } | ||
|
|
||
| void installFlutterIntegrations(PostHogConfig config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be private
| } | ||
| } | ||
|
|
||
| void uninstallFlutterIntegrations() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
|
missing the Isolate autocapture |
| error: error, | ||
| stackTrace: stackTrace, | ||
| handled: false, | ||
| context: 'Platform error'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be the mechanism.type instead of generic, we dont need the context field here
https://github.com/PostHog/posthog-android/blob/3b565baef8b46510ca515a13f211e245edf1b4c6/posthog/src/main/java/com/posthog/internal/errortracking/ThrowableCoercer.kt#L99
💡 Motivation and Context
Part of PostHog/posthog#38845
Base is currently #212. Will rebase once merged
Adds autocapture for Flutter framework and Dart platform unhandled exceptions
💚 How did you test it?
📝 Checklist