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

ref: centralize frames tracker access in SentryDependencyContainer #3119

Merged
merged 4 commits into from
Jun 26, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion SentryTestUtils/ClearTestState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class TestCleanup: NSObject {
setTestDefaultLogLevel()

#if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)
let framesTracker = SentryFramesTracker.sharedInstance()
let framesTracker = SentryDependencyContainer.sharedInstance().framesTracker
framesTracker.stop()
framesTracker.resetFrames()

Expand Down
4 changes: 2 additions & 2 deletions Sources/Sentry/PrivateSentrySDKOnly.m
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,12 @@ + (void)setFramesTrackingMeasurementHybridSDKMode:(BOOL)framesTrackingMeasuremen

+ (BOOL)isFramesTrackingRunning
{
return [SentryFramesTracker sharedInstance].isRunning;
return SentryDependencyContainer.sharedInstance.framesTracker.isRunning;
}

+ (SentryScreenFrames *)currentScreenFrames
{
return [SentryFramesTracker sharedInstance].currentFrames;
return SentryDependencyContainer.sharedInstance.framesTracker.currentFrames;
}

+ (NSArray<NSData *> *)captureScreenshots
Expand Down
19 changes: 17 additions & 2 deletions Sources/Sentry/SentryDependencyContainer.m
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#import "SentryANRTracker.h"
#import "SentryDefaultCurrentDateProvider.h"
#import "SentryDispatchQueueWrapper.h"
#import "SentryDisplayLinkWrapper.h"
#import "SentryFramesTracker.h"
#import "SentryNSProcessInfoWrapper.h"
#import "SentryUIApplication.h"
#import <SentryAppStateManager.h>
Expand Down Expand Up @@ -168,7 +170,20 @@ - (SentryUIApplication *)application
}
return _application;
}
#endif

- (SentryFramesTracker *)framesTracker
{
if (_framesTracker == nil) {
@synchronized(sentryDependencyContainerLock) {
if (_framesTracker == nil) {
_framesTracker = [[SentryFramesTracker alloc]
initWithDisplayLinkWrapper:[[SentryDisplayLinkWrapper alloc] init]];
}
}
}
return _framesTracker;
}
#endif // SENTRY_HAS_UIKIT

- (SentrySwizzleWrapper *)swizzleWrapper
{
Expand Down Expand Up @@ -242,6 +257,6 @@ - (SentryMXManager *)metricKitManager
return _metricKitManager;
}

#endif
#endif // SENTRY_HAS_METRIC_KIT

@end
12 changes: 0 additions & 12 deletions Sources/Sentry/SentryFramesTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -50,18 +50,6 @@ @implementation SentryFramesTracker {
unsigned int _frozenFrames;
}

+ (instancetype)sharedInstance
{
static SentryFramesTracker *sharedInstance = nil;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
sharedInstance =
[[self alloc] initWithDisplayLinkWrapper:[[SentryDisplayLinkWrapper alloc] init]];
});
return sharedInstance;
}

