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

Sentry Flutter Android does not capture any exceptions at all when custom initialization - caused by callback failure, thus would be great to add a log / throw when this happens #1486

Closed
fzyzcjy opened this issue May 26, 2023 · 5 comments

Comments

@fzyzcjy
Copy link
Contributor

fzyzcjy commented May 26, 2023

Platform

Flutter Mobile

Obfuscation

Disabled

Debug Info

Disabled

Doctor

latest

Version

latest

Steps to Reproduce

Custom initialization:

class MyFlutterApplication : FlutterApplication() {
    override fun onCreate() {
        super.onCreate()
        initSentry()
    }

    fun initSentry() {
        SentryAndroid.init(this) { options ->
            options.dsn = SENTRY_DSN
            val removed = options.integrations.removeAll { it is TempSensorBreadcrumbsIntegration }
            check(removed)
        }
    }

And disable Dart side auto initiailization:

    options
      ..autoInitializeNativeSdk = !Platform.isAndroid
      ......

Then, Sentry.captureException(...).

Extra experiment and possible cause analysis

I have compared two cases:

  1. The standard default way - let Flutter side auto init Android side
  2. The way mentioned above - init Sentry Android manually at FlutterApplication.onCreate, and disable Flutter side from initing Android side

I see both cases call SentryClient._attachClientReportsAndSend and FileSystemTransport.send happily. Indeed, with the following extra logs, I can see "hi 1" and "hi 2" both.

image

I will update my findings here. If I can fix it, hopefully this issue can be used as a future reference for who wants to manually initialize sentry.

Expected Result

See exception sent to sentry server

Actual Result

No exception sent to sentry server

Are you willing to submit a PR?

None

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented May 26, 2023

I see the problem:

2023-05-26 08:14:12.449 16445-16445 Sentry                     E  Error in the 'OptionsConfiguration.configure' callback.

if enabling the isDebug output.

So, now the issue becomes: It would be great to have a more user friendly hint when there are errors. For example, when such initialization fails, we may either:

  1. Output a log even if isDebug=false, since this causes a fatal error - the whole sentry will silently fail in the whole lifetime
  2. Or, let sentry run happily even if this callback fails
  3. Or, when Dart side calls captureException, checks whether Android side is happy and picked up the envelope. Currently the Dart side just put the envelope into file system and says it's done, but indeed the Android side may never pick it up

Anyway this is just a tiny suggestion. But hopefully this can save developers' time in the future. Because the bug is not that trivial to recognize currently, while adding a log or a throw will make it quite trivial to fix.

@fzyzcjy fzyzcjy changed the title Sentry Flutter Android does not capture any exceptions at all when custom initialization Sentry Flutter Android does not capture any exceptions at all when custom initialization - caused by callback failure, thus would be great to add a log / throw when this happens May 26, 2023
@marandaneto
Copy link
Contributor

@fzyzcjy thanks for the feedback.

For TempSensorBreadcrumbsIntegration, you can disable it directly on the Android manifest, and still use the automatic init.

<meta-data android:name="io.sentry.breadcrumbs.system-events" android:value="false" />

https://docs.sentry.io/platforms/android/enriching-events/breadcrumbs/

Now to the issue:

  1. If OptionsConfiguration.configure throws an error, Sentry will log out, options.debug has to be enabled, yes.
    At some point, it was enabled by default when running the app on debug mode, but it was reverted due to being noisy, even having a sensible diagnosticLevel, so debug is only enabled if people want to.

  2. Sentry swallows the exceptions if there is a problem, Sentry should never crash the app unless it's a deterministic crash at the very beginning, for example, the Sentry DSN has an invalid format, was your callback throwing on the Dart or Android side? did it crash the app?

  3. That is a good point, I guess we can check if Sentry.isEnabled() is on the Android side, right now the calls to Sentry.x is NoOp but caching the envelope to the disk, we should improve this, thanks for the feedback.

@marandaneto
Copy link
Contributor

marandaneto commented May 26, 2023

[sentry] [error] Failed to save envelope
[sentry] PlatformException (PlatformException(1, The Sentry Android SDK is disabled, null, null))
[sentry] #0 StandardMethodCodec.decodeEnvelope (package:flutter/src/services/message_codecs.dart:652:7)
#1 MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:310:18)

#2 FileSystemTransport.send (package:sentry_flutter/src/file_system_transport.dart:21:7)

#3 SentryClient.captureEvent (package:sentry/src/sentry_client.dart:133:16)

#4 Hub.captureException (package:sentry/src/hub.dart:156:20)

#5 tryCatch (package:sentry_flutter_example/main.dart:523:5)

^ by doing #1489
The SDK will log out and swallow the exception, this is the intended behavior.

If the OptionsConfiguration callback throws (on Dart/Flutter), we also swallow and log out: https://github.com/getsentry/sentry-dart/blob/2bae60c263dfb518582269d320640ec4561f0248/dart/lib/src/sentry.dart#LL55C14-L55C14

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented May 26, 2023

For TempSensorBreadcrumbsIntegration, you can disable it directly on the Android manifest, and still use the automatic init.

Thanks but I hope other integrations are enabled (only disable temp sensor - otherwise some app stores are unhappy)

  1. but it was reverted due to being noisy - I agree. Then what about (by default) only logging when such critical errors happen, instead of by default log nothing when this error happens
  2. Sentry should never crash the app - definitely, it does not crash at all.
  3. That is a good point, I guess we can check if Sentry.isEnabled() is on the Android side, right now the calls to Sentry.x is NoOp but caching the envelope to the disk, we should improve this, thanks for the feedback. - You are welcome! At least some logging can be helpful

@marandaneto
Copy link
Contributor

Closed in favor of #1496
The other improvement already landed #1489

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

2 participants