Skip to content

SentryAssetBundle returns Future by default#1462

Merged
marandaneto merged 4 commits into
mainfrom
chore/improve-async-assetloading
May 23, 2023
Merged

SentryAssetBundle returns Future by default#1462
marandaneto merged 4 commits into
mainfrom
chore/improve-async-assetloading

Conversation

@marandaneto

@marandaneto marandaneto commented May 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

📜 Description

Calls were made async and it was await to check its length.
Now they return the Future by default and its only await when it should be.

💡 Motivation and Context

Relates to #1457

💚 How did you test it?

📝 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

@marandaneto marandaneto mentioned this pull request May 19, 2023
Closed
@codecov

codecov Bot commented May 19, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (6aab859) 90.20% compared to head (f6d9794) 90.22%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1462      +/-   ##
==========================================
+ Coverage   90.20%   90.22%   +0.01%     
==========================================
  Files         181      181              
  Lines        5788     5798      +10     
==========================================
+ Hits         5221     5231      +10     
  Misses        567      567
Impacted Files Coverage Δ
flutter/lib/src/sentry_asset_bundle.dart 94.53% <100.00%> (+0.46%) ⬆️

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

@github-actions

github-actions Bot commented May 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 356.90 ms 444.38 ms 87.48 ms
Size 6.15 MiB 7.13 MiB 999.97 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
bf4aed7 311.24 ms 365.66 ms 54.42 ms
0be962b 325.54 ms 382.83 ms 57.29 ms
8fa3934 340.64 ms 407.92 ms 67.28 ms
e66e71e 296.84 ms 345.43 ms 48.59 ms
ef2f368 350.06 ms 429.44 ms 79.38 ms
62dde43 339.21 ms 423.06 ms 83.85 ms
a094100 388.02 ms 459.50 ms 71.48 ms
b2cbbc8 347.80 ms 395.31 ms 47.51 ms
a61674e 331.35 ms 391.06 ms 59.71 ms
0a82a1e 321.02 ms 393.82 ms 72.80 ms

App size

Revision Plain With Sentry Diff
bf4aed7 6.06 MiB 7.03 MiB 997.04 KiB
0be962b 6.06 MiB 7.03 MiB 990.29 KiB
8fa3934 6.06 MiB 7.09 MiB 1.03 MiB
e66e71e 6.06 MiB 7.09 MiB 1.03 MiB
ef2f368 5.94 MiB 6.89 MiB 975.81 KiB
62dde43 5.94 MiB 6.96 MiB 1.02 MiB
a094100 5.94 MiB 6.96 MiB 1.02 MiB
b2cbbc8 6.06 MiB 7.03 MiB 995.45 KiB
a61674e 6.06 MiB 7.03 MiB 990.29 KiB
0a82a1e 6.15 MiB 7.11 MiB 981.82 KiB

Previous results on branch: chore/improve-async-assetloading

Startup times

Revision Plain With Sentry Diff
8e9aca3 297.49 ms 348.57 ms 51.08 ms
7ee5687 333.66 ms 401.14 ms 67.48 ms

App size

Revision Plain With Sentry Diff
8e9aca3 6.15 MiB 7.11 MiB 982.35 KiB
7ee5687 6.15 MiB 7.13 MiB 999.97 KiB

@github-actions

github-actions Bot commented May 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1239.49 ms 1245.92 ms 6.43 ms
Size 8.29 MiB 9.36 MiB 1.07 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b49bf00 1248.00 ms 1260.35 ms 12.35 ms
a1a1545 1270.85 ms 1289.82 ms 18.96 ms
ef2f368 1259.12 ms 1277.04 ms 17.92 ms
fdac48a 1281.92 ms 1300.22 ms 18.31 ms
d7758e8 1271.69 ms 1288.08 ms 16.39 ms
0be962b 1264.10 ms 1281.16 ms 17.06 ms
89ea268 1252.33 ms 1253.58 ms 1.26 ms
3334ac1 1259.22 ms 1275.40 ms 16.17 ms
1131914 1277.20 ms 1300.20 ms 23.00 ms
ad69abc 1259.00 ms 1261.60 ms 2.60 ms

App size

Revision Plain With Sentry Diff
b49bf00 8.10 MiB 9.08 MiB 1004.36 KiB
a1a1545 8.16 MiB 9.17 MiB 1.01 MiB
ef2f368 8.15 MiB 9.10 MiB 965.24 KiB
fdac48a 8.10 MiB 9.08 MiB 1004.37 KiB
d7758e8 8.15 MiB 9.12 MiB 989.76 KiB
0be962b 8.10 MiB 9.16 MiB 1.07 MiB
89ea268 8.09 MiB 9.16 MiB 1.06 MiB
3334ac1 8.10 MiB 9.17 MiB 1.08 MiB
1131914 8.16 MiB 9.17 MiB 1.01 MiB
ad69abc 8.10 MiB 9.08 MiB 1004.37 KiB

Previous results on branch: chore/improve-async-assetloading

Startup times

Revision Plain With Sentry Diff
8e9aca3 1241.94 ms 1248.43 ms 6.49 ms
7ee5687 1258.53 ms 1267.33 ms 8.80 ms

App size

Revision Plain With Sentry Diff
8e9aca3 8.29 MiB 9.36 MiB 1.07 MiB
7ee5687 8.29 MiB 9.36 MiB 1.07 MiB

@marandaneto marandaneto changed the title Improve async loading of Asset Bundle SentryAssetBundle returns Future by default May 23, 2023
@marandaneto
marandaneto marked this pull request as ready for review May 23, 2023 08:00
krystofwoldrich
krystofwoldrich approved these changes May 23, 2023
View reviewed changes

@krystofwoldrich krystofwoldrich 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.

LGTM 🚀

@marandaneto
marandaneto merged commit 754c630 into main May 23, 2023
@marandaneto
marandaneto deleted the chore/improve-async-assetloading branch May 23, 2023 12:01
@stefanosiano

stefanosiano commented May 23, 2023

Copy link
Copy Markdown
Contributor

I'm not sure i get the change done here.
I mean, we return a future instead of executing the call right away (async). But then that future is going to be called anyway, so what does it exactly change?
Also, the reason is to fix the user complain, but it seems like it didn't.
What am i missing? 😅

@marandaneto

marandaneto commented May 24, 2023

Copy link
Copy Markdown
Contributor Author

I'm not sure i get the change done here. I mean, we return a future instead of executing the call right away (async). But then that future is going to be called anyway, so what does it exactly change? Also, the reason is to fix the user complain, but it seems like it didn't. What am i missing? 😅

This is more semantically correct since it's an async method that has to await on something else.
If it returns a Future, the Future is added to the event loop only and executed whenever it should be executed (the user has the chance to await if needed).
Executing right away, you are taking the decision on the user's behalf.

That does not fix the issue that's why it's Relates to instead of Closes to, the original issues remain open since I can reproduce the issue even without the SentryAssetBundle.

@marandaneto marandaneto mentioned this pull request May 24, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stefanosiano stefanosiano Awaiting requested review from stefanosiano
1 more reviewer
@krystofwoldrich krystofwoldrich krystofwoldrich approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@marandaneto @stefanosiano @krystofwoldrich