Closed GLinnik21 closed 2 months ago
@naftaly, from your perspective, does it worth it?
@naftaly, from your perspective, does it worth it?
I see two distinct things here.
1- Splitting the disk info monitor out so it can be opt in/out by users of KSCrash. 2- Having a good reason to use the data in the first place.
For #1, I think your options enumerated above seem fine. Or maybe we can add a “privacy manifest” group of monitors. KSCrash isn’t a UI package so adding UI isn’t really an option. In any case, that’s up to the user of the SDK and I don’t think we want to get involved in all the tasks adding UI creates.
So, what I’d do. Add a monitor for disk related data. Add that monitor to a privacy manifest monitor group and document why it’s there. At that point we’ve done our due diligence and compiled with Apple rules, it’ll now be up to SDK users to make their apps comply.
I think it’s worth it.
I think we should go with option 2. More specifically, I think we should expose the moniotr API as part of KSCrash public API. This way we can allow users writing custom crash report enrichment plugins which I think good thing to have in general.
And in the context of this task, an additional product(SPM) / subspec(CocoaPods) can be created that will have its own privacy manifest and this extra monitor. And it will be up to a developer to include this library to the app to opt-in to the disk space monitoring.
I think implementing opt-out in this case might be really tricky as we don't want this API to be compiled into the app binary and not just "unused". So I would rather go with the opt-in way.
I know that such refactoruing might not be that easy, but I believe that it should make the lib API even more clean.
Maybe we should move this "public" monitor API to a "private" module, like KSCrashRecordingCore
, to not expose this API to a regular user? And this "PrivacyModule" would depend on KSCrashRecordingCore
.
Here is the basic idea of the current structure for the reference.
Having this in the "extended" API like KSCrashRecordingCore
target makes sense to me. Currently monitors are part of private API (except of Monitor Type). If we keep type-to-monitor-api mapping in the KSCrashRecording
while moving the implementations to KSCrashRecordingCore
, it should work. I like the idea.
We also have a "tricky" bootTime API.
Should we make a module for each privacy related API or just one?
Should there be separate KSCrashMonitorType
or just one KSCrashMonitorTypePrivacy
?
Also, should the KSCrashRecording
module "know" about any monitor that it doesn't explicitly depend on?
Should we make a module for each privacy related API or just one? Should there be separate
KSCrashMonitorType
or just oneKSCrashMonitorTypePrivacy
?
We have 2 now: disk space and boot time, right? Let's have 2 monitors in 2 separate modules. I don't think there's going to be too many of such privacy-sensitive monitors and we can easily maintain 2-3 extra modules like this.
Also, should the
KSCrashRecording
module "know" about any monitor that it doesn't explicitly depend on?
Ideally no. KSCrashRecording
should only know about the monitor API in KSCrashRecordingCore
. The existing monitors can remain in KSCrashRecording
, by the way, I don't see any reason to move them to core (only the monitor API). The tricky part is how to allow adding extra monitors (considering that installation is managed by KSCrashRecording
).
We might have a special case for supporting these 2 new monitors in the monitor types and log an error/warning if the implementation is not "registered" (e.g. in KSCrashRecording
we can have a functuion that registers such injections and a function in each new module that a developer needs to call before install
).
… we can have a functuion that registers such injections and a function in each new module that a developer needs to call before install).
Less related to this discussion, but maybe we can consider simplifying the startup for the user while whe’re at it. I’ve found it a bit confusing in what order things need to be called. I’d like to see a config object, and then we simply pass that to install.
// create a config
var config = CrashConfuration(<options…>)
/// … add config options …
// install it. That’s it.
Crash.shared.install(configuration: config)
In reference to PR #457, where we addressed the declaration requirement for
NSFileSystemSize
asNSPrivacyAccessedAPICategoryDiskSpace
, it falls under 7D9E.1 as we send this information off the device in crash reports. Apple mandates that this part of the crash report should be optional and requires user approval for the usage of disk space information.To comply with Apple's guidelines and avoid potential issues, we should consider implementing an opt-out mechanism for it. However, enforcing an opt-in UI for crash reporting might not be the best approach for our library users. Therefore, creating a module seems more appropriate.
Proposed Solutions
Automatic Injection:
+load
or__attribute__((constructor))
to automatically inject a "monitor" that supplements crash reports with disk size information.Public API for Injection:
Remove NSFileSystemSize Code:
NSFileSystemSize
code. It may not be crucial enough to justify the effort required to implement these systems.