/** Internal constructor for testing */
- (instancetype)initWithDisplayLinkWrapper:(SentryDisplayLinkWrapper *)displayLinkWrapper
{
if (self = [super init]) {
Expand Down
3 changes: 2 additions & 1 deletion Sources/Sentry/SentryFramesTrackingIntegration.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#import "SentryFramesTrackingIntegration.h"
#import "PrivateSentrySDKOnly.h"
#import "SentryDependencyContainer.h"
#import "SentryFramesTracker.h"
#import "SentryLog.h"

Expand All @@ -24,7 +25,7 @@ - (BOOL)installWithOptions:(SentryOptions *)options
return NO;
}

self.tracker = [SentryFramesTracker sharedInstance];
self.tracker = SentryDependencyContainer.sharedInstance.framesTracker;
[self.tracker start];

return YES;
Expand Down
27 changes: 6 additions & 21 deletions Sources/Sentry/SentryProfiler.mm
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,6 @@
SentrySystemWrapper *_gCurrentSystemWrapper;
SentryDispatchFactory *_gDispatchFactory;
SentryNSTimerWrapper *_gTimeoutTimerWrapper;
# if SENTRY_HAS_UIKIT
SentryFramesTracker *_gCurrentFramesTracker;
# endif // SENTRY_HAS_UIKIT

NSString *
profilerTruncationReasonName(SentryProfilerTruncationReason reason)
Expand Down Expand Up @@ -258,17 +255,18 @@
auto metrics = serializedMetrics;

# if SENTRY_HAS_UIKIT
const auto framesTracker = SentryDependencyContainer.sharedInstance.framesTracker;
const auto mutableMetrics =
[NSMutableDictionary<NSString *, id> dictionaryWithDictionary:metrics];
const auto slowFrames = sliceGPUData(_gCurrentFramesTracker.currentFrames.slowFrameTimestamps,
const auto slowFrames = sliceGPUData(framesTracker.currentFrames.slowFrameTimestamps,
transaction, /*useMostRecentRecording */ NO);
if (slowFrames.count > 0) {
mutableMetrics[@"slow_frame_renders"] =
@ { @"unit" : @"nanosecond", @"values" : slowFrames };
}

const auto frozenFrames
= sliceGPUData(_gCurrentFramesTracker.currentFrames.frozenFrameTimestamps, transaction,
= sliceGPUData(framesTracker.currentFrames.frozenFrameTimestamps, transaction,
/*useMostRecentRecording */ NO);
if (frozenFrames.count > 0) {
mutableMetrics[@"frozen_frame_renders"] =
Expand All @@ -277,7 +275,7 @@

if (slowFrames.count > 0 || frozenFrames.count > 0) {
const auto frameRates
= sliceGPUData(_gCurrentFramesTracker.currentFrames.frameRateTimestamps, transaction,
= sliceGPUData(framesTracker.currentFrames.frameRateTimestamps, transaction,
/*useMostRecentRecording */ YES);
if (frameRates.count > 0) {
mutableMetrics[@"screen_frame_rates"] = @ { @"unit" : @"hz", @"values" : frameRates };
Expand Down Expand Up @@ -482,7 +480,7 @@ + (void)startWithHub:(SentryHub *)hub
}

# if SENTRY_HAS_UIKIT
[_gCurrentFramesTracker resetProfilingTimestamps];
[SentryDependencyContainer.sharedInstance.framesTracker resetProfilingTimestamps];
# endif // SENTRY_HAS_UIKIT

[_gCurrentProfiler start];
Expand Down Expand Up @@ -566,14 +564,6 @@ + (void)useTimeoutTimerWrapper:(SentryNSTimerWrapper *)timerWrapper
_gTimeoutTimerWrapper = timerWrapper;
}

# if SENTRY_HAS_UIKIT
+ (void)useFramesTracker:(SentryFramesTracker *)framesTracker
{
std::lock_guard<std::mutex> l(_gProfilerLock);
_gCurrentFramesTracker = framesTracker;
}
# endif // SENTRY_HAS_UIKIT

# pragma mark - Private

+ (NSDictionary<NSString *, id> *)serializeForTransaction:(SentryTransaction *)transaction
Expand Down Expand Up @@ -635,7 +625,7 @@ + (void)stopProfilerForReason:(SentryProfilerTruncationReason)reason
[_gCurrentProfiler stop];
_gCurrentProfiler->_truncationReason = reason;
# if SENTRY_HAS_UIKIT
[_gCurrentFramesTracker resetProfilingTimestamps];
[SentryDependencyContainer.sharedInstance.framesTracker resetProfilingTimestamps];
# endif // SENTRY_HAS_UIKIT
}

Expand All @@ -650,11 +640,6 @@ - (void)startMetricProfiler
if (_gDispatchFactory == nil) {
_gDispatchFactory = [[SentryDispatchFactory alloc] init];
}
# if SENTRY_HAS_UIKIT
if (_gCurrentFramesTracker == nil) {
_gCurrentFramesTracker = SentryFramesTracker.sharedInstance;
}
# endif // SENTRY_HAS_UIKIT
_metricProfiler =
[[SentryMetricProfiler alloc] initWithProcessInfoWrapper:_gCurrentProcessInfoWrapper
systemWrapper:_gCurrentSystemWrapper
Expand Down
8 changes: 3 additions & 5 deletions Sources/Sentry/SentryTimeToDisplayTracker.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#import "SentryTimeToDisplayTracker.h"
#import "SentryCurrentDate.h"
#import "SentryDependencyContainer.h"
#import "SentryFramesTracker.h"
#import "SentryMeasurementValue.h"
#import "SentrySpan.h"
Expand All @@ -24,18 +25,15 @@ @implementation SentryTimeToDisplayTracker {
BOOL _waitForFullDisplay;
BOOL _isReadyToDisplay;
BOOL _fullyDisplayedReported;
SentryFramesTracker *_frameTracker;
NSString *_controllerName;
}

- (instancetype)initForController:(UIViewController *)controller
framesTracker:(SentryFramesTracker *)framestracker
waitForFullDisplay:(BOOL)waitForFullDisplay
{
if (self = [super init]) {
_controllerName = [SwiftDescriptor getObjectClassName:controller];
_waitForFullDisplay = waitForFullDisplay;
_frameTracker = framestracker;

_isReadyToDisplay = NO;
_fullyDisplayedReported = NO;
Expand Down Expand Up @@ -64,7 +62,7 @@ - (void)startForTracer:(SentryTracer *)tracer

self.initialDisplaySpan.startTimestamp = tracer.startTimestamp;

[_frameTracker addListener:self];
[SentryDependencyContainer.sharedInstance.framesTracker addListener:self];
[tracer setFinishCallback:^(
SentryTracer *_tracer) { [self trimTTFDIdNecessaryForTracer:_tracer]; }];
}
Expand Down Expand Up @@ -106,7 +104,7 @@ - (void)framesTrackerHasNewFrame
[self addTimeToDisplayMeasurement:self.initialDisplaySpan name:@"time_to_initial_display"];

[self.initialDisplaySpan finish];
[_frameTracker removeListener:self];
[SentryDependencyContainer.sharedInstance.framesTracker removeListener:self];
}
if (_waitForFullDisplay && _fullyDisplayedReported && self.fullDisplaySpan.isFinished == NO) {
self.fullDisplaySpan.timestamp = finishTime;
Expand Down
4 changes: 2 additions & 2 deletions Sources/Sentry/SentryTracer.m
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ - (instancetype)initWithTransactionContext:(SentryTransactionContext *)transacti
#if SENTRY_HAS_UIKIT
// Store current amount of frames at the beginning to be able to calculate the amount of
// frames at the end of the transaction.
SentryFramesTracker *framesTracker = [SentryFramesTracker sharedInstance];
SentryFramesTracker *framesTracker = SentryDependencyContainer.sharedInstance.framesTracker;
if (framesTracker.isRunning) {
SentryScreenFrames *currentFrames = framesTracker.currentFrames;
initTotalFrames = currentFrames.total;
Expand Down Expand Up @@ -737,7 +737,7 @@ - (void)addMeasurements:(SentryTransaction *)transaction

#if SENTRY_HAS_UIKIT
// Frames
SentryFramesTracker *framesTracker = [SentryFramesTracker sharedInstance];
SentryFramesTracker *framesTracker = SentryDependencyContainer.sharedInstance.framesTracker;
if (framesTracker.isRunning && !_startTimeChanged) {

SentryScreenFrames *currentFrames = framesTracker.currentFrames;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ - (void)createTimeToDisplay:(UIViewController *)controller

SentryTimeToDisplayTracker *ttdTracker =
[[SentryTimeToDisplayTracker alloc] initForController:controller
framesTracker:SentryFramesTracker.sharedInstance
waitForFullDisplay:self.enableWaitForFullDisplay];

objc_setAssociatedObject(controller, &SENTRY_UI_PERFORMANCE_TRACKER_TTD_TRACKER, ttdTracker,
Expand Down
19 changes: 15 additions & 4 deletions Sources/Sentry/include/SentryDependencyContainer.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,22 @@
#import "SentryFileManager.h"
#import "SentryRandom.h"

@class SentryAppStateManager, SentryCrashWrapper, SentryThreadWrapper, SentrySwizzleWrapper,
SentryDispatchQueueWrapper, SentryDebugImageProvider, SentryANRTracker,
SentryNSNotificationCenterWrapper, SentryMXManager, SentryNSProcessInfoWrapper;
@class SentryANRTracker;
@class SentryAppStateManager;
@class SentryCrashWrapper;
@class SentryDebugImageProvider;
@class SentryDispatchQueueWrapper;
@class SentryFramesTracker;
@class SentryMXManager;
@class SentryNSNotificationCenterWrapper;
@class SentryNSProcessInfoWrapper;
@class SentrySwizzleWrapper;
@class SentryThreadWrapper;

#if SENTRY_HAS_UIKIT
@class SentryScreenshot, SentryUIApplication, SentryViewHierarchy;
@class SentryScreenshot;
@class SentryUIApplication;
@class SentryViewHierarchy;
#endif

NS_ASSUME_NONNULL_BEGIN
Expand Down Expand Up @@ -35,6 +45,7 @@ SENTRY_NO_INIT
@property (nonatomic, strong) SentryNSProcessInfoWrapper *processInfoWrapper;

#if SENTRY_HAS_UIKIT
@property (nonatomic, strong) SentryFramesTracker *framesTracker;
@property (nonatomic, strong) SentryScreenshot *screenshot;
@property (nonatomic, strong) SentryViewHierarchy *viewHierarchy;
@property (nonatomic, strong) SentryUIApplication *application;
Expand Down
3 changes: 1 addition & 2 deletions Sources/Sentry/include/SentryFramesTracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@ NS_ASSUME_NONNULL_BEGIN
* Tracks total, frozen and slow frames for iOS, tvOS, and Mac Catalyst.
*/
@interface SentryFramesTracker : NSObject
SENTRY_NO_INIT

+ (instancetype)sharedInstance;
- (instancetype)initWithDisplayLinkWrapper:(SentryDisplayLinkWrapper *)displayLinkWrapper;

@property (nonatomic, assign, readonly) SentryScreenFrames *currentFrames;
@property (nonatomic, assign, readonly) BOOL isRunning;
Expand Down
6 changes: 3 additions & 3 deletions Sources/Sentry/include/SentryTimeToDisplayTracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
#if SENTRY_HAS_UIKIT
# import <UIKit/UIKit.h>

@class SentrySpan, SentryTracer, SentryFramesTracker;
@class SentrySpan;
@class SentryTracer;

NS_ASSUME_NONNULL_BEGIN

Expand All @@ -25,7 +26,6 @@ SENTRY_NO_INIT
@property (nonatomic, readonly) BOOL waitForFullDisplay;

- (instancetype)initForController:(UIViewController *)controller
framesTracker:(SentryFramesTracker *)framestracker
waitForFullDisplay:(BOOL)waitForFullDisplay;

- (void)startForTracer:(SentryTracer *)tracer;
Expand All @@ -38,4 +38,4 @@ SENTRY_NO_INIT

NS_ASSUME_NONNULL_END

#endif
#endif // SENTRY_HAS_UIKIT
2 changes: 1 addition & 1 deletion Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class SentryProfilerSwiftTests: XCTestCase {
systemWrapper.overrides.memoryFootprintBytes = mockMemoryFootprint

#if !os(macOS)
SentryProfiler.useFramesTracker(framesTracker)
SentryDependencyContainer.sharedInstance().framesTracker = framesTracker
framesTracker.start()
displayLinkWrapper.call()
#endif
Expand Down
4 changes: 0 additions & 4 deletions Tests/SentryTests/Helper/SentryProfiler+SwiftTest.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@ SentryProfiler ()
+ (void)useTimeoutTimerWrapper:(SentryNSTimerWrapper *)timerWrapper
NS_SWIFT_NAME(useTimeoutTimerWrapper(_:));

# if SENTRY_HAS_UIKIT
+ (void)useFramesTracker:(SentryFramesTracker *)framesTracker NS_SWIFT_NAME(useFramesTracker(_:));
# endif // SENTRY_HAS_UIKIT

@end

#endif // SENTRY_TARGET_PROFILING_SUPPORTED
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class SentryFramesTrackingIntegrationTests: XCTestCase {
func testUninstall() {
sut.install(with: fixture.options)

SentryFramesTracker.sharedInstance().setDisplayLinkWrapper(fixture.displayLink)
SentryDependencyContainer.sharedInstance().framesTracker.setDisplayLinkWrapper(fixture.displayLink)

sut.uninstall()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,17 @@ class SentryTimeToDisplayTrackerTest: XCTestCase {

init() {
framesTracker = SentryFramesTracker(displayLinkWrapper: displayLinkWrapper)
SentryDependencyContainer.sharedInstance().framesTracker = framesTracker
print("Fixture.init.SentryDependencyContainer.sharedInstance().framesTracker = \(SentryDependencyContainer.sharedInstance().framesTracker)")
framesTracker.start()
}

func getSut(for controller: UIViewController, waitForFullDisplay: Bool) -> SentryTimeToDisplayTracker {
return SentryTimeToDisplayTracker(for: controller, framesTracker: framesTracker, waitForFullDisplay: waitForFullDisplay)
return SentryTimeToDisplayTracker(for: controller, waitForFullDisplay: waitForFullDisplay)
}
}

private let fixture = Fixture()
private lazy var fixture = Fixture()
Comment on lines -27 to +28
Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out that with a constant instance property, it was initialized as part of a pre-test process, and then when the tests ran, the dependency container shared instance was constructed again and had a difference instance of the frames tracker, so the tests would fail. Making it lazy defers initialization until the test process itself is running.


override func setUp() {
super.setUp()
Expand All @@ -44,6 +46,7 @@ class SentryTimeToDisplayTrackerTest: XCTestCase {

sut.start(for: tracer)
XCTAssertEqual(tracer.children.count, 1)
print("testReportInitialDisplay_notWaitingFullDisplay.SentryDependencyContainer.sharedInstance().framesTracker = \(SentryDependencyContainer.sharedInstance().framesTracker)")
XCTAssertEqual(Dynamic(fixture.framesTracker).listeners.count, 1)

let ttidSpan = try XCTUnwrap(tracer.children.first, "Expected a TTID span")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,7 @@ class SentryUIViewControllerPerformanceTrackerTests: XCTestCase {
}

private func reportFrame() {
Dynamic(SentryFramesTracker.sharedInstance()).displayLinkCallback()
Dynamic(SentryDependencyContainer.sharedInstance().framesTracker).displayLinkCallback()
}
}
#endif
Loading