Automattic / Automattic-Tracks-iOS

Client library for tracking user events for later analysis
GNU General Public License v2.0
43 stars 12 forks source link

Expose an API to override Sentry's stitchAsyncCode option #260

Closed salimbraksa closed 1 year ago

salimbraksa commented 1 year ago

Ref p1687271187638419-slack-C05CANZ2B6F

As the PR title says, we are adding the options.stitchAsyncCode during the Sentry initialization. It is false by default but clients can override that option.

This PR is needed for:

At the time of writing this PR, there is a newer option in the latest Sentry version 8.8.0, named swiftAsyncStacktraces. However, this option didn't provide the expected results in the test cases outlined in https://github.com/wordpress-mobile/WordPress-iOS/pull/20912. In contrast, the stitchAsyncCode option delivered better results.


staskus commented 1 year ago

Given stitchAsyncCode was removed due to instability, should we then go with swiftAsyncStacktraces instead? Or do you think it's still dangerous due to being experimental? They mention in PR:

Switching from backtrace to backtrace_async is super trivial and gets Sentry 90% of the way to offering good Swift Concurrency support.

mokagio commented 1 year ago

@staskus @salimbraksa

should we then go with swiftAsyncStacktraces instead? Or do you think it's still dangerous due to being experimental?

The feature being experimental is a liability, in that they might roll it back at some point forcing us to update our code. But, given that's their latest version it seems a more secure approach than using a setting they already removed.

staskus commented 1 year ago

The feature being experimental is a liability, in that they might roll it back at some point forcing us to update our code

True, it is still an experimental and early feature.

Given that's their latest version it seems a more secure approach than using a setting they already removed.

As far as I understood, this option seems to be safe. For non-swift-async use cases, internals behaves the same way as before:

backtrace_async() behaves exactly like backtrace() unless it is invoked from a Swift async context. In that case, instead of writing the return addresses of the OS call stack, the continuation addresses of the async invocations that led to the current state are provided. -- https://keith.github.io/xcode-man-pages/backtrace.3.html#backtrace_async

However, as, @salimbraksa mentioned, this option is not as effective now:

At the time of writing this PR, there is a https://github.com/getsentry/sentry-cocoa/pull/3051 in the latest Sentry version 8.8.0, named swiftAsyncStacktraces. However, this option didn't provide the expected results in the test cases outlined in https://github.com/wordpress-mobile/WordPress-iOS/pull/20912. In contrast, the stitchAsyncCode option delivered better results.

AFAIK, it's only designed to work for native Swift concurrency (async/await). There aren't many places in the code using it yet.

Given the feature is experimental and introduced in the latest Sentry version, and is not likely to provide many insights at the moment, I say we wait for a newer Sentry version when this Swift async support is more solid.

@mokagio, @salimbraksa

salimbraksa commented 1 year ago

Given the feature is experimental and introduced in the latest Sentry version, and is not likely to provide many insights at the moment, I say we wait for a newer Sentry version when this https://github.com/getsentry/sentry-cocoa/issues/304.

That sounds good to me. My initial intention was to use the experimental stitchAsyncCode as a last resort if none of the other solutions worked. And I intended to use it like this:

  1. Enable this feature only in the Beta app, and invite some users ( or Matt ) to use the Beta build.
  2. Enable this feature exclusively for users who are consistently encountering crashes. By doing so, users who have never experienced crashes will remain unaffected by the potential instability of this experimental feature.

Given the reduced occurrence of __NSCFSet crashes following the 22.5.1.0 release ( see p1688004316907819-slack-C05CANZ2B6F ), this positive trend indicates that we may not currently need these experimental features. Therefore, I will close this PR for now, and we can reconsider if necessary at a later time.

@staskus @mokagio