Fix: do not set free memory and is low memory fields when it's a NDK hard crash #1399
Conversation
|
@bruno-garcia let's discuss this, there are more device data that is nonstatic and should not be applied if it's a hard crash. |
| @@ -314,12 +316,15 @@ private void setArchitectures(final @NotNull Device device) { | |||
| device.setModelId(Build.ID); | |||
| setArchitectures(device); | |||
|
|
|||
| // likely to be applied only if applyScopeData | |||
There was a problem hiding this comment.
the chances that the user opens the app again right away is high, so this won't change much
There was a problem hiding this comment.
Even though it's more likely that an app that crashes is reopened immediately by the user, I believe we can agree this is also not the case sometimes. And this sometimes multipled by the number of events we'll get through it'll be millions of false data.
That said, we could 'meet in the middle' and verify the timestamp of the event being within "n seconds" of the current time. And if so, apply any of such data.
Similar approach could be used with the performance product (transactions) for data that is costly to retrieve, so that we cache the value for that amount of seconds and hence, accept that the value could be slightly off from when the event actually happened.
How does that sound?
There was a problem hiding this comment.
I still think the right approach would be doing that thru sentry-native
getsentry/sentry-native#520
hence I'm going to remove free memory and is low memory on this PR, but let's collaborate on the other fields/approach in another PR.
sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #1399 +/- ##
=========================================
Coverage 75.73% 75.73%
Complexity 1861 1861
=========================================
Files 185 185
Lines 6371 6371
Branches 637 637
=========================================
Hits 4825 4825
Misses 1258 1258
Partials 288 288 Continue to review full report at Codecov.
|
marandaneto
changed the title
| @@ -302,7 +304,7 @@ private void setArchitectures(final @NotNull Device device) { | |||
|
|
|||
| // we can get some inspiration here | |||
| // https://github.com/flutter/plugins/blob/master/packages/device_info/android/src/main/java/io/flutter/plugins/deviceinfo/DeviceInfoPlugin.java | |||
| private @NotNull Device getDevice() { | |||
| private @NotNull Device getDevice(final boolean applyScopeData) { | |||
There was a problem hiding this comment.
l: Maybe we can find a better name for applyScopeData. We use it to control whether we add free and low memory. The name could reflect that.
There was a problem hiding this comment.
the naming cant be related to free memory, because the same flag would be used to every nonstatic data that cant be applied after the restart, so it's pretty much how we call on every integration that has such check, applyScopeData
sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java
Show resolved
Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt
Show resolved
Hide resolved
…sentry-java into fix/device-scope-data
📜 Description
💡 Motivation and Context
#1395
💚 How did you test it?
📝 Checklist
🔮 Next steps