ref: use preexisting SentryDependencyContainer.notificationCenterWrapper

[armcknight]

We already had a centralized notification center wrapper. No need to pass around references.

#skip-changelog

[codecov]

Codecov Report

Merging #3246 (6821e06) into main (c471221) will decrease coverage by 0.103%.
The diff coverage is 99.137%.

@@              Coverage Diff              @@
##              main     #3246       +/-   ##
=============================================
- Coverage   89.053%   88.951%   -0.103%     
=============================================
  Files          521       521               
  Lines        56045     55870      -175     
  Branches     20170     19857      -313     
=============================================
- Hits         49910     49697      -213     
- Misses        5220      5253       +33     
- Partials       915       920        +5     

Files	Coverage Δ
SentryTestUtils/Invocations.swift	84.848% <ø> (ø)
Sources/Sentry/SentryAppStateManager.m	100.000% <100.000%> (ø)
...s/Sentry/SentryAutoBreadcrumbTrackingIntegration.m	100.000% <100.000%> (ø)
...rces/Sentry/SentryAutoSessionTrackingIntegration.m	100.000% <100.000%> (ø)
Sources/Sentry/SentryDependencyContainer.m	95.121% <100.000%> (-0.020%)	⬇️
Sources/Sentry/SentrySessionTracker.m	100.000% <100.000%> (ø)
Sources/Sentry/SentrySystemEventBreadcrumbs.m	100.000% <100.000%> (ø)
...entryTests/Helper/SentryAppStateManagerTests.swift	100.000% <100.000%> (ø)
...SentryAutoBreadcrumbTrackingIntegrationTests.swift	97.701% <100.000%> (ø)
...Breadcrumbs/SentrySystemEventBreadcrumbsTest.swift	97.872% <100.000%> (-0.010%)	⬇️
... and 5 more

... and 68 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 c471221...6821e06. Read the comment docs.

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1261.60 ms	1274.94 ms	13.34 ms
Size	22.85 KiB	413.47 KiB	390.62 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
279841c	1237.29 ms	1254.12 ms	16.83 ms
c319795	1256.18 ms	1266.87 ms	10.69 ms
be51b56	1220.84 ms	1249.36 ms	28.52 ms
7ce3cf6	1217.98 ms	1246.41 ms	28.43 ms
d40512b	1231.12 ms	1244.54 ms	13.42 ms
89bc37d	1240.76 ms	1248.24 ms	7.48 ms
1437c68	1223.94 ms	1249.76 ms	25.82 ms
1bf8571	1215.31 ms	1232.48 ms	17.17 ms
ae9c51b	1244.85 ms	1264.33 ms	19.47 ms
034be1c	1222.67 ms	1236.22 ms	13.55 ms

App size

Revision	Plain	With Sentry	Diff
279841c	22.84 KiB	403.19 KiB	380.34 KiB
c319795	20.76 KiB	431.99 KiB	411.22 KiB
be51b56	20.76 KiB	432.20 KiB	411.44 KiB
7ce3cf6	22.85 KiB	407.63 KiB	384.78 KiB
d40512b	20.76 KiB	427.77 KiB	407.00 KiB
89bc37d	20.76 KiB	401.53 KiB	380.77 KiB
1437c68	22.85 KiB	410.96 KiB	388.11 KiB
1bf8571	20.76 KiB	437.12 KiB	416.36 KiB
ae9c51b	22.85 KiB	411.13 KiB	388.28 KiB
034be1c	20.76 KiB	436.66 KiB	415.90 KiB

Previous results on branch: armcknight/ref/centralize-SentryNSNotificationCenter-access

Startup times

Revision	Plain	With Sentry	Diff
507d10c	1211.39 ms	1228.70 ms	17.31 ms
393852c	1239.12 ms	1256.02 ms	16.90 ms
8464878	1209.73 ms	1225.22 ms	15.49 ms
d2f0340	1203.91 ms	1246.53 ms	42.62 ms
1c2519f	1243.71 ms	1258.10 ms	14.39 ms
7e2b8c3	1231.13 ms	1250.84 ms	19.71 ms

App size

Revision	Plain	With Sentry	Diff
507d10c	22.85 KiB	413.12 KiB	390.27 KiB
393852c	22.85 KiB	413.12 KiB	390.27 KiB
8464878	22.85 KiB	404.93 KiB	382.08 KiB
d2f0340	22.85 KiB	406.52 KiB	383.67 KiB
1c2519f	22.85 KiB	407.28 KiB	384.43 KiB
7e2b8c3	22.85 KiB	407.28 KiB	384.43 KiB

[armcknight]

The tests are failing because of a crash, and the reports I looked at involve both SentryFileManager and Invocations.

[getsantry]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you remove the label Waiting for: Community, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[philipphofmann]

m: That seems like a fix that could be in an extra PR, so the whole test suite immediately benefits from this improvement.

[philipphofmann]

m: I'm not a real fan of this change. I prefer passing in dependencies, so you know what a class requires to work correctly, and it increases testability. Now I have no clue when using this class that it needs the SentryNSNotificationCenterWrapper, and I need to modify the dependencyContainer if I want to use the TestNotificationCenterWrapper.

[armcknight]

FWIW I agree with you. If you recall some time ago this is why I actually wanted to move away from SentryDependencyContainer and do full dependency injection: #2942 (comment)

After that I figured we should move things to go to the singleton, and after ironing out the inconsistencies, if we decided we wanted DI later, it'd be straigthforward to change (even if it would be some effort, of course) because the list of things to convert to DI will all be listed in SentryDependencyContainer.

Regardless of which approach we take, I'd prefer we be consistent. If I never know what to expect in a given class, or if there is a mixture of approaches in a given codepath, it makes it harder to reason about.

[armcknight]

Rereading that thread, I'm a little confused why you say the client can't use DI here: #2942 (comment)

[philipphofmann]

DI is a bit tricky for classes our users interact with. We can't ask our users for all the dependencies the client requires when initializing it cause that would be a nightmare for usability, and most classes required are internal. For internal classes, though, we can pass in all dependencies via the constructor. That way, we would be consistent.

[armcknight]

OK, I see what you're saying. We could take a page out of Apple's book and provide a method defaultFileManager, defaultHttpTransport etc for those dependencies, as well as a defaultClient. It shouldn't be a usability nightmare for everyone because most people should not be initializing their own instances of SentryClient, they should just use SentrySDK.start...

[philipphofmann]

But there are plenty of dependencies for the SentryClient; see

sentry-cocoa/Sources/Sentry/SentryClient.m

Lines 132 to 139 in e0f077c

	- (instancetype)initWithOptions:(SentryOptions *)options
	transportAdapter:(SentryTransportAdapter *)transportAdapter
	fileManager:(SentryFileManager *)fileManager
	deleteOldEnvelopeItems:(BOOL)deleteOldEnvelopeItems
	threadInspector:(SentryThreadInspector *)threadInspector
	random:(id<SentryRandom>)random
	locale:(NSLocale *)locale
	timezone:(NSTimeZone *)timezone

Passing in all dependencies is not user-friendly. I don't want to make FileManager, HttpTransport etc, public. I don't want users to know that these classes exist, and making them public reduces the flexibility.

I would like to stick to passing in all dependencies for internal classes and using the dependency container for public classes, to provide dependencies for our users.

[armcknight]

OK, I think I actually had it backwards before. A strategy of DI for internal interfaces and singletons for public interface dependencies sounds good, and easy to keep consistent 👍🏻

[philipphofmann]

I opened a PR to our decision log so this info doesn't get lost: #3396.