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.

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

Add blocked_main_thread to sync file spans #1801

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

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Jan 2, 2024

📜 Description

Add blocked_main_thread to sync file spans

💡 Motivation and Context

Relates to #1727

💚 How did you test it?

Unit tests

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

Copy link
Contributor

github-actions bot commented Jan 8, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 405.09 ms 476.65 ms 71.57 ms
Size 6.33 MiB 7.26 MiB 950.19 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
636cb61 366.59 ms 448.14 ms 81.55 ms
4b29d6e 386.80 ms 430.86 ms 44.06 ms
0be962b 325.54 ms 382.83 ms 57.29 ms
7f75f32 347.36 ms 419.58 ms 72.22 ms
64e4616 349.82 ms 436.96 ms 87.14 ms
683fd34 336.53 ms 418.10 ms 81.57 ms
a7acb24 301.00 ms 357.38 ms 56.38 ms
8932ece 309.56 ms 377.28 ms 67.72 ms
1b66358 314.96 ms 363.39 ms 48.43 ms
df16b96 326.08 ms 391.82 ms 65.74 ms

App size

Revision Plain With Sentry Diff
636cb61 6.27 MiB 7.20 MiB 959.08 KiB
4b29d6e 6.33 MiB 7.26 MiB 946.14 KiB
0be962b 6.06 MiB 7.03 MiB 990.29 KiB
7f75f32 6.26 MiB 7.20 MiB 959.18 KiB
64e4616 6.27 MiB 7.20 MiB 956.52 KiB
683fd34 6.27 MiB 7.20 MiB 960.43 KiB
a7acb24 5.94 MiB 6.95 MiB 1.01 MiB
8932ece 6.16 MiB 7.13 MiB 1000.89 KiB
1b66358 6.06 MiB 7.09 MiB 1.03 MiB
df16b96 6.06 MiB 7.03 MiB 988.94 KiB

Previous results on branch: enha/file-isolate-name

Startup times

Revision Plain With Sentry Diff
906e92a 619.53 ms 714.16 ms 94.63 ms
7e4b32b 445.00 ms 546.73 ms 101.73 ms
053d6e5 379.48 ms 430.35 ms 50.87 ms
03663b0 407.98 ms 502.52 ms 94.54 ms
53df9a1 377.64 ms 453.04 ms 75.40 ms
1627c58 335.62 ms 413.39 ms 77.77 ms

App size

Revision Plain With Sentry Diff
906e92a 6.33 MiB 7.26 MiB 950.38 KiB
7e4b32b 6.33 MiB 7.26 MiB 950.12 KiB
053d6e5 6.33 MiB 7.26 MiB 949.77 KiB
03663b0 6.33 MiB 7.26 MiB 950.37 KiB
53df9a1 6.33 MiB 7.26 MiB 949.77 KiB
1627c58 6.33 MiB 7.26 MiB 950.14 KiB

Copy link
Contributor

github-actions bot commented Jan 8, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.13 ms 1242.44 ms 17.31 ms
Size 8.32 MiB 9.38 MiB 1.06 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
117d988 1200.83 ms 1223.24 ms 22.41 ms
955541a 1230.22 ms 1252.96 ms 22.73 ms
f83bc1d 1247.12 ms 1249.78 ms 2.65 ms
5e8d2b3 1255.71 ms 1267.98 ms 12.27 ms
86d4841 1225.69 ms 1241.12 ms 15.43 ms
68677de 1245.20 ms 1273.51 ms 28.31 ms
3de8b9b 1234.22 ms 1251.94 ms 17.72 ms
1cdcacf 1208.35 ms 1235.13 ms 26.77 ms
0ceb89c 1252.02 ms 1271.78 ms 19.75 ms
8932ece 1234.31 ms 1238.90 ms 4.59 ms

App size

Revision Plain With Sentry Diff
117d988 8.32 MiB 9.38 MiB 1.05 MiB
955541a 8.28 MiB 9.34 MiB 1.06 MiB
f83bc1d 8.28 MiB 9.34 MiB 1.05 MiB
5e8d2b3 8.29 MiB 9.36 MiB 1.07 MiB
86d4841 8.29 MiB 9.36 MiB 1.07 MiB
68677de 8.10 MiB 9.16 MiB 1.07 MiB
3de8b9b 8.28 MiB 9.34 MiB 1.06 MiB
1cdcacf 8.32 MiB 9.39 MiB 1.06 MiB
0ceb89c 8.15 MiB 9.12 MiB 989.78 KiB
8932ece 8.29 MiB 9.36 MiB 1.07 MiB

Previous results on branch: enha/file-isolate-name

Startup times

Revision Plain With Sentry Diff
906e92a 1226.29 ms 1247.96 ms 21.67 ms
7e4b32b 1296.69 ms 1352.51 ms 55.82 ms
03663b0 1242.71 ms 1265.73 ms 23.01 ms
53df9a1 1214.55 ms 1242.00 ms 27.45 ms
1627c58 1215.58 ms 1229.35 ms 13.76 ms
053d6e5 1207.80 ms 1227.45 ms 19.65 ms

