refactor: Add factory for SentryClient to dependency container #5315
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5315 +/- ##
=============================================
- Coverage 92.905% 9.173% -83.733%
=============================================
Files 688 362 -326
Lines 86392 26534 -59858
Branches 31229 121 -31108
=============================================
- Hits 80263 2434 -77829
- Misses 6028 24100 +18072
+ Partials 101 0 -101
... and 675 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I'm looking for feedback here on what you think of the approach of using a file manager factory where possible (e.g. in the ANR integration in the diff).
I think that's OK. We have a couple of locations in the code simply just pulling the file manager from the dependency container. Now you need to pass in the Options, which you can easily get via SentrySDK.options but at least then you know where they come from.
| NSError *error; | ||
| SentryFileManager *manager = | ||
| [[SentryFileManager alloc] initWithOptions:options | ||
| dispatchQueueWrapper:self.dispatchQueueWrapper | ||
| error:&error]; | ||
| if (manager == nil) { | ||
| SENTRY_LOG_FATAL(@"Could not create file manager - %@", error); | ||
| } | ||
| return manager; |
There was a problem hiding this comment.
h: The way the synchronization currently works in the SentryFileManager, we must ensure only having one SentryFileManager instance per DSN, as you already pointed out in the PR description. Otherwise, we end up with race conditions when accessing the files.
I think your idea of keeping weak references using the DSN with the key and the file manager as a value could work.
| SentryClient *newClient = | ||
| [SentryDependencyContainer.sharedInstance getClientWithOptions:options]; |
There was a problem hiding this comment.
m: I'm unsure if that solves anything, because any user can also manually init a client and we must support that use case. Using the SentryDependencyContainer internally increases complexity, and I don't see the benefit. What drove you adding this?
There was a problem hiding this comment.
we must support that use case
is this an actual SDK requirement, or just due to the fact that we don't want to change the current public API in our implementation to avoid a major rev?
There was a problem hiding this comment.
On the topic of what we're solving for here, I would be interested to know as well. AFAIUI this PR would help solve part of the file-backed data for watchdog events. Why do we need multiple instances of SentryFileManager for that?
There was a problem hiding this comment.
is this an actual SDK requirement, or just due to the fact that we don't want to change the current public API in our implementation to avoid a major rev?
It's part of the unified API, which we deprecated some time ago, because we're merging the hub and the scope. Once we do the next major we can revisit if we want to keep the client public. There are some use cases for initializing your own client, which I can't recall from the top of my head. Most users on Cocoa interact with the static API.
There was a problem hiding this comment.
To summarize what we are trying to fix:
- Our current Dependency Injection + Factory Pattern implementation is quite messy because of opportunistic refactoring.
- Some instances are managed by integration instances
- Some are part of the dependency injection container
- Some are created using factories
- Some instances in the static container are bounds to global options, but are used with local options (which can be different).
- For local instances we fetch the dependencies from the container, then initialize it locally. This is duplicated in the code base, instead of delegating it to a factory with access to the dependency container (e.g. here and here.
For reference, if instances are properly managed in a dependency container the entire integration can be delegate to testable and mockable factories.
This technical documentation of Swinject might be helpful in seeing the advantages:
https://github.com/Swinject/Swinject/tree/master/Documentation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 56ec5d0 | 1194.39 ms | 1236.94 ms | 42.55 ms |
| 302ee8b | 1194.02 ms | 1223.34 ms | 29.32 ms |
| 9f0d9e0 | 1212.49 ms | 1220.27 ms | 7.78 ms |
| 542727d | 1227.96 ms | 1246.88 ms | 18.92 ms |
| bd05478 | 1233.82 ms | 1255.56 ms | 21.74 ms |
| c78683b | 1255.78 ms | 1261.94 ms | 6.16 ms |
| f1b97be | 1212.69 ms | 1226.12 ms | 13.43 ms |
| 02a972c | 1222.73 ms | 1235.20 ms | 12.47 ms |
| 2d35ccb | 1220.40 ms | 1239.16 ms | 18.77 ms |
| 7192d9e | 1221.86 ms | 1248.28 ms | 26.42 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 56ec5d0 | 20.76 KiB | 414.44 KiB | 393.68 KiB |
| 302ee8b | 20.76 KiB | 419.62 KiB | 398.87 KiB |
| 9f0d9e0 | 21.58 KiB | 424.28 KiB | 402.70 KiB |
| 542727d | 21.58 KiB | 571.85 KiB | 550.26 KiB |
| bd05478 | 20.76 KiB | 432.33 KiB | 411.57 KiB |
| c78683b | 20.76 KiB | 435.24 KiB | 414.47 KiB |
| f1b97be | 21.58 KiB | 681.73 KiB | 660.15 KiB |
| 02a972c | 22.85 KiB | 413.42 KiB | 390.57 KiB |
| 2d35ccb | 21.58 KiB | 705.96 KiB | 684.38 KiB |
| 7192d9e | 20.76 KiB | 431.71 KiB | 410.95 KiB |
|
Linking #5242 (comment) as it is related. |
SentrySDK.options. While this is set during bootstrapping of the SDK, it can lead to inconsistencies if other parts rely on different options (especially relevant in isolated tests).Closes #5296
@philipphofmann @armcknight I'm looking for feedback here on what you think of the approach of using a file manager factory where possible (e.g. in the ANR integration in the diff).
#skip-changelog