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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option for start up crashes #2283

Closed
philipphofmann opened this issue Oct 13, 2022 · 6 comments
Closed

Add option for start up crashes #2283

philipphofmann opened this issue Oct 13, 2022 · 6 comments
Assignees

Comments

@philipphofmann
Copy link
Member

Description

Add an option enableStartupCrashDetection with a default of true, so users can disable the start up crahes detection added with #2220.

Please also add this option to the docs.

@kevinrenskers
Copy link
Contributor

Why would someone want to disable this feature?

@philipphofmann
Copy link
Member Author

If it causes problems, or you never want your app to be blocked at start-up. Ideally, you should never use it. @romtsn, I think @kevinrenskers has a point. Should we maybe also remove it from getsentry/sentry-java#2277?

@romtsn
Copy link
Member

romtsn commented Oct 14, 2022

you never want your app to be blocked at start-up.

I think this is the reason we should have it. Because we made some assumptions on the flush timeout and startup crash threshold, doesn't mean that they are correct. 2 seconds threshold might very well be enough for most of the apps in developed countries to flush out the envelopes, so that they don't even need to block and would like to disable this altogether.

Another point is - we care so much about performance and talk how we should make it a nobrainer for our customers to buy, yet introducing a feature that potentially degrades one of the core mobile vitals (app start) without a possibility to disable it. That doesn't sound right to me.

@kevinrenskers
Copy link
Contributor

Then the question becomes: why would anyone want to enable it, if it causes problems? To me adding flags and options is not necessarily a good thing. The feature should either be useful and not cause problems, or should be made that way, so there is no reason to disable it. Not sure if that is possible here, but just adding a boolean seems weird to me.

@romtsn
Copy link
Member

romtsn commented Oct 14, 2022

yeah, this feature is more-or-less a tradeoff between getting a crash on Sentry no-matter-what and potential performance quirks caused by this. The problem is that this tradeoff is decided by us and we don't leave any room for the users to change this decision, although it ultimately affects their performance.

Initially I wanted to make both the flushTimeout and the crashThreshold configurable, so the users could set their own numeric values and tweak them at their disposal, but Philipp was opposed to this. Also, the original issue states:

The SDK init function doesn't return until the start-up crash is sent to Sentry or until a configurable timeout is reached.

In any case, I don't wanna spend more time on bikeshedding here, so I can remove those options or keep those 2 options only on Android, but this would lead to misalignment between SDKs.

@philipphofmann
Copy link
Member Author

As discussed on a call with @romtsn, we decided that we don't add this option for now. We think that this feature should be on by default, and there should be no good reason to disable it, as you accept the app start being blocked if there was a crash shortly after the app start on a previous run. We can always add this option later.

Repository owner moved this from Needs Discussion to Done in Mobile & Cross Platform SDK Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants