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: Properly teardown frames tracker #3545

Merged
merged 4 commits into from
Jan 5, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jan 5, 2024

The teardown of the SentryFramesTracker didn't work properly in tests. The clearTestState method continuously initialized a new frames tracker cause it accessed the property dependency container. This is fixed now by calling SentryFramesTracker.stop directly in SentryDependencyContainer.reset which is also called in clearTestState. Furthermore, the SentryFramesTracker.stop now resets all frames and removes all observers.

I identified this problem when looking at the logs of failing tests at the main branch here: https://github.com/getsentry/sentry-cocoa/actions/runs/7419191432/job/20188316830. The logs showed continuous init calls to SentryFramesTracker and also unwanted tracking of frames during tests, which could lead to failing tests.

#skip-changelog

The teardown of the SentryFramesTracker didn't work properly in tests.
The clearTestState method continuously initialized a new frames tracker
cause it accessed the property dependency container. This is fixed now
by calling SentryFramesTracker.stop directly in
SentryDependencyContainer.reset which is also called in clearTestState.
Furthermore, the SentryFramesTracker.stop now resets all frames and
removes all observers.
@@ -26,9 +26,6 @@ class TestCleanup: NSObject {
setTestDefaultLogLevel()

#if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)
let framesTracker = SentryDependencyContainer.sharedInstance().framesTracker
Copy link
Member Author

Choose a reason for hiding this comment

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

SentryDependencyContainer.reset now stops the frames tracker, which this method also calls.

Copy link

github-actions bot commented Jan 5, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1223.82 ms 1235.20 ms 11.39 ms
Size 21.58 KiB 418.82 KiB 397.24 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
257c2a9 1231.45 ms 1252.12 ms 20.67 ms
60dd0f5 1239.94 ms 1252.54 ms 12.60 ms
ae9c51b 1247.33 ms 1254.12 ms 6.80 ms
f0283e8 1253.36 ms 1263.12 ms 9.76 ms
afd1a08 1207.78 ms 1223.44 ms 15.66 ms
a5c946b 1219.33 ms 1236.53 ms 17.20 ms
a6f8b18 1238.54 ms 1265.56 ms 27.02 ms
9fa25c2 1211.88 ms 1228.36 ms 16.48 ms
8b39743 1258.51 ms 1263.26 ms 4.75 ms
3ea21f5 1250.80 ms 1258.88 ms 8.08 ms

App size

Revision Plain With Sentry Diff
257c2a9 20.76 KiB 401.36 KiB 380.60 KiB
60dd0f5 20.76 KiB 393.36 KiB 372.60 KiB
ae9c51b 22.85 KiB 411.13 KiB 388.28 KiB
f0283e8 20.76 KiB 393.36 KiB 372.60 KiB
afd1a08 22.84 KiB 402.57 KiB 379.72 KiB
a5c946b 20.76 KiB 431.93 KiB 411.17 KiB
a6f8b18 20.76 KiB 431.87 KiB 411.11 KiB
9fa25c2 22.85 KiB 407.44 KiB 384.59 KiB
8b39743 22.85 KiB 413.98 KiB 391.13 KiB
3ea21f5 22.84 KiB 402.63 KiB 379.78 KiB

Previous results on branch: ref/frames-tracker-teardown

Startup times

Revision Plain With Sentry Diff
0b56927 1223.33 ms 1246.48 ms 23.15 ms
521051f 1200.55 ms 1211.14 ms 10.59 ms

App size

Revision Plain With Sentry Diff
0b56927 21.58 KiB 418.79 KiB 397.21 KiB
521051f 21.58 KiB 418.83 KiB 397.24 KiB

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (253f628) 89.226% compared to head (d16888b) 89.271%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3545       +/-   ##
=============================================
+ Coverage   89.226%   89.271%   +0.044%     
=============================================
  Files          528       529        +1     
  Lines        57709     57792       +83     
  Branches     20665     20693       +28     
=============================================
+ Hits         51492     51592      +100     
+ Misses        5298      5291        -7     
+ Partials       919       909       -10     
Files Coverage Δ
SentryTestUtils/ClearTestState.swift 97.872% <ø> (-0.128%) ⬇️
Sources/Sentry/SentryDependencyContainer.m 95.312% <100.000%> (+0.055%) ⬆️
Sources/Sentry/SentryFramesTracker.m 100.000% <100.000%> (ø)
...yTests/Helper/SentryDependencyContainerTests.swift 100.000% <100.000%> (ø)
...ance/FramesTracking/SentryFramesTrackerTests.swift 99.140% <100.000%> (+0.041%) ⬆️

... and 25 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 253f628...d16888b. Read the comment docs.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@philipphofmann philipphofmann merged commit 1734d1b into main Jan 5, 2024
69 of 70 checks passed
@philipphofmann philipphofmann deleted the ref/frames-tracker-teardown branch January 5, 2024 13:18
philipphofmann added a commit that referenced this pull request Jan 10, 2024
The teardown of the SentryFramesTracker didn't work properly in tests.
The clearTestState method continuously initialized a new frames tracker
cause it accessed the property dependency container. This is fixed now
by calling SentryFramesTracker.stop directly in
SentryDependencyContainer.reset which is also called in clearTestState.
Furthermore, the SentryFramesTracker.stop now resets all frames and
removes all observers.
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.

None yet

2 participants