Mark file sync spans run in the main isolate with blocked_main_thread (#3270)

Author: denrase

CHANGELOG.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## Unreleased
+
+### Features
+
+- Mark file sync spans run in the main isolate with `blocked_main_thread` ([#3270](https://github.com/getsentry/sentry-dart/pull/3270))
+ - This allows Sentry to create issues automatically out of file spans running a certain time on the main thread: https://docs.sentry.io/product/issues/issue-details/performance-issues/file-main-thread-io/
+
 ## 9.7.0
 
 

packages/dart/lib/src/protocol/sentry_span.dart

@@ -77,6 +77,13 @@ class SentrySpan extends ISentrySpan {
       }
     }
 
+    // Dispatch OnSpanFinish lifecycle event
+    final callback =
+        _hub.options.lifecycleRegistry.dispatchCallback(OnSpanFinish(this));
+    if (callback is Future) {
+      await callback;
+    }
+
     // The finished flag depends on the _endTimestamp
     // If we set this earlier then finished is true and then we cannot use setData etc...
     _endTimestamp = endTimestamp;

packages/dart/lib/src/sdk_lifecycle_hooks.dart

@@ -34,8 +34,17 @@ class SdkLifecycleRegistry {
     callbacks?.remove(callback);
   }
 
-  FutureOr<void> dispatchCallback<T extends SdkLifecycleEvent>(T event) async {
-    final callbacks = _lifecycleCallbacks[event.runtimeType] ?? [];
+  FutureOr<void> dispatchCallback<T extends SdkLifecycleEvent>(T event) {
+    final callbacks = _lifecycleCallbacks[event.runtimeType];
+    if (callbacks == null || callbacks.isEmpty) {
+      // Return synchronously when there are no callbacks to avoid unnecessary async boundary
+      return null;
+    }
+    return _dispatchCallbackAsync(event, callbacks);
+  }
+
+  Future<void> _dispatchCallbackAsync<T extends SdkLifecycleEvent>(
+      T event, List<Function> callbacks) async {
     for (final cb in callbacks) {
       try {
         final result = (cb as SdkLifecycleCallback<T>)(event);
@@ -79,3 +88,11 @@ class OnSpanStart extends SdkLifecycleEvent {
 
   final ISentrySpan span;
 }
+
+/// Dispatched when a sampled span is finished.
+@internal
+class OnSpanFinish extends SdkLifecycleEvent {
+  OnSpanFinish(this.span);
+
+  final ISentrySpan span;
+}

packages/dart/lib/src/span_data_convention.dart

@@ -10,6 +10,7 @@ class SpanDataConvention {
   // https://develop.sentry.dev/sdk/telemetry/traces/span-data-conventions/#thread
   static const threadId = 'thread.id';
   static const threadName = 'thread.name';
+  static const blockedMainThread = 'blocked_main_thread';
 
   // TODO: eventually add other data keys here as well
 }

packages/file/lib/src/sentry_file.dart

@@ -489,6 +489,7 @@ class SentryFile implements File {
 
     span?.origin = SentryTraceOrigins.autoFile;
     span?.setData('file.async', false);
+    span?.setData('sync', true);
 
     final Map<String, dynamic> breadcrumbData = {};
     breadcrumbData['file.async'] = false;

packages/file/test/sentry_file_test.dart

@@ -28,6 +28,9 @@ void main() {
       expect(span.context.operation, 'file.copy');
       expect(span.data['file.size'], 7);
       expect(span.data['file.async'], async);
+      if (!async) {
+        expect(span.data['sync'], true);
+      }
       expect(span.context.description, 'testfile.txt');
       expect(
           (span.data['file.path'] as String)
@@ -115,6 +118,9 @@ void main() {
       expect(span.context.operation, 'file.write');
       expect(span.data['file.size'], size);
       expect(span.data['file.async'], async);
+      if (!async) {
+        expect(span.data['sync'], true);
+      }
       expect(span.context.description, 'testfile_create.txt');
       expect(
           (span.data['file.path'] as String)
@@ -200,6 +206,9 @@ void main() {
       expect(span.context.operation, 'file.delete');
       expect(span.data['file.size'], size);
       expect(span.data['file.async'], async);
+      if (!async) {
+        expect(span.data['sync'], true);
+      }
       expect(span.context.description, 'testfile_delete.txt');
       expect(
           (span.data['file.path'] as String)
@@ -341,6 +350,9 @@ void main() {
       expect(span.context.operation, 'file.read');
       expect(span.data['file.size'], size);
       expect(span.data['file.async'], async);
+      if (!async) {
+        expect(span.data['sync'], true);
+      }
       expect(span.context.description, fileName);
       expect(
           (span.data['file.path'] as String)
@@ -492,6 +504,9 @@ void main() {
       expect(span.context.operation, 'file.rename');
       expect(span.data['file.size'], 0);
       expect(span.data['file.async'], async);
+      if (!async) {
+        expect(span.data['sync'], true);
+      }
       expect(span.context.description, name);
       expect(
           (span.data['file.path'] as String).endsWith('test_resources/$name'),
@@ -580,6 +595,9 @@ void main() {
       final span = call.transaction.spans.first;
 
       expect(span.data['file.async'], async);
+      if (!async) {
+        expect(span.data['sync'], true);
+      }
       expect(span.data['file.path'], null);
       expect(span.origin, SentryTraceOrigins.autoFile);
     }

packages/flutter/lib/src/integrations/thread_info_integration.dart

@@ -3,6 +3,7 @@
 import 'package:meta/meta.dart';
 
 import '../../sentry_flutter.dart';
+// ignore: implementation_imports
 import '../isolate/isolate_helper.dart';
 
 /// Integration for adding thread information to spans.
@@ -33,6 +34,8 @@ class ThreadInfoIntegration implements Integration<SentryFlutterOptions> {
 
     options.lifecycleRegistry
         .registerCallback<OnSpanStart>(_addThreadInfoToSpan);
+    options.lifecycleRegistry
+        .registerCallback<OnSpanFinish>(_processSyncSpanOnFinish);
 
     options.sdk.addIntegration(integrationName);
   }
@@ -41,6 +44,8 @@ class ThreadInfoIntegration implements Integration<SentryFlutterOptions> {
   void close() {
     _hub?.options.lifecycleRegistry
         .removeCallback<OnSpanStart>(_addThreadInfoToSpan);
+    _hub?.options.lifecycleRegistry
+        .removeCallback<OnSpanFinish>(_processSyncSpanOnFinish);
   }
 
   Future<void> _addThreadInfoToSpan(OnSpanStart event) async {
@@ -65,4 +70,25 @@ class ThreadInfoIntegration implements Integration<SentryFlutterOptions> {
       span.setData(SpanDataConvention.threadName, threadName);
     }
   }
+
+  void _processSyncSpanOnFinish(OnSpanFinish event) {
+    final span = event.span;
+    if (span is! SentrySpan) {
+      return;
+    }
+
+    final data = span.data;
+
+    // Check if this is a sync operation
+    if (data.containsKey('sync')) {
+      // Check if we're on the main isolate by looking at thread name
+      if (data['sync'] == true &&
+          data[SpanDataConvention.threadName] == 'main') {
+        span.setData(SpanDataConvention.blockedMainThread, true);
+      }
+
+      // Always remove the sync flag
+      span.removeData('sync');
+    }
+  }
 }

packages/flutter/test/integrations/thread_info_integration_test.dart

@@ -171,6 +171,122 @@ void main() {
       expect(span.setDataCalls, isEmpty);
     });
   });
+
+  group('OnSpanFinish sync processing', () {
+    test('sets blocked_main_thread when sync span finishes on main isolate',
+        () async {
+      fixture.mockHelper.setIsRootIsolate(true);
+
+      final hub = fixture.createHub();
+      final integration = fixture.getSut();
+      final span = fixture.createMockSpanWithData({
+        'sync': true,
+        SpanDataConvention.threadName: 'main',
+      });
+
+      integration.call(hub, fixture.options);
+      // ignore: invalid_use_of_internal_member
+      await fixture.options.lifecycleRegistry
+          .dispatchCallback(OnSpanFinish(span));
+
+      final setDataCalls = span.setDataCalls;
+      expect(setDataCalls.length, equals(1));
+
+      final blockedMainThreadCall = setDataCalls.firstWhere(
+          (call) => call.key == SpanDataConvention.blockedMainThread);
+      expect(blockedMainThreadCall.value, equals(true));
+
+      // Check that sync was removed
+      expect(span.removeDataCalls.length, equals(1));
+      expect(span.removeDataCalls.first.key, equals('sync'));
+    });
+
+    test(
+        'does not set blocked_main_thread when sync span finishes on background isolate',
+        () async {
+      final hub = fixture.createHub();
+      final integration = fixture.getSut();
+      final span = fixture.createMockSpanWithData({
+        'sync': true,
+        SpanDataConvention.threadName: 'worker-thread',
+      });
+
+      integration.call(hub, fixture.options);
+      // ignore: invalid_use_of_internal_member
+      await fixture.options.lifecycleRegistry
+          .dispatchCallback(OnSpanFinish(span));
+
+      // Should not set blocked_main_thread
+      final blockedMainThreadCalls = span.setDataCalls
+          .where((call) => call.key == SpanDataConvention.blockedMainThread);
+      expect(blockedMainThreadCalls, isEmpty);
+
+      // But should still remove sync
+      expect(span.removeDataCalls.length, equals(1));
+      expect(span.removeDataCalls.first.key, equals('sync'));
+    });
+
+    test('does not process spans without sync data', () async {
+      final hub = fixture.createHub();
+      final integration = fixture.getSut();
+      final span = fixture.createMockSpanWithData({
+        SpanDataConvention.threadName: 'main',
+      });
+
+      integration.call(hub, fixture.options);
+      // ignore: invalid_use_of_internal_member
+      await fixture.options.lifecycleRegistry
+          .dispatchCallback(OnSpanFinish(span));
+
+      // Should not add any data or remove anything
+      expect(span.setDataCalls, isEmpty);
+      expect(span.removeDataCalls, isEmpty);
+    });
+
+    test('removes sync flag even when sync is false', () async {
+      final hub = fixture.createHub();
+      final integration = fixture.getSut();
+      final span = fixture.createMockSpanWithData({
+        'sync': false,
+        SpanDataConvention.threadName: 'main',
+      });
+
+      integration.call(hub, fixture.options);
+      // ignore: invalid_use_of_internal_member
+      await fixture.options.lifecycleRegistry
+          .dispatchCallback(OnSpanFinish(span));
+
+      // Should not set blocked_main_thread (sync is false)
+      final blockedMainThreadCalls = span.setDataCalls
+          .where((call) => call.key == SpanDataConvention.blockedMainThread);
+      expect(blockedMainThreadCalls, isEmpty);
+
+      // But should still remove sync
+      expect(span.removeDataCalls.length, equals(1));
+      expect(span.removeDataCalls.first.key, equals('sync'));
+    });
+
+    test('does not set blocked_main_thread when sync span has no thread name',
+        () async {
+      final hub = fixture.createHub();
+      final integration = fixture.getSut();
+      final span = fixture.createMockSpanWithData({'sync': true});
+
+      integration.call(hub, fixture.options);
+      // ignore: invalid_use_of_internal_member
+      await fixture.options.lifecycleRegistry
+          .dispatchCallback(OnSpanFinish(span));
+
+      // Should not set blocked_main_thread (no thread name)
+      final blockedMainThreadCalls = span.setDataCalls
+          .where((call) => call.key == SpanDataConvention.blockedMainThread);
+      expect(blockedMainThreadCalls, isEmpty);
+
+      // But should still remove sync
+      expect(span.removeDataCalls.length, equals(1));
+      expect(span.removeDataCalls.first.key, equals('sync'));
+    });
+  });
 }
 
 class _Fixture {
@@ -194,6 +310,10 @@ class _Fixture {
     return _MockSpan();
   }
 
+  _MockSpan createMockSpanWithData(Map<String, dynamic> data) {
+    return _MockSpan.withData(data);
+  }
+
   MockHub createHub() {
     final hub = MockHub();
     when(hub.options).thenReturn(options);
@@ -218,13 +338,31 @@ class _MockIsolateHelper extends Mock implements IsolateHelper {
 class _MockSpan extends Mock implements SentrySpan {
   final SentrySpanContext _context = SentrySpanContext(operation: 'test');
   final List<_SetDataCall> setDataCalls = [];
+  final List<_RemoveDataCall> removeDataCalls = [];
+  final Map<String, dynamic> _data = {};
+
+  _MockSpan();
+
+  _MockSpan.withData(Map<String, dynamic> data) {
+    _data.addAll(data);
+  }
 
   @override
   SentrySpanContext get context => _context;
 
+  @override
+  Map<String, dynamic> get data => _data;
+
   @override
   void setData(String key, dynamic value) {
     setDataCalls.add(_SetDataCall(key, value));
+    _data[key] = value;
+  }
+
+  @override
+  void removeData(String key) {
+    removeDataCalls.add(_RemoveDataCall(key));
+    _data.remove(key);
   }
 }
 
@@ -234,3 +372,9 @@ class _SetDataCall {
 
   _SetDataCall(this.key, this.value);
 }
+
+class _RemoveDataCall {
+  final String key;
+
+  _RemoveDataCall(this.key);
+}