fix(session): When capturing unhandled hybrid exception session should be ended by krystofwoldrich · Pull Request #3480 · getsentry/sentry-java

krystofwoldrich · 2024-06-14T14:58:07Z

📜 Description

This PR fixes the session handling for hybrid envelopes with unhandled exceptions.

If hybrid exception is unhandled but doesn't crash the application the native captureEnvelope should create a new session.

If the hybrid exception is unhandled and crashes the application the native captureEnvelope should not create a new session.

This is aligned with the iOS behaviour implemented in feat: Close session for unhandled error sentry-cocoa#3091

💡 Motivation and Context

fixes iOS doesn't update session when handled:false set in JS sentry-react-native#3614
related iOS fix fix(sessions): storeEnvelope updates session when passed unhandled event sentry-cocoa#4073
related RN fix fix(sessions): mechanism.handled:false should crash current session sentry-react-native#3900

💚 How did you test it?

rn sample app, unit tests

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

…d be ended

github-actions · 2024-06-14T15:09:51Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	447.00 ms	516.10 ms	69.10 ms
Size	1.70 MiB	2.28 MiB	593.07 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`baaf637`	462.32 ms	579.22 ms	116.90 ms
`28c9a83`	416.98 ms	479.90 ms	62.92 ms
`201048a`	418.62 ms	481.67 ms	63.05 ms
`7ab32b6`	373.62 ms	480.61 ms	106.99 ms
`b172d4e`	352.92 ms	446.50 ms	93.58 ms
`3d8bd2b`	364.76 ms	469.98 ms	105.22 ms
`1e05e43`	411.00 ms	467.29 ms	56.29 ms
`c554ca2`	383.78 ms	449.84 ms	66.06 ms
`0bd723b`	375.20 ms	452.41 ms	77.20 ms
`99a51e2`	405.11 ms	479.65 ms	74.54 ms

App size

Revision	Plain	With Sentry	Diff
`baaf637`	1.72 MiB	2.27 MiB	558.42 KiB
`28c9a83`	1.70 MiB	2.28 MiB	592.00 KiB
`201048a`	1.70 MiB	2.28 MiB	592.32 KiB
`7ab32b6`	1.70 MiB	2.27 MiB	584.63 KiB
`b172d4e`	1.72 MiB	2.29 MiB	578.09 KiB
`3d8bd2b`	1.72 MiB	2.29 MiB	577.53 KiB
`1e05e43`	1.70 MiB	2.28 MiB	590.89 KiB
`c554ca2`	1.70 MiB	2.27 MiB	582.25 KiB
`0bd723b`	1.72 MiB	2.29 MiB	578.09 KiB
`99a51e2`	1.72 MiB	2.29 MiB	576.34 KiB

Previous results on branch: kw/fix-handling-hybrid-unhandled-exceptions

Startup times

Revision	Plain	With Sentry	Diff
`2b69d80`	403.89 ms	465.62 ms	61.73 ms
`34dc85a`	339.22 ms	408.74 ms	69.52 ms
`5680f95`	383.74 ms	442.60 ms	58.87 ms
`450ed8e`	352.68 ms	417.12 ms	64.44 ms
`28f32eb`	370.24 ms	420.49 ms	50.24 ms
`9d79d24`	429.73 ms	484.74 ms	55.01 ms
`3032525`	377.39 ms	428.41 ms	51.02 ms

App size

Revision	Plain	With Sentry	Diff
`2b69d80`	1.70 MiB	2.28 MiB	592.35 KiB
`34dc85a`	1.70 MiB	2.28 MiB	592.47 KiB
`5680f95`	1.70 MiB	2.28 MiB	592.35 KiB
`450ed8e`	1.70 MiB	2.28 MiB	592.47 KiB
`28f32eb`	1.70 MiB	2.28 MiB	592.47 KiB
`9d79d24`	1.70 MiB	2.28 MiB	593.07 KiB
`3032525`	1.70 MiB	2.28 MiB	592.47 KiB

…sion if enabled

krystofwoldrich · 2024-06-18T11:53:43Z

This is also important for Flutter, as dart uncaught error do not crash the application.

@buenaflor

…-exceptions' into kw/fix-handling-hybrid-unhandled-exceptions

buenaflor · 2024-06-19T11:28:59Z

  @Nullable
