-
-
Notifications
You must be signed in to change notification settings - Fork 316
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: Finish transaction for external view controllers #3440
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d8eb419 | 1221.91 ms | 1253.62 ms | 31.71 ms |
e324230 | 1254.92 ms | 1262.92 ms | 8.00 ms |
6e342ac | 1202.98 ms | 1228.74 ms | 25.76 ms |
596ccc1 | 1230.85 ms | 1244.24 ms | 13.39 ms |
4fc7fdf | 1236.69 ms | 1242.80 ms | 6.11 ms |
591a01b | 1242.69 ms | 1259.98 ms | 17.29 ms |
6943de0 | 1215.83 ms | 1253.22 ms | 37.39 ms |
fdfe96b | 1227.90 ms | 1242.56 ms | 14.66 ms |
dbc67d2 | 1239.49 ms | 1248.88 ms | 9.39 ms |
e79ead7 | 1233.39 ms | 1255.52 ms | 22.13 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d8eb419 | 22.85 KiB | 408.87 KiB | 386.02 KiB |
e324230 | 22.85 KiB | 408.88 KiB | 386.03 KiB |
6e342ac | 20.76 KiB | 436.66 KiB | 415.90 KiB |
596ccc1 | 22.84 KiB | 401.44 KiB | 378.60 KiB |
4fc7fdf | 20.76 KiB | 436.25 KiB | 415.49 KiB |
591a01b | 22.84 KiB | 401.67 KiB | 378.83 KiB |
6943de0 | 20.76 KiB | 393.33 KiB | 372.57 KiB |
fdfe96b | 20.76 KiB | 419.70 KiB | 398.95 KiB |
dbc67d2 | 20.76 KiB | 427.74 KiB | 406.98 KiB |
e79ead7 | 20.76 KiB | 426.11 KiB | 405.35 KiB |
Previous results on branch: fiz/external-lib-swizzling
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6e6e3e2 | 1241.96 ms | 1247.32 ms | 5.36 ms |
be7f054 | 1370.04 ms | 1371.65 ms | 1.61 ms |
feef17b | 1228.65 ms | 1253.61 ms | 24.97 ms |
91f07c2 | 1253.82 ms | 1261.12 ms | 7.30 ms |
cb30d90 | 1230.52 ms | 1247.55 ms | 17.03 ms |
af83051 | 1226.14 ms | 1245.22 ms | 19.08 ms |
83e2ce0 | 1221.55 ms | 1238.16 ms | 16.61 ms |
3ba22d7 | 1223.59 ms | 1241.36 ms | 17.77 ms |
b61f614 | 1248.20 ms | 1262.60 ms | 14.40 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6e6e3e2 | 22.85 KiB | 413.75 KiB | 390.91 KiB |
be7f054 | 22.85 KiB | 413.88 KiB | 391.03 KiB |
feef17b | 22.85 KiB | 414.00 KiB | 391.15 KiB |
91f07c2 | 22.85 KiB | 413.87 KiB | 391.02 KiB |
cb30d90 | 22.85 KiB | 413.81 KiB | 390.96 KiB |
af83051 | 22.85 KiB | 414.00 KiB | 391.15 KiB |
83e2ce0 | 22.85 KiB | 413.77 KiB | 390.92 KiB |
3ba22d7 | 22.85 KiB | 413.98 KiB | 391.13 KiB |
b61f614 | 22.85 KiB | 413.81 KiB | 390.96 KiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave the detailled reviews to @armcknight and @philipphofmann, but we shouldn't rush this
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3440 +/- ##
=============================================
- Coverage 89.019% 89.018% -0.001%
=============================================
Files 525 525
Lines 56590 56634 +44
Branches 20365 20383 +18
=============================================
+ Hits 50376 50415 +39
- Misses 5298 5302 +4
- Partials 916 917 +1
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks great. Thanks for fixing this. There are some issues to address before we can merge this @brustolin.
...ryTests/Integrations/Performance/UIViewController/SentryUIViewControllerSwizzlingTests.swift
Show resolved
Hide resolved
...ryTests/Integrations/Performance/UIViewController/SentryUIViewControllerSwizzlingTests.swift
Outdated
Show resolved
Hide resolved
...ryTests/Integrations/Performance/UIViewController/SentryUIViewControllerSwizzlingTests.swift
Show resolved
Hide resolved
…try-cocoa into fiz/external-lib-swizzling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some quick questions/suggestions!
|
||
init() { | ||
subClassFinder = TestSubClassFinder(dispatchQueue: dispatchQueue, objcRuntimeWrapper: objcRuntimeWrapper) | ||
binaryImageCache = SentryDependencyContainer.sharedInstance().binaryImageCache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of doing this? Why add another parameter to SentryUIViewControllerSwizzling where it was already using the singleton from SentryDependencyContainer, just to assign it to the same value again in these tests? It's not even accessed anywhere in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following @philipphofmann suggestion to use injection in the constructor instead of making direct use of SentryDependencyContainer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's following our decision on how to handle dependency injection. I'm a fan of specifying all dependencies via their constructor to improve testability. If you disagree, I'm happy to discuss this again, but not here in this PR 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i mean, we're not injecting anything, we just added it and aren't using it. yagni, do this if/when we actually need to inject a mock
//To finish the transaction we need to finish `initialDisplay` span | ||
//by calling `viewWillAppear` and reporting a new frame | ||
controller.viewWillAppear(false) | ||
Dynamic(SentryDependencyContainer.sharedInstance().framesTracker).reportNewFrame() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to cause SentryTimeToDisplayTracker.framesTrackerHasNewFrame
to be called, ending the span it's managing? If so, please mention it here, or whatever other side effect is desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not call SentryTimeToDisplayTracker.framesTrackerHasNewFrame
it calls SentryTimeToDisplayTracker.reportNewFrame
which cause SentryTimeToDisplayTracker to report a new frame to all its delegate. This desired side effect is described in the comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the reason I asked about SentryTimeToDisplayTracker.framesTrackerHasNewFrame
is because when I looked at SentryTimeToDisplayTracker.reportNewFrame
and clicked through to the delegate call, the only option that comes up is SentryTimeToDisplayTracker.framesTrackerHasNewFrame
:
So either we're both right or there's something else going on? Please let us know the mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, I misread the class name you wrote. reportNewFrame is a function of SentryFramesTracker
that will call framesTrackerHasNewFrame
from its delegates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, after you either include or ditch my suggestion to extract the logic of calling hasPrefix
to a function in #3452.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not merge the change declaring SentryTestUtils as a dynamic lib, as it breaks our ability to run some unit tests locally. Or let me know if that's not the case for anyone else, and I can look into why it's happening on my machine.
Add method isImageNameInApp to SentryInAppLogic, so both SentryInAppLogic and SentryBinaryImageCache use the same code. Co-authored-by: Andrew McKnight <[email protected]>
Solved already. Thanks for pointing this out.
...ryTests/Integrations/Performance/UIViewController/SentryUIViewControllerSwizzlingTests.swift
Show resolved
Hide resolved
…ntryUIViewControllerSwizzlingTests.swift
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Finish transaction for external view controllers ([#3440](https://github.com/getsentry/sentry-cocoa/pull/3440)) If none of the above apply, you can opt out of this check by adding |
Move Finish transaction for external view controllers (#3440) up to unreleased
Move Finish transaction for external view controllers (#3440) up to unreleased
📜 Description
View controllers from frameworks in
inAppIncludes
option are not being swizzled, and so, their transactions are not being closed, but we do start the transaction, because start happens with swizzling ofUIViewController
.💡 Motivation and Context
close #3434
💚 How did you test it?
Sample, Unit Test
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.🔮 Next steps