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

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

Closed
wants to merge 6 commits into from

Conversation

salimbraksa
Copy link
Contributor

@salimbraksa salimbraksa commented Jun 20, 2023

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 wordpress-mobile/WordPress-iOS#20912. In contrast, the stitchAsyncCode option delivered better results.


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

@salimbraksa salimbraksa changed the title Add swiftAsyncStacktraces Sentry option. Disabled by default. Add stitchAsyncCode Sentry option. Disabled by default. Jun 20, 2023
@salimbraksa salimbraksa changed the title Add stitchAsyncCode Sentry option. Disabled by default. Expose an API to override Sentry's stitchAsyncCode option Jun 20, 2023
@salimbraksa salimbraksa self-assigned this Jun 20, 2023
@salimbraksa salimbraksa requested a review from mokagio June 20, 2023 19:07
@salimbraksa salimbraksa marked this pull request as ready for review June 20, 2023 19:07
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the time of writing this PR, there is a getsentry/sentry-cocoa#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 wordpress-mobile/WordPress-iOS#20912. In contrast, the stitchAsyncCode option delivered better results.

I appreciated that implementation might have been working well during testing, however, according to Sentry's own changelog

[The stitchAsyncCode] behavior was unpredictable and sometimes resulted in unexpected errors.

As a matter of fact, the option was removed from the library.

I'm afraid relying on this option will either result us facing the unexpected runtime behavior the Sentry team reported or put us in the difficult position of not being able to upgrade.

@@ -63,6 +63,7 @@ public class CrashLogging {

/// Attach stack traces to non-fatal errors
options.attachStacktrace = true
options.stitchAsyncCode = self.dataProvider.enhancedAsyncStacktraces
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the "enhanced async stacktraces" as a name for this parameter.

@staskus
Copy link

staskus commented Jun 22, 2023

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
Copy link
Contributor

mokagio commented Jun 23, 2023

@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
Copy link

staskus commented Jul 4, 2023

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 getsentry/sentry-cocoa#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 wordpress-mobile/WordPress-iOS#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
Copy link
Contributor Author

salimbraksa commented Jul 4, 2023

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 getsentry/sentry-cocoa#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

@salimbraksa salimbraksa closed this Jul 4, 2023
@mokagio mokagio deleted the task/sentry-stitch-async-code branch July 18, 2023 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants