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

App start: Create transaction when no SentryNavigatorObserver is present #2017

Merged
merged 45 commits into from
May 10, 2024

Conversation

buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Apr 29, 2024

📜 Description

Builds upon #2009

When SentryNavigatorObserver is not present, it will not create a transaction for the app start spans to attach to - so we need to create it manually

💡 Motivation and Context

Mobile starfish, improve app start instrumentation

💚 How did you test it?

Existing tests from #2009

📝 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
Contributor

github-actions bot commented Apr 29, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 402.24 ms 478.31 ms 76.07 ms
Size 6.33 MiB 7.30 MiB 992.08 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
abcdba3 354.68 ms 399.04 ms 44.36 ms
683fd34 336.53 ms 418.10 ms 81.57 ms
afa6e2a 349.73 ms 428.48 ms 78.75 ms
873fb42 352.10 ms 397.43 ms 45.33 ms
f770c4c 385.88 ms 449.86 ms 63.98 ms
464b4d0 320.71 ms 380.02 ms 59.31 ms
69670c9 325.59 ms 385.82 ms 60.22 ms
08a7b4f 346.47 ms 403.29 ms 56.82 ms
333903e 332.76 ms 406.86 ms 74.10 ms
e2d89fc 323.84 ms 376.23 ms 52.39 ms

App size

Revision Plain With Sentry Diff
abcdba3 5.94 MiB 6.95 MiB 1.01 MiB
683fd34 6.27 MiB 7.20 MiB 960.43 KiB
afa6e2a 6.27 MiB 7.20 MiB 955.69 KiB
873fb42 5.94 MiB 6.96 MiB 1.02 MiB
f770c4c 6.33 MiB 7.26 MiB 950.37 KiB
464b4d0 6.06 MiB 7.03 MiB 990.27 KiB
69670c9 6.06 MiB 7.09 MiB 1.03 MiB
08a7b4f 5.94 MiB 6.95 MiB 1.01 MiB
333903e 6.06 MiB 7.10 MiB 1.04 MiB
e2d89fc 6.06 MiB 7.03 MiB 989.37 KiB

Previous results on branch: feat/app-start-improve-2

Startup times

Revision Plain With Sentry Diff
8b9c0c0 380.41 ms 462.63 ms 82.22 ms
04724d5 433.62 ms 500.70 ms 67.08 ms
f7573f1 349.55 ms 411.79 ms 62.23 ms
58f2585 431.80 ms 500.70 ms 68.90 ms
e294533 355.73 ms 421.98 ms 66.25 ms
7001b46 351.55 ms 408.36 ms 56.81 ms
8984d63 357.78 ms 430.02 ms 72.24 ms
26e5834 450.93 ms 526.94 ms 76.01 ms
5fff0be 404.65 ms 476.16 ms 71.51 ms

App size

Revision Plain With Sentry Diff
8b9c0c0 6.33 MiB 7.30 MiB 987.83 KiB
04724d5 6.33 MiB 7.30 MiB 990.95 KiB
f7573f1 6.33 MiB 7.30 MiB 990.94 KiB
58f2585 6.33 MiB 7.30 MiB 990.98 KiB
e294533 6.33 MiB 7.30 MiB 991.27 KiB
7001b46 6.33 MiB 7.30 MiB 992.04 KiB
8984d63 6.33 MiB 7.30 MiB 991.35 KiB
26e5834 6.33 MiB 7.30 MiB 990.95 KiB
5fff0be 6.33 MiB 7.30 MiB 990.97 KiB

Copy link
Contributor

github-actions bot commented Apr 29, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1245.53 ms 1269.61 ms 24.08 ms
Size 8.32 MiB 9.51 MiB 1.19 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
a1a1545 1270.85 ms 1289.82 ms 18.96 ms
4b943a1 1250.23 ms 1262.28 ms 12.05 ms
d189e01 1219.94 ms 1228.20 ms 8.27 ms
bf4aed7 1274.63 ms 1286.48 ms 11.85 ms
6572f8d 1242.16 ms 1246.63 ms 4.47 ms
1d47eb7 1212.57 ms 1222.00 ms 9.43 ms
24b6e60 1250.69 ms 1268.63 ms 17.94 ms
bd37365 1236.22 ms 1243.27 ms 7.04 ms
ae02632 1286.77 ms 1300.37 ms 13.60 ms
c978477 1228.43 ms 1249.17 ms 20.74 ms

App size

Revision Plain With Sentry Diff
a1a1545 8.16 MiB 9.17 MiB 1.01 MiB
4b943a1 8.33 MiB 9.40 MiB 1.07 MiB
d189e01 8.29 MiB 9.38 MiB 1.09 MiB
bf4aed7 8.10 MiB 9.17 MiB 1.08 MiB
6572f8d 8.29 MiB 9.36 MiB 1.07 MiB
1d47eb7 8.29 MiB 9.39 MiB 1.09 MiB
24b6e60 8.32 MiB 9.38 MiB 1.06 MiB
bd37365 8.28 MiB 9.34 MiB 1.06 MiB
ae02632 8.16 MiB 9.15 MiB 1020.68 KiB
c978477 8.32 MiB 9.50 MiB 1.18 MiB

Previous results on branch: feat/app-start-improve-2

Startup times

Revision Plain With Sentry Diff
58f2585 1243.20 ms 1259.61 ms 16.41 ms
f7573f1 1251.57 ms 1276.22 ms 24.65 ms
26e5834 1236.00 ms 1264.20 ms 28.20 ms
e294533 1257.85 ms 1280.23 ms 22.37 ms
5fff0be 1250.41 ms 1271.00 ms 20.59 ms
8984d63 1233.57 ms 1258.60 ms 25.04 ms
7001b46 1252.49 ms 1275.23 ms 22.74 ms
04724d5 1225.65 ms 1244.87 ms 19.23 ms
8b9c0c0 1251.80 ms 1272.17 ms 20.38 ms

App size

Revision Plain With Sentry Diff
58f2585 8.32 MiB 9.43 MiB 1.11 MiB
f7573f1 8.32 MiB 9.37 MiB 1.05 MiB
26e5834 8.32 MiB 9.37 MiB 1.05 MiB
e294533 8.32 MiB 9.50 MiB 1.19 MiB
5fff0be 8.32 MiB 9.37 MiB 1.05 MiB
8984d63 8.32 MiB 9.50 MiB 1.19 MiB
7001b46 8.32 MiB 9.51 MiB 1.19 MiB
04724d5 8.32 MiB 9.37 MiB 1.05 MiB
8b9c0c0 8.32 MiB 9.43 MiB 1.10 MiB

@buenaflor buenaflor marked this pull request as ready for review May 3, 2024 17:38
@buenaflor
Copy link
Contributor Author

@stefanosiano this is now ready for review pls

Copy link
Member

@stefanosiano stefanosiano left a comment

Choose a reason for hiding this comment

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

looks good!

// This is a workaround since there is no api that tells us if
// the navigator observer exists or not. The currentRouteName is always
// set during a didPush triggered by the navigator observer.
if (SentryNavigatorObserver.currentRouteName == null) {
Copy link
Member

Choose a reason for hiding this comment

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

we could even have an additional static flag isAttached that is set to true when the class is instantiated, and use it together with this check, but not sure if that would actually help some case.
Your call here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm I think it would be good to have it additionally, I'll add it 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think naming it something like isCreated is more accurate since you can also instantiate it earlier and attach it much later

@buenaflor buenaflor merged commit 40e30e1 into main May 10, 2024
44 checks passed
@buenaflor buenaflor deleted the feat/app-start-improve-2 branch May 10, 2024 10:30
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.

2 participants