develop icon indicating copy to clipboard operation
develop copied to clipboard

Finish and capture transaction/span bound to the Scope in case of a fatal crash-shutdown (eg Java Unhandled exception)

Open marandaneto opened this issue 4 years ago • 8 comments

  • Document spec https://develop.sentry.dev/sdk/performance/
    • What's gonna be the status?
    • Specify that this is on hard crash, just because background thread crashes doesn't mean that transaction should be auto-finished.
    • Only if we are aware in the handlers if the app will crash or not
    • Note that the transaction should be finished before the exception is captured
  • Change the SDKs
    • [ ] https://github.com/getsentry/sentry-java
    • [ ] https://github.com/getsentry/sentry-cocoa
    • [X] Dart
    • [ ] https://github.com/getsentry/sentry-react-native
    • [x] https://github.com/getsentry/sentry-dotnet

Add missing SDKs, See SDKs that support the Perf API https://docs.sentry.io/product/performance/getting-started/

marandaneto avatar Oct 04 '21 19:10 marandaneto

cc @rhcarvalho @AbhiPrasad @mitsuhiko in case you know which SDKs should do it as well, please edit the description so we can track it

marandaneto avatar Oct 04 '21 19:10 marandaneto

In sentry-cocoa it would require serializing the data into the SentryCrash state so it gets dumped on a crash so not super trivial. Mainly because of the overhead it'll add since any change in the transaction would require serialization.

On SDKs that have a runtime like ~Dart~ (Not in Flutter since it doesn't crash), C#, Java it could be as simple as changing the uncaught handlers to not only capturing the exception but closing any open transaction from the scope and capturing that too before terminating the app

bruno-garcia avatar Oct 20 '21 14:10 bruno-garcia

On Dart Flutter never crashes so we don't need to do anything.

On hybrid sdks we only care about the transaction running on its own layer (JS RN).

krystofwoldrich avatar Oct 13 '22 12:10 krystofwoldrich

For .NET we are addressing in https://github.com/getsentry/sentry-dotnet/pull/1996

We needed to choose a transaction status for these failed transactions. Since the unhandled exception is interrupting the transaction, we went with aborted.

mattjohnsonpint avatar Oct 17 '22 14:10 mattjohnsonpint

@mattjohnsonpint awesome, can you amend the spec for this specific use case?

marandaneto avatar Oct 17 '22 14:10 marandaneto

@mattjohnsonpint awesome, can you amend the spec for this specific use case?

@mattjohnsonpint have you had the chance to work on this? Apparently, .NET and JS diverge in status, .NET uses aborted and JS uses internal_error for quite some time.

marandaneto avatar Nov 30 '22 10:11 marandaneto

I haven't updated the spec yet. Thanks for the reminder.

In TSC it was said that this behavior wasn't applicable for JS. I also mentioned status aborted in TSC without any objection, so I think we should stick with that unless there's a concern?

I'm not sure that it really matters much, as long as it's not status ok.

mattjohnsonpint avatar Nov 30 '22 12:11 mattjohnsonpint

In TSC it was said that this behavior wasn't applicable for JS

JS sets the status, but the transaction keeps running because the process (browser) never dies, that's why it's not applicable to JS, so aborted wouldn't make sense, I believe the spec should include both behaviors.

  • If the process is going to die, set status to aborted and finish the transaction.
  • If the process/browser keeps running, set the status to internal_error and let the transaction finish itself.

marandaneto avatar Nov 30 '22 12:11 marandaneto