App size

Revision Plain With Sentry Diff
906e92a 8.32 MiB 9.38 MiB 1.06 MiB
7e4b32b 8.32 MiB 9.38 MiB 1.06 MiB
03663b0 8.32 MiB 9.38 MiB 1.06 MiB
53df9a1 8.32 MiB 9.39 MiB 1.06 MiB
1627c58 8.32 MiB 9.38 MiB 1.06 MiB
053d6e5 8.32 MiB 9.38 MiB 1.06 MiB

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (42b79b3) 91.17% compared to head (a5d3d15) 88.55%.
Report is 12 commits behind head on main.

Files Patch % Lines
dart/lib/src/sentry_options.dart 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1801      +/-   ##
==========================================
- Coverage   91.17%   88.55%   -2.63%     
==========================================
  Files         172      207      +35     
  Lines        5418     6901    +1483     
==========================================
+ Hits         4940     6111    +1171     
- Misses        478      790     +312     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@denrase
Copy link
Collaborator Author

denrase commented Jan 8, 2024

@buenaflor I'm not sure we can guarantee that the debugName will always return main on the main isolate. AFAIK there is no other way to check in dart only code.

@buenaflor
Copy link
Contributor

@denrase do we know any scenarios where it won't work?

@ueman
Copy link
Collaborator

ueman commented Jan 9, 2024

@denrase do we know any scenarios where it won't work?

Probably when someone calls PlatformDispatcher.setIsolateDebugName() in the main isolate from Flutter, but it's probably not that likely. I'm not sure for Dart only code though.

@buenaflor
Copy link
Contributor

Got it, If we can't guarantee that it's always correct maybe it should be opt-in?

How to proceed with backend implementation?

https://docs.sentry.io/product/issues/issue-details/performance-issues/file-main-thread-io/

it should already be in place and all we need is to add the correct span data. If I looked correctly it should be

blocked_main_thread: true/false

examples: java, cocoa

file/lib/src/sentry_file.dart Outdated Show resolved Hide resolved
@denrase denrase changed the title Add isolate name to sync file spans Add blocked_main_thread name to sync file spans Jan 16, 2024
@denrase denrase changed the title Add blocked_main_thread name to sync file spans Add blocked_main_thread to sync file spans Jan 16, 2024
@denrase
Copy link
Collaborator Author

denrase commented Jan 16, 2024

@buenaflor Think i found a good solution. Added a callback to options to check for the main Isolate. When using the dart SDK we use the debugName, and on flutter we use ServicesBinding.rootIsolateToken to determine if we are on the main Isolate. So for flutter this should always be correct, and in dart only it should be correct as long as 'main' is returned as a value.

@denrase denrase marked this pull request as ready for review January 16, 2024 17:31
@buenaflor
Copy link
Contributor

ServicesBinding.rootIsolateToken is only available on Flutter >= 3.7.0.
See here: look for flutter/flutter#111712

I guess we need to fall back to debugName if someone is using a lower Flutter version.

Maybe we can leverage the dynamic - NoSuchMethod approach again here?

@denrase
Copy link
Collaborator Author

denrase commented Jan 22, 2024

@buenaflor It looks like this won't work in this case, as this is a static property on on ServicesBinding. So we don't have an instance which we can cast to dynamic.

Is there another way to check this? 🤔

If not, it looks like we can only fall back to the Isolate debug name.

@denrase
Copy link
Collaborator Author

denrase commented Jan 22, 2024

@buenaflor Removed ServicesBinding.rootIsolateToken check for now, since I'm not aware how to call static API that is not present in older flutter versions. Maybe @marandaneto has an idea?

@marandaneto
Copy link
Contributor

marandaneto commented Jan 22, 2024

@buenaflor Removed ServicesBinding.rootIsolateToken check for now, since I'm not aware how to call static API that is not present in older flutter versions. Maybe @marandaneto has an idea?

All rootIsolateToken does is to call RootIsolateToken.instance and this class and property exists in older versions, right?
All you need is to result = RootIsolateToken.instance != null I guess?

@denrase
Copy link
Collaborator Author

denrase commented Jan 22, 2024

@marandaneto I checked andRootIsolateToken.instance was introduced to flutter/engine on 8. September 2022 as far as I can tell. I did not find any direct reference to it in release notes in the repos.

flutter/engine#35174

Flutter 3.3.2, released on 14.9.2022, was the first flutter release after this PR merge.

So I'm guessig that flutter versions < 3.3.2 do not have this API, and per our oubspec.yaml we support Flutter >= 3.0.0.

I have not seen the class definition in other repositories, expept in dart-lang/flute, which is used for benchmarking.

I wanted to test this and downgrade to Flutter 3, but with no luck so far on macOS.

@marandaneto
Copy link
Contributor

