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(ttfd): unfinished ttfd should match ttid duration #2347

Merged
merged 11 commits into from
Oct 15, 2024

Conversation

buenaflor
Copy link
Contributor

@buenaflor buenaflor commented Oct 11, 2024

💡 Motivation and Context

Set unfinished ttid, ttfd to cancelled

This might happen if the user navigates to another screen while ttfd is not finished

💚 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

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.20%. Comparing base (4d763a5) to head (579b1e1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2347      +/-   ##
==========================================
+ Coverage   84.80%   85.20%   +0.39%     
==========================================
  Files         253       79     -174     
  Lines        9058     2791    -6267     
==========================================
- Hits         7682     2378    -5304     
+ Misses       1376      413     -963     

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

@stefanosiano
Copy link
Member

not sure about this
we already specify the deadline_exceeded in the docs
Also, the same thing is done in other SDKs. The idea was that the user can immediately understand something is wrong with their instrumentation, which is more difficult to realize with the canceled status
Is there a reason/demand for this change?

Copy link
Contributor

github-actions bot commented Oct 11, 2024

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1254.88 ms 1279.22 ms 24.35 ms
Size 8.38 MiB 9.75 MiB 1.37 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
014c3ea 1298.73 ms 1351.24 ms 52.51 ms
c3e6c82 1256.93 ms 1276.17 ms 19.24 ms
af2d175 1280.37 ms 1282.24 ms 1.88 ms
3f3ef0b 1223.73 ms 1237.67 ms 13.94 ms
fe4aa56 1248.82 ms 1261.35 ms 12.53 ms
d5696bf 1232.96 ms 1254.50 ms 21.54 ms
891efac 1233.78 ms 1248.31 ms 14.53 ms
fe6dcac 1234.19 ms 1260.71 ms 26.52 ms
e8603bb 1240.85 ms 1254.79 ms 13.94 ms
ae02632 1286.77 ms 1300.37 ms 13.60 ms

App size

Revision Plain With Sentry Diff
014c3ea 8.33 MiB 9.39 MiB 1.06 MiB
c3e6c82 8.32 MiB 9.38 MiB 1.06 MiB
af2d175 8.15 MiB 9.12 MiB 986.22 KiB
3f3ef0b 8.32 MiB 9.38 MiB 1.05 MiB
fe4aa56 8.10 MiB 9.08 MiB 1004.36 KiB
d5696bf 8.38 MiB 9.73 MiB 1.35 MiB
891efac 8.28 MiB 9.34 MiB 1.06 MiB
fe6dcac 8.38 MiB 9.74 MiB 1.36 MiB
e8603bb 8.33 MiB 9.40 MiB 1.07 MiB
ae02632 8.16 MiB 9.15 MiB 1020.68 KiB

Previous results on branch: fix/ttid-ttfd-cancelled-navigatio

Startup times

Revision Plain With Sentry Diff
4149552 1236.90 ms 1266.00 ms 29.10 ms
78301f7 1240.86 ms 1267.11 ms 26.25 ms
f3c3971 1241.08 ms 1266.06 ms 24.98 ms

App size

Revision Plain With Sentry Diff
4149552 8.38 MiB 9.75 MiB 1.37 MiB
78301f7 8.38 MiB 9.75 MiB 1.37 MiB
f3c3971 8.38 MiB 9.75 MiB 1.37 MiB

@buenaflor
Copy link
Contributor Author

buenaflor commented Oct 11, 2024

@stefanosiano

Also, the same thing is done in other SDKs. The idea was that the user can immediately understand something is wrong with their instrumentation, which is more difficult to realize with the canceled status

it still finishes with deadline exceeded if it actually times out after 30 seconds but it's also possible that a user navigates away after 2 seconds. we can keep it as deadline_exceeded to keep in line with other sdks if they do the same in that scenario, I don't have a strong opinion about it

the rest of the changes still make sense imo: setting ttfd span duration to ttid span duration if ttid duration is available and additional test

@stefanosiano
Copy link
Member

it still finishes with deadline exceeded if it actually times out after 30 seconds but it's also possible that a user navigates away after 2 seconds. we can keep it as deadline_exceeded to keep in line with other sdks if they do the same in that scenario, I don't have a strong opinion about it

Yeah, the others keep it as deadline_exceeded even when the transaction (navigation) is finished before a frame is drawn

the rest of the changes still make sense imo: setting ttfd span duration to ttid span duration if ttid duration is available and additional test

👍

Copy link
Contributor

github-actions bot commented Oct 11, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 474.66 ms 537.30 ms 62.64 ms
Size 6.49 MiB 7.57 MiB 1.08 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
48adddf 353.96 ms 431.10 ms 77.14 ms
3e33891 313.60 ms 379.51 ms 65.91 ms
5e8d2b3 342.17 ms 375.83 ms 33.66 ms
64e4616 349.82 ms 436.96 ms 87.14 ms
3334ac1 303.98 ms 366.65 ms 62.67 ms
0210372 444.12 ms 489.78 ms 45.65 ms
754cdbe 325.08 ms 390.53 ms 65.45 ms
03e4c9b 410.34 ms 493.20 ms 82.86 ms
bf8d36c 505.00 ms 563.18 ms 58.18 ms
9d7e862 426.35 ms 510.88 ms 84.53 ms

App size

Revision Plain With Sentry Diff
48adddf 6.27 MiB 7.20 MiB 959.09 KiB
3e33891 6.16 MiB 7.14 MiB 1007.46 KiB
5e8d2b3 6.15 MiB 7.13 MiB 1000.11 KiB
64e4616 6.27 MiB 7.20 MiB 956.52 KiB
3334ac1 6.06 MiB 7.03 MiB 993.54 KiB
0210372 6.52 MiB 7.61 MiB 1.09 MiB
754cdbe 6.16 MiB 7.14 MiB 1003.78 KiB
03e4c9b 6.35 MiB 7.42 MiB 1.07 MiB
bf8d36c 6.49 MiB 7.56 MiB 1.07 MiB
9d7e862 6.33 MiB 7.26 MiB 943.41 KiB

Previous results on branch: fix/ttid-ttfd-cancelled-navigatio

Startup times

Revision Plain With Sentry Diff
78301f7 517.41 ms 599.19 ms 81.78 ms
f3c3971 515.48 ms 573.86 ms 58.38 ms
4149552 458.27 ms 510.24 ms 51.97 ms

App size

Revision Plain With Sentry Diff
78301f7 6.49 MiB 7.57 MiB 1.08 MiB
f3c3971 6.49 MiB 7.57 MiB 1.08 MiB
4149552 6.49 MiB 7.57 MiB 1.08 MiB

@buenaflor buenaflor changed the title fix(ttifd & ttfd): unfinished ttid or ttfd spans should be set to status cancelled fix(ttfd): unfinished ttfd should match ttid duration Oct 11, 2024
@buenaflor buenaflor merged commit 6ec7b50 into main Oct 15, 2024
51 checks passed
@buenaflor buenaflor deleted the fix/ttid-ttfd-cancelled-navigatio branch October 15, 2024 00:54
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