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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report Startup Crashes #2277

Merged
merged 17 commits into from Oct 17, 2022
Merged

Report Startup Crashes #2277

merged 17 commits into from Oct 17, 2022

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Oct 5, 2022

馃摐 Description

  • Write Startup Crash marker file to disk when storing the envelope (if a crash happened within 2 seconds (configurable) after the SDK init)
  • Read this marker file in SendCachedFireAndForgetIntegration and block the execution of init in case it exists for up to 5 seconds (configurable)
  • Continue asynchronously in background in case the blocking flush times out

馃挕 Motivation and Context

getsentry/team-mobile#12

馃挌 How did you test it?

馃摑 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

馃敭 Next steps

@romtsn romtsn requested review from philipphofmann, markushi and stefanosiano and removed request for stefanosiano October 5, 2022 15:42
Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Left some questions. Maybe @philipphofmann also wants to take a quick look since he implemented getsentry/sentry-cocoa#2220

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Concept-wise, this looks OK; I didn't take a too close look at the code thought.

sentry/src/main/java/io/sentry/SentryOptions.java Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022

Messages
馃摉 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 馃毇 dangerJS against be08f25

@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022

Performance metrics 馃殌

Plain With Sentry Diff
Startup time 335.00 ms 353.60 ms 18.60 ms
Size 1.73 MiB 2.29 MiB 581.39 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3d89dea 322.38 ms 350.82 ms 28.45 ms
54cebc8 331.12 ms 385.14 ms 54.02 ms
4dd88fe 302.12 ms 331.17 ms 29.04 ms
7d87f22 348.79 ms 378.46 ms 29.67 ms
3d89dea 345.59 ms 364.06 ms 18.47 ms
4ca1d7b 331.33 ms 335.78 ms 4.45 ms
2c5f172 351.18 ms 373.52 ms 22.34 ms
7300956 337.57 ms 384.21 ms 46.64 ms
1e4690d 354.69 ms 387.88 ms 33.19 ms
2c5f172 310.20 ms 357.16 ms 46.96 ms

App size

Revision Plain With Sentry Diff
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB
54cebc8 1.73 MiB 2.29 MiB 579.43 KiB
4dd88fe 1.73 MiB 2.29 MiB 579.50 KiB
7d87f22 1.73 MiB 2.29 MiB 580.01 KiB
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB
4ca1d7b 1.73 MiB 2.29 MiB 579.88 KiB
2c5f172 1.73 MiB 2.29 MiB 580.10 KiB
7300956 1.73 MiB 2.29 MiB 578.69 KiB
1e4690d 1.74 MiB 2.33 MiB 604.92 KiB
2c5f172 1.73 MiB 2.29 MiB 580.10 KiB

Previous results on branch: feat/startup-crashes

Startup times

Revision Plain With Sentry Diff
d5d6cf3 375.93 ms 405.68 ms 29.75 ms
98043eb 330.04 ms 368.90 ms 38.86 ms
485f4dd 301.36 ms 346.67 ms 45.31 ms
f1f719e 330.89 ms 356.18 ms 25.29 ms
34a9734 310.16 ms 344.86 ms 34.70 ms
1ddea7b 285.65 ms 315.50 ms 29.85 ms
920f6da 335.00 ms 358.33 ms 23.33 ms
13dabe9 298.25 ms 368.96 ms 70.71 ms
0850b0c 288.33 ms 325.18 ms 36.85 ms

App size

Revision Plain With Sentry Diff
d5d6cf3 1.73 MiB 2.29 MiB 581.44 KiB
98043eb 1.73 MiB 2.29 MiB 580.83 KiB
485f4dd 1.73 MiB 2.29 MiB 580.83 KiB
f1f719e 1.73 MiB 2.29 MiB 580.96 KiB
34a9734 1.73 MiB 2.29 MiB 580.96 KiB
1ddea7b 1.73 MiB 2.29 MiB 581.44 KiB
920f6da 1.73 MiB 2.29 MiB 581.44 KiB
13dabe9 1.73 MiB 2.29 MiB 581.39 KiB
0850b0c 1.73 MiB 2.29 MiB 581.15 KiB

@markushi
Copy link
Member

Looks good to me! Is there a reason why this feature is Android-only? I guess it could be useful for Java as well.

@romtsn romtsn marked this pull request as ready for review October 13, 2022 11:39
@romtsn
Copy link
Member Author

romtsn commented Oct 13, 2022

this one is ready for review now, I still need at least one approval 馃槃 @philipphofmann @marandaneto @adinauer

@romtsn
Copy link
Member Author

romtsn commented Oct 13, 2022

Looks good to me! Is there a reason why this feature is Android-only? I guess it could be useful for Java as well.

yeah, because for backend/desktop this SendCachedEnvelope integrations are optional, because they have a ShutdownHook to flush the pending events in a blocking way (which isn't working on Android). See #2277 (comment)

@philipphofmann
Copy link
Member

@kevinrenskers, made a good point in getsentry/sentry-cocoa#2283 (comment) on why you should have to use enableStartupCrashDetection. Unless this feature is broken, I don't think there is a good use case for disabling this feature.

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

LGTM; maybe update PR title

CHANGELOG.md Show resolved Hide resolved
@romtsn romtsn changed the title Block init to flush Startup Crashes Report Startup Crashes Oct 14, 2022
@philipphofmann
Copy link
Member

Thank you, @romtsn, for shipping this feature and being patient with my comments about the options.

@romtsn
Copy link
Member Author

romtsn commented Oct 18, 2022

thanks for the thorough review and prior art 馃憤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants