Skip to content

Conversation

@brustolin
Copy link
Contributor

@brustolin brustolin commented Feb 27, 2023

📜 Description

A new Span is created to track time for initial display in automatic UIViewController performance tracking.

A new API was added to SentrySDK where users may report a full display. This is meant to be called when the view controller content is fully loaded and ready to be used.

💡 Motivation and Context

Adding more information during view controller life cycle

💚 How did you test it?

Unit tests

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

First pass; I don't understand the purpose of the middleware thingy. I think it would be best to discuss this over a call. There are also plenty of conflicts to resolve, so I didn't look at the tracer yet.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 28, 2023

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Time to initial and full display ([#2724](https://github.com/getsentry/sentry-cocoa/pull/2724))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against ca27faa

@github-actions
Copy link
Contributor

github-actions bot commented Feb 28, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1205.20 ms 1227.48 ms 22.28 ms
Size 20.76 KiB 430.52 KiB 409.76 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d40512b 1231.12 ms 1244.54 ms 13.42 ms
45d3ca5 1238.53 ms 1263.09 ms 24.55 ms
8f397a7 1230.10 ms 1253.88 ms 23.77 ms
cd3bfeb 1226.61 ms 1226.96 ms 0.35 ms
c9724f9 1199.38 ms 1229.54 ms 30.16 ms
4259afd 1222.12 ms 1249.74 ms 27.62 ms
bd2afa6 1241.37 ms 1246.20 ms 4.83 ms
6ad07ae 1240.76 ms 1242.98 ms 2.22 ms
28333b6 1247.29 ms 1262.51 ms 15.22 ms
d253cdf 1231.61 ms 1259.52 ms 27.91 ms

App size

Revision Plain With Sentry Diff
d40512b 20.76 KiB 427.77 KiB 407.00 KiB
45d3ca5 20.76 KiB 427.54 KiB 406.78 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
cd3bfeb 20.76 KiB 425.77 KiB 405.01 KiB
c9724f9 20.76 KiB 427.66 KiB 406.90 KiB
4259afd 20.76 KiB 419.70 KiB 398.94 KiB
bd2afa6 20.76 KiB 420.55 KiB 399.79 KiB
6ad07ae 20.76 KiB 424.69 KiB 403.93 KiB
28333b6 20.76 KiB 424.69 KiB 403.93 KiB
d253cdf 20.76 KiB 427.66 KiB 406.90 KiB

Previous results on branch: feat/ttid-ttfd

Startup times

Revision Plain With Sentry Diff
3bbaf5d 1229.64 ms 1256.92 ms 27.28 ms
b90ad7d 1239.96 ms 1246.66 ms 6.70 ms
f3085b3 1221.69 ms 1232.10 ms 10.41 ms
ea900f3 1219.27 ms 1247.30 ms 28.03 ms
0af4650 1231.40 ms 1250.98 ms 19.58 ms
54097aa 1252.80 ms 1259.18 ms 6.38 ms
007489f 1206.54 ms 1224.50 ms 17.96 ms
626a0cf 1235.69 ms 1247.00 ms 11.31 ms
b9c94c1 1235.56 ms 1250.86 ms 15.30 ms
ac724bb 1238.30 ms 1262.42 ms 24.12 ms

App size

Revision Plain With Sentry Diff
3bbaf5d 20.76 KiB 428.19 KiB 407.42 KiB
b90ad7d 20.76 KiB 429.70 KiB 408.94 KiB
f3085b3 20.76 KiB 430.33 KiB 409.57 KiB
ea900f3 20.76 KiB 428.20 KiB 407.43 KiB
0af4650 20.76 KiB 428.91 KiB 408.14 KiB
54097aa 20.76 KiB 431.97 KiB 411.21 KiB
007489f 20.76 KiB 430.24 KiB 409.48 KiB
626a0cf 20.76 KiB 429.24 KiB 408.48 KiB
b9c94c1 20.76 KiB 428.71 KiB 407.94 KiB
ac724bb 20.76 KiB 431.96 KiB 411.20 KiB

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Another pass, lets have a chat about my proposal for changing and renaming the protocols #2724 (comment) via a call.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

There's a lot of good stuff in here, nice work fleshing out the implementation. I left a bunch of suggestions you can hopefully take up easily to align some names to improve searchability for later maintainers.

I do have some questions around how types are being used. And I don't think we should move forward with the tracer extension concept. That should wait on discussion in the related issue and be saved for a later PR.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Another pass, I still have a couple of files to go, but I will do that tomorrow. This is turning into something great 👍 😃

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Almost done. It could be that we have a retain cycle, but it could also be that I'm wrong.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, ship it 😃 🚀

#endif

#if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)
func testReportFullyDisplayed() {
Copy link
Member

Choose a reason for hiding this comment

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

💯

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.

7 participants