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

fix: simplify dispatch queue wrapper access #3477

Merged
merged 7 commits into from
Jan 2, 2024

Conversation

armcknight
Copy link
Member

A possible solution to fix #3476. There are several places that we start a transaction with a tracer configuration that lacks a dispatch queue wrapper here:

- (id<SentrySpan>)startTransactionWithContext:(SentryTransactionContext *)transactionContext
bindToScope:(BOOL)bindToScope
customSamplingContext:(NSDictionary<NSString *, id> *)customSamplingContext
{
return [self startTransactionWithContext:transactionContext
bindToScope:bindToScope
customSamplingContext:customSamplingContext
configuration:[SentryTracerConfiguration defaultConfiguration]];

and here:

newSpan = [SentrySDK.currentHub
startTransactionWithContext:context
bindToScope:bindToScope
customSamplingContext:@{}
configuration:[SentryTracerConfiguration configurationWithBlock:^(
SentryTracerConfiguration *configuration) {
configuration.waitForChildren = YES;
}]];

In the spot where we do actually provide it here:

transaction = [SentrySDK.currentHub
startTransactionWithContext:context
bindToScope:bindToScope
customSamplingContext:@{}
configuration:[SentryTracerConfiguration configurationWithBlock:^(
SentryTracerConfiguration *config) {
config.idleTimeout = self.idleTimeout;
config.waitForChildren = YES;
config.dispatchQueueWrapper = self.dispatchQueueWrapper;
}]];

that reference is actually just set to the instance held by the dependency container here:

SentryDependencyContainer *dependencies = [SentryDependencyContainer sharedInstance];
SentryUIEventTrackerTransactionMode *mode = [[SentryUIEventTrackerTransactionMode alloc]
initWithDispatchQueueWrapper:dependencies.dispatchQueueWrapper

I know we have an ongoing debate about when and where to use DI vs singletons, but let's avoid having this mixture, and take time to completely refactor one dependency at a time to remove the reference completely from SentryDependencyContainer convert to DI in all codepaths.

Copy link

github-actions bot commented Dec 5, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1201.65 ms 1213.20 ms 11.55 ms
Size 21.58 KiB 418.43 KiB 396.85 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
98a8c16 1234.69 ms 1265.02 ms 30.33 ms
591a01b 1197.94 ms 1222.53 ms 24.59 ms
98713e6 1211.48 ms 1244.42 ms 32.94 ms
01a28a9 1237.24 ms 1253.24 ms 16.00 ms
dd0557f 1246.31 ms 1258.46 ms 12.15 ms
2a894d5 1211.02 ms 1236.72 ms 25.70 ms
f576153 1217.17 ms 1225.06 ms 7.89 ms
17afc4b 1228.94 ms 1251.10 ms 22.16 ms
1bf8571 1250.96 ms 1255.36 ms 4.40 ms
c40576a 1202.45 ms 1224.60 ms 22.15 ms

App size

Revision Plain With Sentry Diff
98a8c16 20.76 KiB 431.00 KiB 410.24 KiB
591a01b 22.84 KiB 401.67 KiB 378.83 KiB
98713e6 20.76 KiB 435.22 KiB 414.46 KiB
01a28a9 22.85 KiB 405.39 KiB 382.54 KiB
dd0557f 22.85 KiB 411.75 KiB 388.90 KiB
2a894d5 21.58 KiB 414.57 KiB 392.99 KiB
f576153 20.76 KiB 425.77 KiB 405.01 KiB
17afc4b 20.76 KiB 436.25 KiB 415.49 KiB
1bf8571 20.76 KiB 437.12 KiB 416.36 KiB
c40576a 22.85 KiB 407.63 KiB 384.78 KiB

Previous results on branch: armcknight/fix/3476-unscheduled-deadline-timer

Startup times

Revision Plain With Sentry Diff
cca9946 1207.15 ms 1217.80 ms 10.65 ms
123eec4 1223.96 ms 1235.18 ms 11.22 ms
aead7e8 1219.10 ms 1246.33 ms 27.22 ms
619f72b 1217.63 ms 1241.55 ms 23.92 ms

App size

Revision Plain With Sentry Diff
cca9946 21.58 KiB 418.43 KiB 396.85 KiB
123eec4 21.58 KiB 414.52 KiB 392.94 KiB
aead7e8 21.58 KiB 418.43 KiB 396.85 KiB
619f72b 21.58 KiB 418.21 KiB 396.62 KiB

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ea2a263) 89.214% compared to head (0158505) 89.227%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3477       +/-   ##
=============================================
+ Coverage   89.214%   89.227%   +0.013%     
=============================================
  Files          528       528               
  Lines        57538     57552       +14     
  Branches     20617     20626        +9     
=============================================
+ Hits         51332     51352       +20     
+ Misses        5295      5291        -4     
+ Partials       911       909        -2     
Files Coverage Δ
Sources/Sentry/SentryTracer.m 97.159% <100.000%> (+0.397%) ⬆️
...urces/Sentry/SentryUIEventTrackerTransactionMode.m 100.000% <ø> (ø)
Sources/Sentry/SentryUIEventTrackingIntegration.m 100.000% <100.000%> (ø)
...SentryProfilerTests/SentryProfilerSwiftTests.swift 97.769% <100.000%> (ø)
...iewController/SentryTimeToDisplayTrackerTest.swift 99.696% <100.000%> (ø)
...egrations/UIEvents/SentryUIEventTrackerTests.swift 100.000% <100.000%> (ø)
...ts/SentryTests/Transaction/SentryTracerTests.swift 98.858% <100.000%> (-0.008%) ⬇️

... and 12 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 ea2a263...0158505. Read the comment docs.

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.

Good catch, @armcknight. Thanks for fixing this.

Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
@armcknight armcknight changed the title fix: implify dispatch queue wrapper access fix: simplify dispatch queue wrapper access Dec 5, 2023
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, if you add a changelog entry @armcknight. You could also change the PR's title to reflect the user benefit instead of the detailed technical change.

@armcknight armcknight force-pushed the armcknight/fix/3476-unscheduled-deadline-timer branch from 3f5d3b1 to 588bf1d Compare January 2, 2024 19:17
@armcknight
Copy link
Member Author

The thread sanitizer is in an unrelated test case, and failed recently on main: https://github.com/getsentry/sentry-cocoa/actions/runs/7263319984/job/19788493775#step:7:54

@armcknight armcknight merged commit b9a9ffd into main Jan 2, 2024
69 of 70 checks passed
@armcknight armcknight deleted the armcknight/fix/3476-unscheduled-deadline-timer branch January 2, 2024 21:32
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.

TTFD timeout timer never scheduled
3 participants