Skip to content

Commit

Permalink
Review improvements
Browse files Browse the repository at this point in the history
  • Loading branch information
buenaflor committed Mar 4, 2024
1 parent 9d09211 commit cf5af40
Show file tree
Hide file tree
Showing 7 changed files with 54 additions and 83 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class NativeAppStartEventProcessor implements EventProcessor {

if (measurement != null) {
event.measurements[measurement.name] = measurement;
_native.setDidAddAppStartMeasurement(true);
_native.didAddAppStartMeasurement = true;
}
return event;
}
Expand Down
5 changes: 4 additions & 1 deletion flutter/lib/src/frame_callback_handler.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ abstract class FrameCallbackHandler {
class DefaultFrameCallbackHandler implements FrameCallbackHandler {
@override
void addPostFrameCallback(FrameCallback callback) {
SchedulerBinding.instance.addPostFrameCallback(callback);
try {
/// Flutter >= 2.12 throws if SchedulerBinding.instance isn't initialized.
SchedulerBinding.instance.addPostFrameCallback(callback);
} catch (_) {}
}
}
95 changes: 42 additions & 53 deletions flutter/lib/src/integrations/native_app_start_integration.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ 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;
Expand Down Expand Up @@ -48,54 +48,46 @@ class NativeAppStartIntegration extends Integration<SentryFlutterOptions> {
@override
void call(Hub hub, SentryFlutterOptions options) {
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) async {
if (_native.didFetchAppStart) {
setAppStartInfo(null);
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);
});
}
_frameCallbackHandler.addPostFrameCallback((timeStamp) async {
if (_native.didFetchAppStart) {
setAppStartInfo(null);
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));
Expand All @@ -104,9 +96,6 @@ class NativeAppStartIntegration extends Integration<SentryFlutterOptions> {
}
}

/// Used to provide scheduler binding at call time.
typedef SchedulerBindingProvider = FrameCallbackHandler? Function();

enum AppStartType { cold, warm }

class AppStartInfo {
Expand Down
8 changes: 1 addition & 7 deletions flutter/lib/src/native/sentry_native.dart
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,8 @@ class SentryNative {
/// Flag indicating if app start was already fetched.
bool get didFetchAppStart => _didFetchAppStart;

bool _didAddAppStartMeasurement = false;

/// Flag indicating if app start measurement was added to the first transaction.
bool get didAddAppStartMeasurement => _didAddAppStartMeasurement;

void setDidAddAppStartMeasurement(bool value) {
_didAddAppStartMeasurement = value;
}
bool didAddAppStartMeasurement = false;

/// Fetch [NativeAppStart] from native channels. Can only be called once.
Future<NativeAppStart?> fetchNativeAppStart() async {
Expand Down
9 changes: 2 additions & 7 deletions flutter/lib/src/sentry_flutter.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 DefaultFrameCallbackHandler();
} catch (_) {}
return null;
},
DefaultFrameCallbackHandler(),
));
}
return integrations;
Expand Down Expand Up @@ -231,6 +225,7 @@ mixin SentryFlutter {

@internal
static SentryNative? get native => _native;

@internal
static set native(SentryNative? value) => _native = value;
static SentryNative? _native;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ void main() {
expect(enriched.measurements.isEmpty, true);
});


test('native app start integration is called and sets app start info', () async {
test('native app start integration is called and sets app start info',
() async {
fixture.native.appStartEnd = DateTime.fromMillisecondsSinceEpoch(10);
fixture.binding.nativeAppStart = NativeAppStart(0, true);

Expand All @@ -119,10 +119,7 @@ class Fixture {
NativeAppStartIntegration getNativeAppStartIntegration() {
return NativeAppStartIntegration(
native,
() {
TestWidgetsFlutterBinding.ensureInitialized();
return FakeFrameCallbackHandler();
},
FakeFrameCallbackHandler(),
);
}

Expand Down
9 changes: 1 addition & 8 deletions flutter/test/mocks.dart
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,8 @@ class TestMockSentryNative implements SentryNative {
@override
bool get didFetchAppStart => _didFetchAppStart;

bool _didAddAppStartMeasurement = false;

@override
bool get didAddAppStartMeasurement => _didAddAppStartMeasurement;
bool didAddAppStartMeasurement = false;

Breadcrumb? breadcrumb;
var numberOfAddBreadcrumbCalls = 0;
Expand Down Expand Up @@ -295,11 +293,6 @@ class TestMockSentryNative implements SentryNative {
numberOfDiscardProfilerCalls++;
return Future.value(null);
}

@override
void setDidAddAppStartMeasurement(bool value) {
_didAddAppStartMeasurement = value;
}
}

// TODO can this be replaced with https://pub.dev/packages/mockito#verifying-exact-number-of-invocations--at-least-x--never
Expand Down

0 comments on commit cf5af40

Please sign in to comment.