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

SentryAssetBundle returns Future by default #1462

Merged
merged 4 commits into from
May 23, 2023

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented May 19, 2023

📜 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

@codecov
Copy link

codecov bot commented May 19, 2023

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
Copy link
Contributor

github-actions bot commented May 19, 2023

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
Copy link
Contributor

github-actions bot commented May 19, 2023

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
Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

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
Copy link
Member

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
Copy link
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.

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.

3 participants