Skip to content

Fix image flickering when using SentryAssetBundle#2577

Merged
denrase merged 9 commits into
mainfrom
fix/sentry-asset-bundle-flickering
Jan 22, 2025
Merged

Fix image flickering when using SentryAssetBundle#2577
denrase merged 9 commits into
mainfrom
fix/sentry-asset-bundle-flickering

Conversation

@denrase

@denrase denrase commented Jan 14, 2025

Copy link
Copy Markdown
Collaborator

📜 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

@codecov

codecov Bot commented Jan 14, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

Project coverage is 92.51%. Comparing base (ed08c68) to head (e84ea97).
Report is 2 commits behind head on main.

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   89.00%   92.51%   +3.50%     
==========================================
  Files         262       91     -171     
  Lines        8968     2979    -5989     
==========================================
- Hits         7982     2756    -5226     
+ Misses        986      223     -763

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

@denrase

denrase commented Jan 14, 2025

Copy link
Copy Markdown
Collaborator Author

@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?

@github-actions

github-actions Bot commented Jan 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 440.16 ms 511.00 ms 70.84 ms
Size 6.46 MiB 7.48 MiB 1.03 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
62de927 313.81 ms 358.15 ms 44.34 ms
2d74010 400.42 ms 466.50 ms 66.08 ms
1d47eb7 282.26 ms 342.52 ms 60.26 ms
d0476e1 412.20 ms 492.62 ms 80.42 ms
d4120ac 373.14 ms 447.35 ms 74.21 ms
48c3cf1 491.55 ms 541.64 ms 50.09 ms
d7758e8 300.12 ms 349.88 ms 49.76 ms
6159a2f 461.44 ms 527.69 ms 66.26 ms
e4d5aa8 375.28 ms 443.27 ms 67.99 ms
25e9b59 307.00 ms 348.83 ms 41.83 ms

App size

Revision Plain With Sentry Diff
62de927 6.15 MiB 7.11 MiB 981.78 KiB
2d74010 6.33 MiB 7.26 MiB 943.41 KiB
1d47eb7 6.16 MiB 7.14 MiB 1010.29 KiB
d0476e1 6.35 MiB 7.40 MiB 1.05 MiB
d4120ac 6.27 MiB 7.20 MiB 960.42 KiB
48c3cf1 6.49 MiB 7.55 MiB 1.07 MiB
d7758e8 5.94 MiB 6.95 MiB 1.01 MiB
6159a2f 6.46 MiB 7.48 MiB 1.01 MiB
e4d5aa8 6.35 MiB 7.35 MiB 1017.84 KiB
25e9b59 5.94 MiB 6.95 MiB 1.01 MiB

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

Startup times

Revision Plain With Sentry Diff
e003cac 452.52 ms 492.86 ms 40.34 ms
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
e003cac 6.46 MiB 7.48 MiB 1.02 MiB
ba7f41b 6.46 MiB 7.48 MiB 1.03 MiB
a24e107 6.46 MiB 7.48 MiB 1.03 MiB

@github-actions

github-actions Bot commented Jan 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1239.54 ms 1250.35 ms 10.81 ms
Size 8.42 MiB 9.91 MiB 1.49 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
905bf99 1240.84 ms 1271.47 ms 30.63 ms
cf91c9d 1217.08 ms 1233.00 ms 15.92 ms
09eab47 1246.88 ms 1267.71 ms 20.83 ms
d8519f9 1208.57 ms 1229.80 ms 21.22 ms
c1bb00f 1265.14 ms 1290.85 ms 25.71 ms
0b204c0 1232.19 ms 1244.37 ms 12.18 ms
0e0581f 1250.13 ms 1278.96 ms 28.83 ms
d301b11 1260.61 ms 1272.06 ms 11.45 ms
8d64376 1260.92 ms 1289.32 ms 28.40 ms
4656f10 1247.04 ms 1276.16 ms 29.12 ms

App size

Revision Plain With Sentry Diff
905bf99 8.38 MiB 9.74 MiB 1.36 MiB
cf91c9d 8.33 MiB 9.40 MiB 1.07 MiB
09eab47 8.38 MiB 9.77 MiB 1.40 MiB
d8519f9 8.32 MiB 9.38 MiB 1.05 MiB
c1bb00f 8.09 MiB 9.07 MiB 1001.06 KiB
0b204c0 8.42 MiB 9.84 MiB 1.41 MiB
0e0581f 8.38 MiB 9.78 MiB 1.41 MiB
d301b11 8.10 MiB 9.07 MiB 1000.82 KiB
8d64376 8.16 MiB 9.17 MiB 1.01 MiB
4656f10 8.32 MiB 9.50 MiB 1.19 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
e003cac 1253.83 ms 1262.25 ms 8.42 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
e003cac 8.42 MiB 9.89 MiB 1.47 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

buenaflor commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

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

@buenaflor

buenaflor commented Jan 20, 2025

Copy link
Copy Markdown
Contributor

@denrase unfortunately havent found anything, kinda hard to check, is the issue because we cannot await the span.finish?

@denrase

denrase commented Jan 20, 2025

Copy link
Copy Markdown
Collaborator Author

@buenaflor Yeah, this is a behaviour change, but it's needed to resolve the issue.

@buenaflor

buenaflor commented Jan 20, 2025

Copy link
Copy Markdown
Contributor

hm I'm trying to find scenarios where this would be problematic, but on first glance it looks like it's alright

But hard to say, I assume you haven't encountered anything alarming during testing

buenaflor
buenaflor reviewed Jan 20, 2025
View reviewed changes
Comment thread flutter/lib/src/sentry_asset_bundle.dart
@denrase
denrase requested a review from buenaflor January 21, 2025 09:41
buenaflor
buenaflor reviewed Jan 22, 2025
View reviewed changes
Comment thread flutter/lib/src/sentry_asset_bundle.dart Outdated
Comment thread flutter/lib/src/sentry_asset_bundle.dart Outdated
@denrase
denrase requested a review from buenaflor January 22, 2025 13:16
buenaflor
buenaflor approved these changes Jan 22, 2025
View reviewed changes

@buenaflor buenaflor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@denrase
denrase merged commit 0965b4f into main Jan 22, 2025
@denrase
denrase deleted the fix/sentry-asset-bundle-flickering branch January 22, 2025 14:10
buenaflor added a commit that referenced this pull request Jan 22, 2025
@buenaflor
Fix changelog - moved #2577 to unreleased (#2608)
80734dc 
* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@buenaflor buenaflor buenaflor approved these changes
@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich
@stefanosiano stefanosiano Awaiting requested review from stefanosiano

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

2 participants

@denrase @buenaflor