Skip to content

Comments

Report Startup Crashes#2277

Merged
romtsn merged 17 commits intomainfrom
feat/startup-crashes
Oct 17, 2022
Merged

Report Startup Crashes#2277
romtsn merged 17 commits intomainfrom
feat/startup-crashes

Conversation

@romtsn
Copy link
Member

@romtsn romtsn commented Oct 5, 2022 •
edited
Loading

📜 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 markushi, philipphofmann and stefanosiano and removed request for stefanosiano October 5, 2022 15:42
marandaneto
marandaneto reviewed Oct 5, 2022
View reviewed changes
adinauer
adinauer reviewed Oct 6, 2022
View reviewed changes
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

marandaneto
marandaneto reviewed Oct 6, 2022
View reviewed changes
marandaneto
marandaneto reviewed Oct 6, 2022
View reviewed changes
marandaneto
marandaneto reviewed Oct 6, 2022
View reviewed changes
marandaneto
marandaneto reviewed Oct 6, 2022
View reviewed changes
philipphofmann
philipphofmann reviewed Oct 10, 2022
View reviewed changes
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.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022 •
edited
Loading

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 •
edited
Loading

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

markushi 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.

@romtsn romtsn marked this pull request as ready for review October 13, 2022 11:39
@romtsn romtsn requested a review from stefanosiano as a code owner 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 philipphofmann mentioned this pull request Oct 14, 2022
Closed
@philipphofmann
Copy link
Member

philipphofmann commented Oct 14, 2022

@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.

adinauer
adinauer approved these changes Oct 14, 2022
View reviewed changes
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

@romtsn romtsn changed the title Block init to flush Startup Crashes Report Startup Crashes Oct 14, 2022
@romtsn romtsn merged commit a4fb390 into main Oct 17, 2022
@romtsn romtsn deleted the feat/startup-crashes branch October 17, 2022 20:27
@romtsn romtsn mentioned this pull request Oct 17, 2022
Closed
27 tasks
@philipphofmann
Copy link
Member

philipphofmann commented Oct 18, 2022

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 👍

@philipphofmann philipphofmann mentioned this pull request Oct 24, 2022
Closed
5 tasks
@gslevinkov gslevinkov mentioned this pull request Oct 27, 2022
Merged
@wangbax wangbax mentioned this pull request Nov 17, 2022
Closed
@romtsn romtsn mentioned this pull request Sep 1, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@philipphofmann philipphofmann philipphofmann left review comments

@adinauer adinauer adinauer approved these changes

@markushi markushi Awaiting requested review from markushi markushi is a code owner

@stefanosiano stefanosiano Awaiting requested review from stefanosiano

+1 more reviewer

@marandaneto marandaneto marandaneto left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@romtsn @markushi @philipphofmann @adinauer @marandaneto @getsentry-bot