@marandaneto I checked andRootIsolateToken.instance was introduced to flutter/engine on 8. September 2022 as far as I can tell. I did not find any direct reference to it in release notes in the repos.

flutter/engine#35174

Flutter 3.3.2, released on 14.9.2022, was the first flutter release after this PR merge.

So I'm guessig that flutter versions < 3.3.2 do not have this API, and per our oubspec.yaml we support Flutter >= 3.0.0.

I have not seen the class definition in other repositories, expept in dart-lang/flute, which is used for benchmarking.

I wanted to test this and downgrade to Flutter 3, but with no luck so far on macOS.

https://github.com/leoafarias/fvm

@denrase
Copy link
Collaborator Author

denrase commented Jan 23, 2024

Like expected, we do not have access to RootIsolateToken. Installed Flutter version 3.3.10. Had to remove several sentry plugins from the example app, as those do not all support the same low SDK versions as the main package to finally get the compilation error.

denis@Deniss-MBP example % fvm doctor

FVM Version: 2.4.1
___________________________________________________

FVM config found:
___________________________________________________

Project: example
Directory: /Users/denis/Repos/sentry/sentry-dart/flutter/example
Version: 3.3.10
Project Flavor: None selected
___________________________________________________

Version is currently cached locally.

Cache Path: /Users/denis/fvm/versions/3.3.10
Channel: false
SDK Version: 3.3.10

IDE Links
VSCode: .fvm/flutter_sdk
Android Studio: /Users/denis/Repos/sentry/sentry-dart/flutter/example/.fvm/flutter_sdk


Configured env paths:
___________________________________________________

Flutter:
/Users/denis/Repos/flutter/bin/flutter

Dart:
/opt/homebrew/bin/dart

FVM_HOME:
not set

denis@Deniss-MBP example % fvm flutter run -d 6B9B6A68-2804-4447-9959-3CCAB49E9501
Launching lib/main.dart on iPhone 15 Pro in debug mode...
Running Xcode build...                                          
Xcode build done.                                            8,5s
Failed to build iOS app
Error output from Xcode build:
↳
    --- xcodebuild: WARNING: Using the first of multiple matching destinations:
    { platform:iOS Simulator, id:6B9B6A68-2804-4447-9959-3CCAB49E9501, OS:17.2, name:iPhone 15 Pro }
    { platform:iOS Simulator, id:6B9B6A68-2804-4447-9959-3CCAB49E9501, OS:17.2, name:iPhone 15 Pro }
    ** BUILD FAILED **


Xcode's output:
↳
    Writing result bundle at path:
    	/var/folders/_n/s4t83kqj137f_9465c5z63k40000gn/T/flutter_tools.3jOctp/flutter_ios_build_temp_dirGarRkE/temporary_xcresult_bundle

    ../lib/src/sentry_flutter.dart:127:14: Error: Undefined name 'RootIsolateToken'.
          return RootIsolateToken.instance != null;
                 ^^^^^^^^^^^^^^^^
    Failed to package /Users/denis/Repos/sentry/sentry-dart/flutter/example.
    Command PhaseScriptExecution failed with a nonzero exit code
    note: Run script build phase 'Run Script' will be run during every build because the option to run the script phase "Based on dependency analysis" is unchecked. (in target 'Runner' from project 'Runner')
    note: Run script build phase 'Thin Binary' will be run during every build because the option to run the script phase "Based on dependency analysis" is unchecked. (in target 'Runner' from project 'Runner')

Could not build the application for the simulator.
Error launching application on iPhone 15 Pro.
denis@Deniss-MBP example % 

@marandaneto
Copy link
Contributor

RootIsolateToken.instance

@denrase background isolates is Flutter >= 3.7
So in this case, either do not use this class/field and do not set the blocked_main_thread at all or bump the min. Flutter version.
Isolate.current.debugName is mutable so it can lead to false positives since it can be changed.

@@ -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! 👍

@denrase
Copy link
Collaborator Author

denrase commented Jan 23, 2024

@marandaneto Exactly.

  • Can't use [rootIsolateToken](https://api.flutter.dev/flutter/services/ServicesBinding/rootIsolateToken.html) without bumping the Flutter SDK.
  • It is a static and we can't use the (instance as dynamic).anyMethod approach for versions < 3.7
  • Using Isolate.current.debugName is not reliable.

Only way as I see it to reliably and automatically detect this is to bump to Flutter 3.7

@buenaflor buenaflor marked this pull request as draft January 25, 2024 15:29
@denrase denrase marked this pull request as ready for review January 30, 2024 12:04
@denrase
Copy link
Collaborator Author

denrase commented Jan 30, 2024

@buenaflor The PR still has the caveat that users might override the debugName we are using.

#1727 (comment)

@denrase denrase removed the Blocked label Jan 30, 2024
@buenaflor
Copy link
Contributor

Let's keep this blocked by flutter min 3.7 wdyt? @denrase

@denrase
Copy link
Collaborator Author

denrase commented Feb 5, 2024

@buenaflor Sounds good to me. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants