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: Add app start to first finished transaction #2252

Merged
merged 9 commits into from
Oct 5, 2022

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Oct 4, 2022

📜 Description

The SDK added the app start measurement and spans to the first finished auto-generated UIViewController transaction. The first finished UIViewController transaction is not necessarily the transaction of the first loaded screen. As auto-generated transactions for UIViewControllers wait for all children to finish, it may kick off, for example, a web request that lasts for a couple of seconds in the background. In the meantime, the user navigates to another screen, which finishes loading before the web request. The SDK added the app start data to the wrong transaction in such an edge case. This is fixed now by adding the app start data to the first started transaction.

💡 Motivation and Context

Fixes GH-2248

💚 How did you test it?

Unit tests and on a real device with the steps described in GH-2248.

LoremIpsumViewController without the app start data.
Screen Shot 2022-10-04 at 15 15 05

iOS_Swift.ViewController with the app start data
Screen Shot 2022-10-04 at 15 15 24

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

The SDK added the app start measurement and spans to the first
finished auto-generated UIViewController transaction. The first finished
UIViewController transaction is not necessarily the transaction of the
first loaded screen. As auto-generated transactions for
UIViewControllers wait for all children to finish, it may kick off, for
example, a web request that lasts for a couple of seconds in the
background. In the meantime, the user navigates to another screen,
which finishes loading before the web request. The SDK added the
app start data to the wrong transaction in such an edge case. This is
fixed now by adding the app start data to the first started transaction.

Fixes GH-2248
@@ -58,6 +58,7 @@ @implementation SentryTracer {
BOOL _waitForChildren;
SentryTraceContext *_traceContext;
SentryProfilesSamplerDecision *_profilesSamplerDecision;
SentryAppStartMeasurement *appStartMeasurement;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you did here 😁👏🏻...

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

github-actions bot commented Oct 4, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1251.08 ms 1252.94 ms 1.86 ms
Size 20.51 KiB 333.58 KiB 313.07 KiB

Baseline results on branch: master

Startup times

Revision Plain With Sentry Diff
e2f1150 1220.39 ms 1249.94 ms 29.55 ms
ee127f4 1203.49 ms 1231.28 ms 27.79 ms
4a66f00 1259.84 ms 1281.66 ms 21.82 ms
864c39a 1191.14 ms 1233.38 ms 42.24 ms
b172a8b 1257.68 ms 1272.38 ms 14.70 ms
4a66f00 1224.73 ms 1241.14 ms 16.41 ms
864c39a 1239.45 ms 1256.76 ms 17.31 ms
347a8e9 1243.50 ms 1265.90 ms 22.40 ms
e958899 1230.40 ms 1248.31 ms 17.91 ms
a5ca27b 1231.31 ms 1252.56 ms 21.25 ms

App size

Revision Plain With Sentry Diff
e2f1150 20.51 KiB 333.10 KiB 312.59 KiB
ee127f4 20.51 KiB 333.10 KiB 312.59 KiB
4a66f00 20.51 KiB 331.79 KiB 311.28 KiB
864c39a 20.51 KiB 335.57 KiB 315.06 KiB
b172a8b 20.51 KiB 331.79 KiB 311.28 KiB
4a66f00 20.51 KiB 331.79 KiB 311.28 KiB
864c39a 20.51 KiB 335.57 KiB 315.06 KiB
347a8e9 20.51 KiB 333.16 KiB 312.65 KiB
e958899 20.51 KiB 331.92 KiB 311.41 KiB
a5ca27b 20.51 KiB 331.81 KiB 311.31 KiB

Previous results on branch: fix/app-start-on-first-finished-transaction

Startup times

Revision Plain With Sentry Diff
d8176b6 1223.71 ms 1258.48 ms 34.77 ms
b00b932 1209.52 ms 1242.17 ms 32.65 ms
667289c 1217.00 ms 1230.14 ms 13.14 ms
1963019 1259.18 ms 1267.12 ms 7.94 ms
5bcd104 1246.86 ms 1263.78 ms 16.92 ms
e85c3b9 1239.02 ms 1246.06 ms 7.04 ms

App size

Revision Plain With Sentry Diff
d8176b6 20.51 KiB 333.09 KiB 312.58 KiB
b00b932 20.51 KiB 333.08 KiB 312.57 KiB
667289c 20.51 KiB 333.08 KiB 312.57 KiB
1963019 20.51 KiB 333.09 KiB 312.58 KiB
5bcd104 20.51 KiB 333.09 KiB 312.58 KiB
e85c3b9 20.51 KiB 333.09 KiB 312.58 KiB

CHANGELOG.md Outdated Show resolved Hide resolved
@philipphofmann philipphofmann merged commit 6dc0bd1 into master Oct 5, 2022
@philipphofmann philipphofmann deleted the fix/app-start-on-first-finished-transaction branch October 5, 2022 08:38
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.

Attaching app start data to first finished transaction
3 participants