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

Use getMyMemoryState() instead of getRunningAppProcesses() #3004

Merged
merged 6 commits into from
Oct 23, 2023

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Oct 20, 2023

📜 Description

Use ActivityManager.getMyMemoryState(); instead of ActivityManager.getRunningAppProcesses() to retrieve process importance to prevent some app stores from flagging apps as violating privacies.

The PR is targeting v7 because getMyMemoryState requires API 16, and we bumped it to 19 in v7.

💡 Motivation and Context

Closes #2187

💚 How did you test it?

Automated

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@romtsn romtsn changed the title Use ActivityManager.getMyMemoryState(); instead of ActivityManager.getRunningAppProcesses() to prevent some app stores from flagging apps as violating privacies. Use getMyMemoryState() instead of getRunningAppProcesses() to prevent some app stores from flagging apps as violating privacies. Oct 20, 2023
@romtsn romtsn changed the title Use getMyMemoryState() instead of getRunningAppProcesses() to prevent some app stores from flagging apps as violating privacies. Use getMyMemoryState() instead of getRunningAppProcesses() Oct 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 20, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 373.61 ms 453.88 ms 80.27 ms
Size 1.72 MiB 2.26 MiB 550.29 KiB

Baseline results on branch: feat/7.0.0

Startup times

Revision Plain With Sentry Diff
6a40795 370.00 ms 437.33 ms 67.33 ms
6684058 414.76 ms 511.46 ms 96.70 ms
db8763d 386.60 ms 453.66 ms 67.06 ms
ff784c6 388.51 ms 461.90 ms 73.39 ms

App size

Revision Plain With Sentry Diff
6a40795 1.72 MiB 2.26 MiB 550.26 KiB
6684058 1.72 MiB 2.26 MiB 550.19 KiB
db8763d 1.72 MiB 2.26 MiB 550.22 KiB
ff784c6 1.72 MiB 2.26 MiB 550.22 KiB

Previous results on branch: rz/fix/running-processes

Startup times

Revision Plain With Sentry Diff
6bfbb73 400.66 ms 478.04 ms 77.38 ms
422d8cb 375.00 ms 463.92 ms 88.92 ms
e16f9ae 355.59 ms 421.61 ms 66.02 ms

App size

Revision Plain With Sentry Diff
6bfbb73 1.72 MiB 2.26 MiB 550.29 KiB
422d8cb 1.72 MiB 2.26 MiB 550.29 KiB
e16f9ae 1.72 MiB 2.26 MiB 550.29 KiB

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Attention: 106 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...try/android/core/ActivityLifecycleIntegration.java 83.38% <100.00%> (+0.11%) ⬆️
...a/io/sentry/android/core/AndroidTransportGate.java 100.00% <100.00%> (ø)
...roid/core/AppComponentsBreadcrumbsIntegration.java 89.06% <100.00%> (ø)
...o/sentry/android/core/AppLifecycleIntegration.java 68.25% <100.00%> (ø)
...ain/java/io/sentry/android/core/AppStartState.java 97.72% <100.00%> (+0.10%) ⬆️
...try/android/core/DefaultAndroidEventProcessor.java 84.68% <100.00%> (-2.36%) ⬇️
.../android/core/EnvelopeFileObserverIntegration.java 83.33% <100.00%> (+0.57%) ⬆️
...in/java/io/sentry/android/core/NdkIntegration.java 91.83% <100.00%> (ø)
...ry/android/core/NetworkBreadcrumbsIntegration.java 93.93% <100.00%> (ø)
...android/core/PhoneStateBreadcrumbsIntegration.java 84.09% <100.00%> (ø)
... and 74 more

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@stefanosiano
Copy link
Member

looks good!
Just a couple of suggestions:

  1. Good to rename ContextUtilsTest, as it was confusing, but I'd keep the Test word inside the name, so that we know immediately it's only for tests, like ContextUtilsTestHelper
  2. There is ContextUtilsUnitTests, which tests ContextUtils. You can merge it inside your newly ContextUtilsTests

@romtsn romtsn merged commit 1f3f5dc into feat/7.0.0 Oct 23, 2023
17 checks passed
@romtsn romtsn deleted the rz/fix/running-processes branch October 23, 2023 19:28
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.

2 participants