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

Fix image flickering when using SentryAssetBundle #2577

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

denrase
Copy link
Collaborator

@denrase denrase commented Jan 14, 2025

📜 Description

We cannot await within futures, or asset loading will lead to "flickering". This PR replaces async/await with Completers who check if they are sync from the call order.

💡 Motivation and Context

Closes #2528

💚 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

🔮 Next steps

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 95.58824% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.49%. Comparing base (a22e451) to head (e003cac).

Files with missing lines Patch % Lines
flutter/lib/src/sentry_asset_bundle.dart 95.58% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2577      +/-   ##
==========================================
+ Coverage   88.96%   92.49%   +3.52%     
==========================================
  Files         262       91     -171     
  Lines        8957     2970    -5987     
==========================================
- Hits         7969     2747    -5222     
+ Misses        988      223     -765     

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

@denrase
Copy link
Collaborator Author

denrase commented Jan 14, 2025

@buenaflor Do we have some larger sample that is loading multiple assets where we known hot the spans are supposed to look like on sentry.io?

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 452.52 ms 492.86 ms 40.34 ms
Size 6.46 MiB 7.48 MiB 1.02 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
dd76eef 461.37 ms 540.55 ms 79.18 ms
e8603bb 377.51 ms 444.77 ms 67.26 ms
0a82a1e 321.02 ms 393.82 ms 72.80 ms
72eeb80 421.38 ms 520.81 ms 99.44 ms
2f8f173 323.31 ms 373.29 ms 49.97 ms
d10745a 318.86 ms 374.82 ms 55.96 ms
84bc635 395.57 ms 464.23 ms 68.66 ms
8932ece 309.56 ms 377.28 ms 67.72 ms
633cf2e 289.36 ms 340.38 ms 51.02 ms
4b943a1 348.17 ms 437.15 ms 88.98 ms

App size

Revision Plain With Sentry Diff
dd76eef 6.35 MiB 7.40 MiB 1.05 MiB
e8603bb 6.34 MiB 7.28 MiB 967.80 KiB
0a82a1e 6.15 MiB 7.11 MiB 981.82 KiB
72eeb80 6.35 MiB 7.42 MiB 1.06 MiB
2f8f173 5.94 MiB 6.95 MiB 1.01 MiB
d10745a 6.26 MiB 7.20 MiB 956.08 KiB
84bc635 6.34 MiB 7.28 MiB 968.41 KiB
8932ece 6.16 MiB 7.13 MiB 1000.89 KiB
633cf2e 5.94 MiB 6.92 MiB 1001.53 KiB
4b943a1 6.34 MiB 7.28 MiB 968.41 KiB

Previous results on branch: fix/sentry-asset-bundle-flickering

Startup times

Revision Plain With Sentry Diff
ba7f41b 449.35 ms 522.44 ms 73.09 ms
a24e107 460.25 ms 537.63 ms 77.38 ms

App size

Revision Plain With Sentry Diff
ba7f41b 6.46 MiB 7.48 MiB 1.03 MiB
a24e107 6.46 MiB 7.48 MiB 1.03 MiB

Copy link
Contributor

github-actions bot commented Jan 14, 2025

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1253.83 ms 1262.25 ms 8.42 ms
Size 8.42 MiB 9.89 MiB 1.47 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
90db9ff 1277.18 ms 1283.69 ms 6.51 ms
1a93825 1257.25 ms 1261.55 ms 4.30 ms
905bf99 1240.84 ms 1271.47 ms 30.63 ms
2d74010 1264.45 ms 1268.42 ms 3.97 ms
e5b744f 1250.82 ms 1284.46 ms 33.64 ms
3e9fb0e 1262.49 ms 1280.65 ms 18.16 ms
7045efc 1246.45 ms 1262.24 ms 15.80 ms
09eab47 1246.88 ms 1267.71 ms 20.83 ms
8609bd8 1267.16 ms 1291.39 ms 24.22 ms
33d0587 1262.16 ms 1270.50 ms 8.34 ms

App size

Revision Plain With Sentry Diff
90db9ff 8.10 MiB 9.08 MiB 1004.27 KiB
1a93825 8.28 MiB 9.34 MiB 1.05 MiB
905bf99 8.38 MiB 9.74 MiB 1.36 MiB
2d74010 8.32 MiB 9.38 MiB 1.05 MiB
e5b744f 8.09 MiB 9.07 MiB 1001.19 KiB
3e9fb0e 8.15 MiB 9.12 MiB 989.77 KiB
7045efc 8.38 MiB 9.78 MiB 1.40 MiB
09eab47 8.38 MiB 9.77 MiB 1.40 MiB
8609bd8 8.28 MiB 9.34 MiB 1.06 MiB
33d0587 8.29 MiB 9.38 MiB 1.09 MiB

Previous results on branch: fix/sentry-asset-bundle-flickering

Startup times

Revision Plain With Sentry Diff
ba7f41b 1244.04 ms 1259.22 ms 15.18 ms
a24e107 1251.55 ms 1269.35 ms 17.80 ms

App size

Revision Plain With Sentry Diff
ba7f41b 8.42 MiB 9.89 MiB 1.46 MiB
a24e107 8.42 MiB 9.89 MiB 1.46 MiB

@denrase denrase marked this pull request as ready for review January 15, 2025 09:38
@buenaflor
Copy link
Contributor

@denrase hmm I don't think we have a larger sample, I could try to check if we have customer data on that

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.

Using SentryAssetBundle() causes asset images to flash when CupertinoContextMenu() tapped
2 participants