refactor: fetch app start in integration instead of event processor (#1905)

buenaflor · web-flow · commit 5e7abc5157b9 · 2024-03-04T14:52:44.000+01:00
* Change app start integration in a way that works with ttid as well

* Formatting

* Update

* add visibleForTesting

* Update

* update

* Add app start info test

* Remove set app start info null

* Review improvements

* run on arm mac

* Fix integration test
diff --git a/.github/workflows/flutter_test.yml b/.github/workflows/flutter_test.yml
@@ -111,7 +111,7 @@ jobs:
 
   cocoa:
     name: "${{ matrix.target }} | ${{ matrix.sdk }}"
-    runs-on: macos-13
+    runs-on: macos-latest-xlarge
     timeout-minutes: 30
     defaults:
       run:
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,10 @@
 
 - Use `recordHttpBreadcrumbs` to set iOS `enableNetworkBreadcrumbs` ([#1884](https://github.com/getsentry/sentry-dart/pull/1884))
 
+### Improvements
+
+- App start is now fetched within integration instead of event processor ([#1905](https://github.com/getsentry/sentry-dart/pull/1905))
+
 ## 7.16.1
 
 ### Fixes
diff --git a/flutter/example/integration_test/integration_test.dart b/flutter/example/integration_test/integration_test.dart
@@ -1,4 +1,5 @@
 // ignore_for_file: avoid_print
+// ignore_for_file: invalid_use_of_internal_member
 
 import 'dart:async';
 import 'dart:convert';
@@ -8,6 +9,7 @@ import 'package:flutter_test/flutter_test.dart';
 import 'package:sentry_flutter/sentry_flutter.dart';
 import 'package:sentry_flutter_example/main.dart';
 import 'package:http/http.dart';
+import 'package:sentry_flutter/src/integrations/native_app_start_integration.dart';
 
 void main() {
   // const org = 'sentry-sdks';
@@ -24,6 +26,8 @@ void main() {
   // Using fake DSN for testing purposes.
   Future<void> setupSentryAndApp(WidgetTester tester,
       {String? dsn, BeforeSendCallback? beforeSendCallback}) async {
+    NativeAppStartIntegration.isIntegrationTest = true;
+
     await setupSentry(
       () async {
         await tester.pumpWidget(SentryScreenshotWidget(
diff --git a/flutter/lib/src/event_processor/native_app_start_event_processor.dart b/flutter/lib/src/event_processor/native_app_start_event_processor.dart
@@ -2,57 +2,29 @@ import 'dart:async';
 
 import 'package:sentry/sentry.dart';
 
+import '../integrations/integrations.dart';
 import '../native/sentry_native.dart';
 
 /// EventProcessor that enriches [SentryTransaction] objects with app start
 /// measurement.
 class NativeAppStartEventProcessor implements EventProcessor {
-  /// We filter out App starts more than 60s
-  static const _maxAppStartMillis = 60000;
-
-  NativeAppStartEventProcessor(
-    this._native,
-  );
-
   final SentryNative _native;
 
+  NativeAppStartEventProcessor(this._native);
+
   @override
   Future<SentryEvent?> apply(SentryEvent event, {Hint? hint}) async {
-    final appStartEnd = _native.appStartEnd;
+    if (_native.didAddAppStartMeasurement || event is! SentryTransaction) {
+      return event;
+    }
+
+    final appStartInfo = await NativeAppStartIntegration.getAppStartInfo();
+    final measurement = appStartInfo?.toMeasurement();
 
-    if (appStartEnd != null &&
-        event is SentryTransaction &&
-        !_native.didFetchAppStart) {
-      final nativeAppStart = await _native.fetchNativeAppStart();
-      if (nativeAppStart == null) {
-        return event;
-      }
-      final measurement = nativeAppStart.toMeasurement(appStartEnd);
-      // We filter out app start more than 60s.
-      // This could be due to many different reasons.
-      // If you do the manual init and init the SDK too late and it does not
-      // compute the app start end in the very first Screen.
-      // If the process starts but the App isn't in the foreground.
-      // If the system forked the process earlier to accelerate the app start.
-      // And some unknown reasons that could not be reproduced.
-      // We've seen app starts with hours, days and even months.
-      if (measurement.value >= _maxAppStartMillis) {
-        return event;
-      }
+    if (measurement != null) {
       event.measurements[measurement.name] = measurement;
+      _native.didAddAppStartMeasurement = true;
     }
     return event;
   }
 }
-
-extension NativeAppStartMeasurement on NativeAppStart {
-  SentryMeasurement toMeasurement(DateTime appStartEnd) {
-    final appStartDateTime =
-        DateTime.fromMillisecondsSinceEpoch(appStartTime.toInt());
-    final duration = appStartEnd.difference(appStartDateTime);
-
-    return isColdStart
-        ? SentryMeasurement.coldAppStart(duration)
-        : SentryMeasurement.warmAppStart(duration);
-  }
-}
diff --git a/flutter/lib/src/frame_callback_handler.dart b/flutter/lib/src/frame_callback_handler.dart
@@ -0,0 +1,15 @@
+import 'package:flutter/scheduler.dart';
+
+abstract class FrameCallbackHandler {
+  void addPostFrameCallback(FrameCallback callback);
+}
+
+class DefaultFrameCallbackHandler implements FrameCallbackHandler {
+  @override
+  void addPostFrameCallback(FrameCallback callback) {
+    try {
+      /// Flutter >= 2.12 throws if SchedulerBinding.instance isn't initialized.
+      SchedulerBinding.instance.addPostFrameCallback(callback);
+    } catch (_) {}
+  }
+}
diff --git a/flutter/lib/src/integrations/native_app_start_integration.dart b/flutter/lib/src/integrations/native_app_start_integration.dart
@@ -1,31 +1,103 @@
-import 'package:flutter/scheduler.dart';
-import 'package:sentry/sentry.dart';
+import 'dart:async';
 
-import '../sentry_flutter_options.dart';
+import 'package:meta/meta.dart';
+
+import '../../sentry_flutter.dart';
+import '../frame_callback_handler.dart';
 import '../native/sentry_native.dart';
 import '../event_processor/native_app_start_event_processor.dart';
 
 /// Integration which handles communication with native frameworks in order to
 /// enrich [SentryTransaction] objects with app start data for mobile vitals.
 class NativeAppStartIntegration extends Integration<SentryFlutterOptions> {
-  NativeAppStartIntegration(this._native, this._schedulerBindingProvider);
+  NativeAppStartIntegration(this._native, this._frameCallbackHandler);
 
   final SentryNative _native;
-  final SchedulerBindingProvider _schedulerBindingProvider;
+  final FrameCallbackHandler _frameCallbackHandler;
+
+  /// We filter out App starts more than 60s
+  static const _maxAppStartMillis = 60000;
+
+  static Completer<AppStartInfo?> _appStartCompleter =
+      Completer<AppStartInfo?>();
+  static AppStartInfo? _appStartInfo;
+
+  @internal
+  static bool isIntegrationTest = false;
+
+  @internal
+  static void setAppStartInfo(AppStartInfo? appStartInfo) {
+    _appStartInfo = appStartInfo;
+    if (_appStartCompleter.isCompleted) {
+      _appStartCompleter = Completer<AppStartInfo?>();
+    }
+    _appStartCompleter.complete(appStartInfo);
+  }
+
+  @internal
+  static Future<AppStartInfo?> getAppStartInfo() {
+    if (_appStartInfo != null) {
+      return Future.value(_appStartInfo);
+    }
+    return _appStartCompleter.future;
+  }
+
+  @visibleForTesting
+  static void clearAppStartInfo() {
+    _appStartInfo = null;
+    _appStartCompleter = Completer<AppStartInfo?>();
+  }
 
   @override
   void call(Hub hub, SentryFlutterOptions options) {
+    if (isIntegrationTest) {
+      final appStartInfo = AppStartInfo(AppStartType.cold,
+          start: DateTime.now(),
+          end: DateTime.now().add(const Duration(milliseconds: 100)));
+      setAppStartInfo(appStartInfo);
+      return;
+    }
+
     if (options.autoAppStart) {
-      final schedulerBinding = _schedulerBindingProvider();
-      if (schedulerBinding == null) {
-        options.logger(SentryLevel.debug,
-            'Scheduler binding is null. Can\'t auto detect app start time.');
-      } else {
-        schedulerBinding.addPostFrameCallback((timeStamp) {
-          // ignore: invalid_use_of_internal_member
-          _native.appStartEnd = options.clock();
-        });
-      }
+      _frameCallbackHandler.addPostFrameCallback((timeStamp) async {
+        if (_native.didFetchAppStart) {
+          return;
+        }
+
+        // We only assign the current time if it's not already set - this is useful in tests
+        // ignore: invalid_use_of_internal_member
+        _native.appStartEnd ??= options.clock();
+        final appStartEnd = _native.appStartEnd;
+        final nativeAppStart = await _native.fetchNativeAppStart();
+
+        if (nativeAppStart == null || appStartEnd == null) {
+          return;
+        }
+
+        final appStartDateTime = DateTime.fromMillisecondsSinceEpoch(
+            nativeAppStart.appStartTime.toInt());
+        final duration = appStartEnd.difference(appStartDateTime);
+
+        // We filter out app start more than 60s.
+        // This could be due to many different reasons.
+        // If you do the manual init and init the SDK too late and it does not
+        // compute the app start end in the very first Screen.
+        // If the process starts but the App isn't in the foreground.
+        // If the system forked the process earlier to accelerate the app start.
+        // And some unknown reasons that could not be reproduced.
+        // We've seen app starts with hours, days and even months.
+        if (duration.inMilliseconds > _maxAppStartMillis) {
+          setAppStartInfo(null);
+          return;
+        }
+
+        final appStartInfo = AppStartInfo(
+            nativeAppStart.isColdStart ? AppStartType.cold : AppStartType.warm,
+            start: DateTime.fromMillisecondsSinceEpoch(
+                nativeAppStart.appStartTime.toInt()),
+            end: appStartEnd);
+        setAppStartInfo(appStartInfo);
+      });
     }
 
     options.addEventProcessor(NativeAppStartEventProcessor(_native));
@@ -34,5 +106,19 @@ class NativeAppStartIntegration extends Integration<SentryFlutterOptions> {
   }
 }
 
-/// Used to provide scheduler binding at call time.
-typedef SchedulerBindingProvider = SchedulerBinding? Function();
+enum AppStartType { cold, warm }
+
+class AppStartInfo {
+  AppStartInfo(this.type, {required this.start, required this.end});
+
+  final AppStartType type;
+  final DateTime start;
+  final DateTime end;
+
+  SentryMeasurement toMeasurement() {
+    final duration = end.difference(start);
+    return type == AppStartType.cold
+        ? SentryMeasurement.coldAppStart(duration)
+        : SentryMeasurement.warmAppStart(duration);
+  }
+}
diff --git a/flutter/lib/src/native/sentry_native.dart b/flutter/lib/src/native/sentry_native.dart
@@ -27,6 +27,9 @@ class SentryNative {
   /// Flag indicating if app start was already fetched.
   bool get didFetchAppStart => _didFetchAppStart;
 
+  /// Flag indicating if app start measurement was added to the first transaction.
+  bool didAddAppStartMeasurement = false;
+
   /// Fetch [NativeAppStart] from native channels. Can only be called once.
   Future<NativeAppStart?> fetchNativeAppStart() async {
     _didFetchAppStart = true;
diff --git a/flutter/lib/src/sentry_flutter.dart b/flutter/lib/src/sentry_flutter.dart
@@ -1,14 +1,14 @@
 import 'dart:async';
 import 'dart:ui';
 
-import 'package:flutter/scheduler.dart';
 import 'package:flutter/services.dart';
 import 'package:flutter/widgets.dart';
 import 'package:meta/meta.dart';
 import '../sentry_flutter.dart';
 import 'event_processor/android_platform_exception_event_processor.dart';
 import 'event_processor/flutter_exception_event_processor.dart';
 import 'event_processor/platform_exception_event_processor.dart';
+import 'frame_callback_handler.dart';
 import 'integrations/connectivity/connectivity_integration.dart';
 import 'integrations/screenshot_integration.dart';
 import 'native/factory.dart';
@@ -189,13 +189,7 @@ mixin SentryFlutter {
     if (_native != null) {
       integrations.add(NativeAppStartIntegration(
         _native!,
-        () {
-          try {
-            /// Flutter >= 2.12 throws if SchedulerBinding.instance isn't initialized.
-            return SchedulerBinding.instance;
-          } catch (_) {}
-          return null;
-        },
+        DefaultFrameCallbackHandler(),
       ));
     }
     return integrations;
@@ -231,6 +225,7 @@ mixin SentryFlutter {
 
   @internal
   static SentryNative? get native => _native;
+
   @internal
   static set native(SentryNative? value) => _native = value;
   static SentryNative? _native;
diff --git a/flutter/test/fake_frame_callback_handler.dart b/flutter/test/fake_frame_callback_handler.dart
@@ -0,0 +1,19 @@
+import 'package:flutter/scheduler.dart';
+import 'package:sentry_flutter/src/frame_callback_handler.dart';
+
+class FakeFrameCallbackHandler implements FrameCallbackHandler {
+  FrameCallback? storedCallback;
+
+  final Duration _finishAfterDuration;
+
+  FakeFrameCallbackHandler(
+      {Duration finishAfterDuration = const Duration(milliseconds: 500)})
+      : _finishAfterDuration = finishAfterDuration;
+
+  @override
+  void addPostFrameCallback(FrameCallback callback) async {
+    // ignore: inference_failure_on_instance_creation
+    await Future.delayed(_finishAfterDuration);
+    callback(Duration.zero);
+  }
+}
diff --git a/flutter/test/integrations/native_app_start_integration_test.dart b/flutter/test/integrations/native_app_start_integration_test.dart
diff --git a/flutter/test/mocks.dart b/flutter/test/mocks.dart
