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

feat: Add data use in privacy manifests #3259

Merged
merged 3 commits into from
Aug 31, 2023
Merged

feat: Add data use in privacy manifests #3259

merged 3 commits into from
Aug 31, 2023

Conversation

brustolin
Copy link
Contributor

📜 Description

Added "data use" in privacy manifests.

💡 Motivation and Context

Apple requires this information from Apps and Frameworks

@github-actions
Copy link

github-actions bot commented Aug 29, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 31a222a

@brustolin
Copy link
Contributor Author

brustolin commented Aug 29, 2023

Im wondering whether it make sense to ship this manifest with the SDK. Each feature may be disabled by the user and therefore it should be the developer responsibility to add in its manifest with the features they use.

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #3259 (1449c50) into main (a423161) will decrease coverage by 0.066%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3259       +/-   ##
=============================================
- Coverage   89.235%   89.169%   -0.066%     
=============================================
  Files          503       503               
  Lines        54263     54265        +2     
  Branches     19480     19481        +1     
=============================================
- Hits         48422     48388       -34     
- Misses        4983      5013       +30     
- Partials       858       864        +6     

see 16 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 a423161...1449c50. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Aug 29, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1220.04 ms 1228.08 ms 8.04 ms
Size 22.85 KiB 406.81 KiB 383.97 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
01a28a9 1237.24 ms 1253.24 ms 16.00 ms
368187c 1220.00 ms 1247.12 ms 27.12 ms
ab6fee4 1227.63 ms 1239.80 ms 12.17 ms
a5c946b 1219.33 ms 1236.53 ms 17.20 ms
c2ec420 1256.27 ms 1269.24 ms 12.97 ms
23e2db5 1240.96 ms 1253.86 ms 12.90 ms
257c2a9 1239.52 ms 1251.08 ms 11.56 ms
11ccc16 1206.45 ms 1225.60 ms 19.15 ms
7cd187e 1239.39 ms 1258.02 ms 18.63 ms
228a909 1248.92 ms 1267.42 ms 18.50 ms

App size

Revision Plain With Sentry Diff
01a28a9 22.85 KiB 405.39 KiB 382.54 KiB
368187c 20.76 KiB 436.50 KiB 415.74 KiB
ab6fee4 22.84 KiB 401.67 KiB 378.83 KiB
a5c946b 20.76 KiB 431.93 KiB 411.17 KiB
c2ec420 20.76 KiB 401.65 KiB 380.89 KiB
23e2db5 20.76 KiB 434.97 KiB 414.21 KiB
257c2a9 20.76 KiB 401.36 KiB 380.60 KiB
11ccc16 20.76 KiB 431.71 KiB 410.94 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
228a909 20.76 KiB 425.71 KiB 404.95 KiB

Previous results on branch: feat/add-data-use

Startup times

Revision Plain With Sentry Diff
37cb940 1275.78 ms 1276.62 ms 0.84 ms

App size

Revision Plain With Sentry Diff
37cb940 22.85 KiB 406.81 KiB 383.96 KiB

@armcknight
Copy link
Member

Im wondering whether it make sense to ship this manifest with the SDK. Each feature may be disabled by the user and therefore it should be the developer responsibility to add in its manifest which features they use.

It's a tough question, because either way there are risks affecting developers who either don't know about the new requirements and/or don't know we deliver functionality that requires it.

If we don't ship it, we risk customers' apps being rejected and not understanding why, and then later finding out it is our "fault".

If we do ship it, then developers' users may wonder why an app requires "unnecessary" functionality, and if the developers aren't aware of what's going on, they're going to wonder what we're doing to their app.

I feel like the former is better, that we put a hard stop at the developers, and not let the issue trickle down to users, because that's going to cause a lot more confusion.

This is also another argument in favor of modularizing the SDK, to isolate components that would require these declarations, so our customers can still use some parts of Sentry even if they can't/won't use these.

@brustolin
Copy link
Contributor Author

Adding to what @armcknight said. All of the information on the manifest is regarding capturing crash and performance data. It's extremely unlikely for a user to choose to use Sentry and disable this things.

I believe it's ok to ship the manifest.

@kahest
Copy link
Member

kahest commented Aug 30, 2023

My take: for the intents and purposes set out by Apple's Article on the topic, we should ship the manifest as it is laid out in this PR, IMO it covers the bare minimum the SDK does. The core sentence is the callout at the top

Third-party SDKs need to provide their own privacy manifest files that record the types of data they collect. Your app’s privacy manifest file doesn’t need to cover data collected by third-party SDKs that your app links to.

The only drawback I see is that if Perf+Profiling are disabled, the usage of NSPrivacyCollectedDataTypePerformanceData could be removed. But not including the manifest seems incorrect to me atm.

@kahest
Copy link
Member

kahest commented Aug 30, 2023

It's also highly likely that app devs need to list more data usages for their apps, so they should be aware of how this works and what the requirements are. We should also follow up with better guidance on the Data Privacy page about what kinds of data usage they need to enter if they use certain data/features the SDK provides, see getsentry/sentry-docs#5724 and getsentry/rfcs#70

@brustolin brustolin merged commit 4b1a58e into main Aug 31, 2023
@brustolin brustolin deleted the feat/add-data-use branch August 31, 2023 06:37
airtelshivam pushed a commit to airtelshivam/sentry-cocoa that referenced this pull request Apr 12, 2024
* feat: Add data use in privacy manifests

* Update CHANGELOG.md

(cherry picked from commit 4b1a58e)
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.

3 participants