Skip to content

fix(ttfd): unfinished ttfd should match ttid duration#2347

Merged
buenaflor merged 11 commits into
mainfrom
fix/ttid-ttfd-cancelled-navigatio
Oct 15, 2024
Merged

fix(ttfd): unfinished ttfd should match ttid duration#2347
buenaflor merged 11 commits into
mainfrom
fix/ttid-ttfd-cancelled-navigatio

Conversation

@buenaflor

@buenaflor buenaflor commented Oct 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

💡 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

@codecov

codecov Bot commented Oct 11, 2024 •
edited
Loading

Copy link
Copy Markdown

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

stefanosiano commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

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?

@github-actions

github-actions Bot commented Oct 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

buenaflor commented Oct 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

stefanosiano commented Oct 11, 2024

Copy link
Copy Markdown
Contributor

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

👍

@github-actions

github-actions Bot commented Oct 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Reviewers

@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich
@martinhaintz martinhaintz Awaiting requested review from martinhaintz
1 more reviewer
@stefanosiano stefanosiano stefanosiano 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.

2 participants

@buenaflor @stefanosiano