-
-
Notifications
You must be signed in to change notification settings - Fork 441
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
fix(session): When capturing unhandled hybrid exception session should be ended #3480
fix(session): When capturing unhandled hybrid exception session should be ended #3480
Conversation
Performance metrics 🚀
|
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 |
This is also important for Flutter, as dart uncaught error do not crash the application. |
…-exceptions' into kw/fix-handling-hybrid-unhandled-exceptions
…-exceptions' into kw/fix-handling-hybrid-unhandled-exceptions
@@ -148,7 +155,8 @@ public static Map<String, Object> serializeScope( | |||
* captured | |||
*/ | |||
@Nullable | |||
public static SentryId captureEnvelope(final @NotNull byte[] envelopeData) { | |||
public static SentryId captureEnvelope( | |||
final @NotNull byte[] envelopeData, final boolean maybeStartNewSession) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
@krystofwoldrich did you mean to close this pr? think it happened automatically |
@buenaflor Thanks, no that happened by accident. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! I left a few clarifying questions
sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java
Outdated
Show resolved
Hide resolved
@@ -148,7 +155,8 @@ public static Map<String, Object> serializeScope( | |||
* captured | |||
*/ | |||
@Nullable | |||
public static SentryId captureEnvelope(final @NotNull byte[] envelopeData) { | |||
public static SentryId captureEnvelope( | |||
final @NotNull byte[] envelopeData, final boolean maybeStartNewSession) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -252,6 +264,26 @@ private static void addTimeSpanToSerializedSpans(TimeSpan span, List<Map<String, | |||
spans.add(spanMap); | |||
} | |||
|
|||
private static void deleteCurrentSessionFile(final @NotNull SentryOptions options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm why do you need to delete the file manually here? I see we anyway delete the session file on session end here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for this is the warning I get if not cleaning the session file
options.getLogger().log(WARNING, "Current session is not ended, we'd need to end it."); |
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. |
sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
📜 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.💡 Motivation and Context
handled:false
set in JS sentry-react-native#3614storeEnvelope
updates session when passed unhandled event sentry-cocoa#4073mechanism.handled:false
should crash current session sentry-react-native#3900💚 How did you test it?
rn sample app, unit tests
📝 Checklist
sendDefaultPII
is enabled.