Sentry-dart: Consider making `beforeSend` and `beforeBreadcrumb` async

[ueman]

Currently beforeSend and beforeBreadcrumb are not async which makes it hard to use any libraries like device_info_plus hard to use in those callbacks.

beforeSend is already only used in an async context. beforeBreadcrumb is used in a non-async context.

Event processors are already async.

[marandaneto]

FutureOr<SentryEvent?> and FutureOr<Breadcrumb?> make sense to me

[ueman]

Changing beforeBreadcrumb method signature would also cause that the signature of addBreadcrumb needs to be changed.

[bruno-garcia]

it hard to use any libraries like device_info_plus hard to use in those callbacks.

Can you call to get the value before setting up the beforeSend? Or you need to read the value on each call

[ueman]

Can you call to get the value before setting up the beforeSend?

You can but it's cumbersome.

Consider the following (pseudo) code:

import 'dart:async';

import 'package:sentry/sentry.dart';

Future<void> main() async {
  runZonedGuarded(() async {
    WidgetsFlutterBinding.ensureIntialize();
    // In order to call native code, on has to call WidgetsFlutterBinding.ensureIntialize(); before
    // calling any platformchannels 
    var deviceInfo = await DeviceInfo.loadInfo();
    await Sentry.init(
      (options) {
        options.dsn = 'https://[email protected]/example';
        options.beforeSend = () {
          // do something with deviceInfo
        }
      },
    );

    // Init your App.
    initApp();
  }, (exception, stackTrace) async {
    await Sentry.captureException(exception, stackTrace: stackTrace);
  });
}

void initApp() {
  // your app code
}

vs

Future<void> main() async {
  await Sentry.init(
    (options) {
      options.dsn = 'https://[email protected]/example';
      options.beforeSend = () {
          // Sentry already called WidgetsFlutterBinding.ensureIntialize(); for us
          // We also don't need to add a runZonGuarded ourself

          // do something with deviceInfo
         var deviceInfo = await DeviceInfo.loadInfo();
         // one could cache deviceInfo for every other call to beforeSend
      }
    },
    appRunner: initApp, // Init your App.
  );
}

void initApp() {
  // your app code
}

In the first example you can't use a lot of our inbuilt functionality.
The second example is at least what I would prefer because it does most things for me.

[bruno-garcia]

Semantically different two because in the first example you're reading the device data once, and using that reference to augment events. On the second, each event captured will call out to get device info

[ueman]

Sure, but you can easily save deviceInfo to a variable in the second example so that both examples are equal for the most part.

[marandaneto]

yeah, the difference is, today to achieve that, you need to have your own runZonedGuarded, call WidgetsFlutterBinding.ensureIntialize(), cache deviceInfo to a var, and then use the var on beforeSend, not nice, and obviously crossing fingers that this async call won't crash because the SDK won't be init. yet so events won't be captured.

[bruno-garcia]

That makes sense. Sounds like it's the right thing to do, I don't have any arguments against it at this point.
I'm not sure what level the tooling is in regards to warning about methods that return Future not being awaited but we need to keep in mind docs/examples etc need to change, and customers current callbacks will be affected.

If we changed the signature of the current callbacks, would the compiler alert that it is now an async method and must be declared as such? For a user that simply updated pub.dev without reading the migration plan, what would be the experience?

[ueman]

We would change the method signature from Event beforeSend(Event event) to FutureOr<Event> beforeSend(Event event).
What this means is that the user doesn't even need to return a future and code which is valid today is also valid after the change.
It's basically painless.

[marandaneto]

there's also https://dart-lang.github.io/linter/lints/unawaited_futures.html when people turn it on

[bruno-garcia]

The user would need to return a Future if they declared the callback as async though, no?
Wouldn't they need to add async to the signature?

there's also https://dart-lang.github.io/linter/lints/unawaited_futures.html when people turn it on

So only when using pedantic they'd get the warning I see.

[marandaneto]

@bruno-garcia no, that's the meaning of FutureOr instead of just Future