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

File I/O on main thread #2382

Merged
merged 15 commits into from Dec 15, 2022
Merged

File I/O on main thread #2382

merged 15 commits into from Dec 15, 2022

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Nov 21, 2022

📜 Description

  • Adds necessary data to File I/O spans for creating performance issues out of them
    • Adds blocked_ui_thread boolean flag indicating whether the span has run its entire lifespan on the main thread
    • Adds call_stack array which contains stack frames leading to the File I/O span for proper grouping of the issues
  • Attaches debug_meta on the SentryBaseEvent level now, to also send it with transactions and not only events.

💡 Motivation and Context

Q4 Goal
rfc: getsentry/rfcs#36

💚 How did you test it?

Manually, automated tests will follow

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Nov 21, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 13165f0

@github-actions
Copy link
Contributor

github-actions bot commented Nov 21, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 290.84 ms 336.36 ms 45.52 ms
Size 1.73 MiB 2.33 MiB 615.62 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b85d8aa 289.35 ms 335.92 ms 46.56 ms
1e1ab7f 383.48 ms 441.37 ms 57.89 ms
703d523 358.00 ms 369.94 ms 11.94 ms
7967d22 289.28 ms 377.11 ms 87.83 ms
3695453 301.78 ms 371.14 ms 69.36 ms
3695453 314.63 ms 353.10 ms 38.47 ms
4a9c176 320.62 ms 334.68 ms 14.06 ms
31f3e4c 325.22 ms 342.77 ms 17.55 ms
4a9c176 336.33 ms 384.73 ms 48.41 ms
4a9c176 319.77 ms 363.20 ms 43.43 ms

App size

Revision Plain With Sentry Diff
b85d8aa 1.73 MiB 2.32 MiB 611.62 KiB
1e1ab7f 1.73 MiB 2.32 MiB 612.36 KiB
703d523 1.73 MiB 2.33 MiB 613.23 KiB
7967d22 1.73 MiB 2.32 MiB 612.47 KiB
3695453 1.73 MiB 2.32 MiB 611.62 KiB
3695453 1.73 MiB 2.32 MiB 611.62 KiB
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
31f3e4c 1.73 MiB 2.32 MiB 612.47 KiB
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB

Previous results on branch: feat/file-io-main-thread

Startup times

Revision Plain With Sentry Diff
9fdf794 275.04 ms 346.22 ms 71.18 ms
48e9f36 314.20 ms 370.79 ms 56.60 ms
0f9e808 329.47 ms 346.92 ms 17.45 ms
d92d9f7 317.26 ms 355.65 ms 38.39 ms

App size

Revision Plain With Sentry Diff
9fdf794 1.73 MiB 2.32 MiB 612.07 KiB
48e9f36 1.73 MiB 2.33 MiB 615.62 KiB
0f9e808 1.73 MiB 2.32 MiB 612.07 KiB
d92d9f7 1.73 MiB 2.33 MiB 615.62 KiB

@codecov-commenter
Copy link

Codecov Report

Base: 80.28% // Head: 80.23% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (24e5ea7) compared to base (d49f98e).
Patch coverage: 80.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2382      +/-   ##
============================================
- Coverage     80.28%   80.23%   -0.06%     
- Complexity     3690     3707      +17     
============================================
  Files           292      295       +3     
  Lines         13845    13896      +51     
  Branches       1833     1847      +14     
============================================
+ Hits          11116    11149      +33     
- Misses         2014     2025      +11     
- Partials        715      722       +7     
Impacted Files Coverage Δ
...c/main/java/io/sentry/SentryStackTraceFactory.java 92.10% <ø> (ø)
...y/instrumentation/file/SentryFileOutputStream.java 45.28% <50.00%> (-0.18%) ⬇️
...java/io/sentry/util/thread/IMainThreadChecker.java 50.00% <50.00%> (ø)
sentry/src/main/java/io/sentry/Sentry.java 54.69% <66.66%> (+0.20%) ⬆️
...sentry/instrumentation/file/FileIOSpanManager.java 84.12% <73.91%> (-4.25%) ⬇️
.../java/io/sentry/util/thread/MainThreadChecker.java 75.00% <75.00%> (ø)
sentry/src/main/java/io/sentry/SentryOptions.java 81.26% <100.00%> (+0.18%) ⬆️
.../instrumentation/file/FileInputStreamInitData.java 100.00% <100.00%> (ø)
...instrumentation/file/FileOutputStreamInitData.java 100.00% <100.00%> (ø)
...ry/instrumentation/file/SentryFileInputStream.java 68.08% <100.00%> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@romtsn romtsn marked this pull request as ready for review December 15, 2022 12:09
Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Nicely done, please have a look at my comments 🚀

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Nice! :shipit:

@romtsn romtsn merged commit 81a3c32 into main Dec 15, 2022
@romtsn romtsn deleted the feat/file-io-main-thread branch December 15, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants