getsentry / sentry-cocoa

The official Sentry SDK for iOS, tvOS, macOS, watchOS.
https://sentry.io/for/cocoa/
MIT License
812 stars 324 forks source link

Out of Memory Crash Report Has Incorrect User Id #1152

Open jimrutherford opened 3 years ago

jimrutherford commented 3 years ago

Environment

How do you use Sentry? Sentry SaaS (sentry.io)

Which SDK and version? Swift 7.0.3

Steps to Reproduce

  1. Review an OutOfMemory crash report
  2. Observe User.id value does not match User.id value as set by SentrySDK.setUser(...)

Expected Result

I expect that the value I set as the user id for the sentry session would appear in OOM crash reports. We have over 100 OOM crashes affecting 92 users and none of the 92 users match any user id's in our user database. All other non-OOM crash reports we observe have user id's that match users in our database.

Actual Result

We are observing that for OOM crash reports, reported user ids do not match any user id's in our database.

philipphofmann commented 3 years ago

Hey @jimrutherford, thanks for reaching out. The SDKs uses installationId as a fallback if you don't provide a user, see our docs. When your application uses too much RAM, the operating system kills your app without a normal termination process. This means we can't store the data of the scope when the app terminates as we do for crashes. Therefore we lose most of the context. One possibility to solve this problem would be to write the scope to disk whenever you change it. We didn't go for this approach because this would cause a lot of IO.

However, when do you call SentrySDK.setUser(...)? Directly after the SDK init or later?

CraigSiemens commented 3 years ago

Just an idea, what if you only wrote to disk when applicationDidReceiveMemoryWarning is received? It'd allow sentry to try and persist the details so it's available incase the app is terminated.

philipphofmann commented 3 years ago

Thanks for the input, @CraigSiemens, but the problem is that there is no guarantee to receive applicationDidReceiveMemoryWarning before an OOM occurs.

We could only put data to an OOM crash that we are sure is accurate. This would mean less data, but we wouldn't confuse our users.

philipphofmann commented 3 years ago

We can store some information to disk when receiving an applicationDidReceiveMemoryWarning notification. As pointed out, there is no guarantee of receiving the notification before an OOM, but we could get some context for some of them. It is worth noting that the OOM and a previous applicationDidReceiveMemoryWarning notification don't have to be related to each other.

Another possible solution is to persist the scope periodically to disk, which a scope change, some time interval, or an event to be defined can trigger. As this is going to cause more I/O, the SDK could offer a feature flag.

Helpful information to attach to the OOM:

bruno-garcia commented 3 years ago

It sounds like we need to clarify how this works on the troubleshooting page of the docs. Perhaps suggestion a BeforeSend solution where the user can manage persisting data and setting to the event after restart.

philipphofmann commented 2 years ago

As @bruno-garcia said, we should communicate better in the docs https://github.com/getsentry/sentry-docs/issues/4499 what an OOM on Apple is. We should build awareness that this is not a standard issue.

philipphofmann commented 1 year ago

We could solve this problem by implementing the setUser scope observer here https://github.com/getsentry/sentry-cocoa/blob/4af7581aaf9cf964c5e7ab54f9f31aa8a1d26f75/Sources/Sentry/SentryWatchdogTerminationScopeObserver.m#L157-L160 and then read the user similar as we do it for breadcrumbs: https://github.com/getsentry/sentry-cocoa/blob/4af7581aaf9cf964c5e7ab54f9f31aa8a1d26f75/Sources/Sentry/SentryWatchdogTerminationTracker.m#L63-L64