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.

Sign up for GitHub

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

Jump to bottom

Add blocked_main_thread to sync file spans #1801

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

Add blocked_main_thread to sync file spans #1801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
7f22e0e
Add isolate debug name to sync file span
denrase Jan 2, 2024
e38f8e4
Add changelog entry
denrase Jan 2, 2024
053d6e5
Merge branch 'main' into enha/file-isolate-name
denrase Jan 8, 2024
3202d56
check against Isolate debug name
denrase Jan 8, 2024
9733286
Merge branch 'main' into enha/file-isolate-name
denrase Jan 8, 2024
53df9a1
update chengelog
denrase Jan 8, 2024
6ecb1c9
Merge branch 'main' into enha/file-isolate-name
denrase Jan 9, 2024
1627c58
Merge branch 'main' into enha/file-isolate-name
denrase Jan 16, 2024
5ee0983
inject isolate check, use blocked_main_thread
denrase Jan 16, 2024
39cf7f7
move callback to options
denrase Jan 16, 2024
33d40b1
Use `ServicesBinding.rootIsolateToken` to check for main isolate in f…
denrase Jan 16, 2024
e6c860a
Update changelog entry
denrase Jan 16, 2024
7e4b32b
add newline
denrase Jan 16, 2024
691d564
Merge branch 'main' into enha/file-isolate-name
denrase Jan 17, 2024
e36a618
fix changelog
denrase Jan 17, 2024
fb7e787
Remove `ServicesBinding.rootIsolateToken` check in flutter options
denrase Jan 22, 2024
aebaa8a
Merge branch 'main' into enha/file-isolate-name
denrase Jan 23, 2024
906e92a
fix changelog
denrase Jan 23, 2024
9683d1d
Merge branch 'main' into enha/file-isolate-name
denrase Jan 29, 2024
03663b0
add integration test in sample app
denrase Jan 29, 2024
a5d3d15
Merge branch 'main' into enha/file-isolate-name
denrase Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
- Method `hint.addAll(Map<String, dynamic> keysAndValues)` now takes `Map<String, dynamic>` instead of `Map<String, Object>`
- Method `set(String key, dynamic value)` now takes value of `dynamic` instead of `Object`
- Method `hint.get(String key)` now returns `dynamic` instead of `Object?`
- Add `blocked_main_thread` to sync file spans ([#1801](https://github.com/getsentry/sentry-dart/pull/1801))

## 7.15.0

Expand All @@ -27,7 +28,7 @@
- Starting with Flutter 3.16, Sentry adds the [`appFlavor`](https://api.flutter.dev/flutter/services/appFlavor-constant.html) to the `flutter_context` ([#1799](https://github.com/getsentry/sentry-dart/pull/1799))
- Add beforeScreenshotCallback to SentryFlutterOptions ([#1805](https://github.com/getsentry/sentry-dart/pull/1805))
- Add support for `readTransaction` in `sqflite` ([#1819](https://github.com/getsentry/sentry-dart/pull/1819))

### Dependencies

- Bump Android SDK from v7.0.0 to v7.2.0 ([#1788](https://github.com/getsentry/sentry-dart/pull/1788), [#1815](https://github.com/getsentry/sentry-dart/pull/1815))
Expand Down
9 changes: 9 additions & 0 deletions dart/lib/src/sentry_options.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import 'dart:async';
import 'dart:developer';
import 'dart:isolate';

import 'package:meta/meta.dart';
import 'package:http/http.dart';
Expand Down Expand Up @@ -446,6 +447,11 @@
late SentryStackTraceFactory stackTraceFactory =
SentryStackTraceFactory(this);

@internal
IsMainIsolateCallback isMainIsolate = () {
return Isolate.current.debugName == 'main';

Check warning on line 452 in dart/lib/src/sentry_options.dart

View check run for this annotation

Codecov / codecov/patch

dart/lib/src/sentry_options.dart#L451-L452 

Added lines #L451 - L452 were not covered by tests
};

void _debugLogger(
SentryLevel level,
String message, {
Expand Down Expand Up @@ -527,3 +533,6 @@
stackTrace: stackTrace,
);
}

/// A callback used to check if we are on the main isolate.
typedef IsMainIsolateCallback = bool Function();
3 changes: 3 additions & 0 deletions file/lib/src/sentry_file.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,9 @@ class SentryFile implements File {
span?.origin = SentryTraceOrigins.autoFile;
span?.setData('file.async', false);

final isMainIsolate = _hub.options.isMainIsolate();
span?.setData('blocked_main_thread', isMainIsolate);

final Map<String, dynamic> breadcrumbData = {};
breadcrumbData['file.async'] = false;

Expand Down
284 changes: 284 additions & 0 deletions file/test/sentry_file_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,286 @@ void main() {
);
});
});

group('$SentryFile span sets `blocked_main_thread`', () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests are using the isMainIsolate fake but not starting a real background isolate and reading a file from it to verify that indeed returns true or false correctly.
So it is hard to tell that the solution works.
I'd write a test that creates a background isolate and wait for its result, and asserting that the span contains the right result.
This test could be done on the flutter integration test for example if needs to be, or even the example app, since there the flutter min. version is higher or can be higher

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added two tests to the example app. The first runs on the main isolate and the second init's sentry only on a bg isolate and runs there. Results as expected, thx for the suggestion! 👍

late Fixture fixture;

setUp(() {
fixture = Fixture();
});

tearDown(() {});

void _assertSpan(bool isMainIsolate) {
final call = fixture.client.captureTransactionCalls.first;
final span = call.transaction.spans.first;
expect(span.data['blocked_main_thread'], isMainIsolate);
}

test('to `true` when calling `copySync` on main', () async {
final file = File('test_resources/testfile.txt');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: true,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
sut.copySync('test_resources/testfile_copy.txt');
await tr.finish();

_assertSpan(true);
});

test('to `false` when not calling `copySync` on main', () async {
final file = File('test_resources/testfile.txt');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: false,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
final newFile = sut.copySync('test_resources/testfile_copy.txt');
await tr.finish();

_assertSpan(false);

newFile.deleteSync();
});

test('to `true` when calling `createSync` on main', () async {
final file = File('test_resources/testfile.txt');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: true,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
sut.createSync();
await tr.finish();

_assertSpan(true);
});

test('to `false` when not calling `createSync` on main', () async {
final file = File('test_resources/testfile.txt');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: false,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
sut.createSync();
await tr.finish();

_assertSpan(false);
});

// deleteSync

test('to `true` when calling `deleteSync` on main', () async {
final file = File('test_resources/testfile_delete.txt');
file.createSync();
expect(file.existsSync(), true);

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: true,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
sut.deleteSync();
await tr.finish();

_assertSpan(true);
});

test('to `false` when not calling `deleteSync` on main', () async {
final file = File('test_resources/testfile_delete.txt');
file.createSync();
expect(file.existsSync(), true);

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: false,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
sut.deleteSync();
await tr.finish();

_assertSpan(false);
});

// readAsBytesSync

test('to `true` when calling `readAsBytesSync` on main', () async {
final file = File('test_resources/sentry.png');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: true,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
sut.readAsBytesSync();
await tr.finish();

_assertSpan(true);
});

test('to `false` when not calling `readAsBytesSync` on main', () async {
final file = File('test_resources/testfile.txt');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: false,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
sut.readAsBytesSync();
await tr.finish();

_assertSpan(false);
});

// readAsLinesSync

test('to `true` when calling `readAsLinesSync` on main', () async {
final file = File('test_resources/testfile.txt');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: true,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);

sut.readAsLinesSync();

await tr.finish();

_assertSpan(true);
});

test('to `false` when not calling `readAsLinesSync` on main', () async {
final file = File('test_resources/testfile.txt');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: false,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
sut.readAsLinesSync();
await tr.finish();

_assertSpan(false);
});

// readAsStringSync

test('to `true` when calling `readAsStringSync` on main', () async {
final file = File('test_resources/testfile.txt');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: true,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);

sut.readAsStringSync();

await tr.finish();

_assertSpan(true);
});

test('to `false` when not calling `readAsStringSync` on main', () async {
final file = File('test_resources/testfile.txt');

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: false,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
sut.readAsStringSync();
await tr.finish();

_assertSpan(false);
});

// renameSync

test('to `true` when calling `renameSync` on main', () async {
final file = File('test_resources/old_name.txt');
file.createSync();

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: true,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
final newFile = sut.renameSync('test_resources/testfile_copy.txt');
await tr.finish();

_assertSpan(true);

newFile.deleteSync();
});

test('to `false` when not calling `renameSync` on main', () async {
final file = File('test_resources/old_name.txt');
file.createSync();

final sut = fixture.getSut(
file,
sendDefaultPii: true,
tracesSampleRate: 1.0,
isMainIsolate: false,
);

final tr = fixture.hub.startTransaction('name', 'op', bindToScope: true);
final newFile = sut.renameSync('test_resources/testfile_copy.txt');
await tr.finish();

_assertSpan(false);

newFile.deleteSync();
});
});
}

class Fixture {
Expand All @@ -667,10 +947,14 @@ class Fixture {
SentryFile getSut(
File file, {
bool sendDefaultPii = false,
bool isMainIsolate = false,
double? tracesSampleRate,
}) {
options.sendDefaultPii = sendDefaultPii;
options.tracesSampleRate = tracesSampleRate;
options.isMainIsolate = () {
return isMainIsolate;
};

hub = Hub(options);
hub.bindClient(client);
Expand Down
Loading
Loading