-  public static SentryId captureEnvelope(final @NotNull byte[] envelopeData) {
+  public static SentryId captureEnvelope(
+      final @NotNull byte[] envelopeData, final boolean maybeStartNewSession) {


So if I get it right on the Flutter Android side I just need to check if the error is unhandled and set maybeStartNewSession to true in those cases

Yes, exactly.

Does the envelope contain any extra information we could use instead of maybeStartNewSession?
I see that we have event.isCrashed() maybe, having a event.isUnhandled() would make sense as well.

buenaflor · 2024-06-19T11:31:34Z

Yeah makes sense for Flutter as well.

Just an idea, maybe we could surface it in the product a bit better that Flutter does not crash (only the native parts) so users understand what crash free sessions are made out of on Flutter

buenaflor · 2024-06-20T08:41:44Z

@krystofwoldrich did you mean to close this pr? think it happened automatically

krystofwoldrich · 2024-06-20T10:57:42Z

@buenaflor Thanks, no that happened by accident.

markushi

Looks good overall! I left a few clarifying questions

markushi · 2024-06-21T11:36:37Z

  @Nullable
-  public static SentryId captureEnvelope(final @NotNull byte[] envelopeData) {
+  public static SentryId captureEnvelope(
+      final @NotNull byte[] envelopeData, final boolean maybeStartNewSession) {


Does the envelope contain any extra information we could use instead of maybeStartNewSession?
I see that we have event.isCrashed() maybe, having a event.isUnhandled() would make sense as well.

markushi · 2024-06-21T11:39:34Z

    spans.add(spanMap);
  }

+  private static void deleteCurrentSessionFile(final @NotNull SentryOptions options) {


Hmm why do you need to delete the file manually here? I see we anyway delete the session file on session end here.

Hmm, when I don't explicitly delete it, it's not removed and it's read on the next app start. But I didn't know it should be deleted automatically.

If I'm seeing this right, the envelope is only deleted when the session end is send as standalone envelope, but in the case of hybrids the session end will be send with the crash event in the same envelope.

Just for this is the warning I get if not cleaning the session file

sentry-java/sentry/src/main/java/io/sentry/PreviousSessionFinalizer.java

Line 74 in 6de823e

options.getLogger().log(WARNING, "Current session is not ended, we'd need to end it.");

krystofwoldrich · 2024-06-21T14:45:13Z

Does the envelope contain any extra information we could use instead of maybeStartNewSession?
I see that we have event.isCrashed() maybe, having a event.isUnhandled() would make sense as well.

Not at the moment, if the event is coming from the global error handler in RN, we have ensure no new session is created, as the application is going to crash.

If the user set the unhandled flag we need to create a new session, because the application is not going to crash.

In flutter all unhandled events are not crashing so we alway need to create a new session if unhandled.

We could check for the platform and mechanism type and unhandled, but to me setting the behaviour in the hybrid SDK looks more clear. Plus other hybrids might not fit into checking platform and mechanism type and unhandled.

stefanosiano

lgtm!

…d-unhandled-exceptions

fix(session): When capturing unhandled hybrid exception session shoul…

f94cbe2

…d be ended

krystofwoldrich mentioned this pull request Jun 14, 2024

fix(sessions): storeEnvelope updates session when passed unhandled event getsentry/sentry-cocoa#4073

Merged

7 tasks

Krystof Woldrich and others added 2 commits June 17, 2024 18:22

add clear session, add test, only capture session once, start new ses…

f9538c8

…sion if enabled

Format code

7ce635b

krystofwoldrich mentioned this pull request Jun 18, 2024

fix(sessions): mechanism.handled:false should crash current session getsentry/sentry-react-native#3900

Merged

8 tasks

krystofwoldrich marked this pull request as ready for review June 18, 2024 11:53

krystofwoldrich requested review from adinauer, markushi, romtsn and stefanosiano as code owners June 18, 2024 11:53

krystofwoldrich and others added 4 commits June 18, 2024 13:53

Merge branch 'main' into kw/fix-handling-hybrid-unhandled-exceptions

f59fbfc

remove processed session file

0181755

Merge remote-tracking branch 'origin/kw/fix-handling-hybrid-unhandled…

36b1640

…-exceptions' into kw/fix-handling-hybrid-unhandled-exceptions

Format code

0193bd7

krystofwoldrich requested a review from buenaflor June 18, 2024 12:26

Krystof Woldrich added 2 commits June 18, 2024 18:23

update api

7ec01cc

Merge remote-tracking branch 'origin/kw/fix-handling-hybrid-unhandled…

7128222

…-exceptions' into kw/fix-handling-hybrid-unhandled-exceptions

buenaflor reviewed Jun 19, 2024

View reviewed changes

krystofwoldrich closed this in getsentry/sentry-cocoa#4073 Jun 20, 2024

buenaflor reopened this Jun 20, 2024

markushi reviewed Jun 21, 2024

View reviewed changes

Krystof Woldrich and others added 3 commits June 21, 2024 17:04

fix test, and review comments

23b7734

add changelog

6a62b9b

Merge branch 'main' into kw/fix-handling-hybrid-unhandled-exceptions

92eec8e

stefanosiano reviewed Jun 25, 2024

View reviewed changes

Comment thread sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java

stefanosiano approved these changes Jun 25, 2024

View reviewed changes

krystofwoldrich and others added 4 commits June 27, 2024 16:56

Merge branch 'main' into kw/fix-handling-hybrid-unhandled-exceptions

42f4007

fix file io on the main thread

06683e6

fix format

e944901

Merge remote-tracking branch 'origin/main' into kw/fix-handling-hybri…

6de823e

…d-unhandled-exceptions

stefanosiano reviewed Jun 28, 2024

View reviewed changes

Comment thread sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java Outdated

stefanosiano approved these changes Jun 28, 2024

View reviewed changes

krystofwoldrich merged commit 8b0dfba into main Jun 28, 2024

krystofwoldrich deleted the kw/fix-handling-hybrid-unhandled-exceptions branch June 28, 2024 10:41

Uh oh!

Uh oh!

Conversation

krystofwoldrich commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

Uh oh!

github-actions Bot commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: kw/fix-handling-hybrid-unhandled-exceptions

Startup times

App size

Uh oh!

krystofwoldrich commented Jun 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buenaflor commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buenaflor commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krystofwoldrich commented Jun 20, 2024

Uh oh!

markushi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krystofwoldrich commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

stefanosiano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

krystofwoldrich commented Jun 14, 2024 •

edited

Loading

github-actions Bot commented Jun 14, 2024 •

edited

Loading

buenaflor commented Jun 19, 2024 •

edited

Loading

buenaflor commented Jun 20, 2024 •

edited

Loading

krystofwoldrich commented Jun 21, 2024 •

edited

Loading