Skip to content

Introduce initialize method#213

Merged
wzieba merged 6 commits intotrunkfrom
introduce_initialize_method
Apr 24, 2024
Merged

Introduce initialize method#213
wzieba merged 6 commits intotrunkfrom
introduce_initialize_method

Conversation

@wzieba
Copy link
Copy Markdown
Member

@wzieba wzieba commented Apr 22, 2024

Closes #103

Description

This PR introduces CrashLogging#initialize method. The motivation for this is to reduce risks of issues like this one: Automattic/simplenote-android#1518 . In other words: let's not assume that CrashLogging will be injected in the main application, but rather enforce it.

Testing

You can edit gradle.properties and provide DSN of a test project (you can find it on Sentry) and report some events using sample app.

wzieba added 2 commits April 22, 2024 15:48
Instead of relying on initialization during construction invocation of `SentryCrashLogging`, we'll now ask clients to initialize the SDK at the convenient lifecycle moment.
@wzieba wzieba requested a review from iangmaia April 22, 2024 14:27
@wzieba wzieba marked this pull request as ready for review April 22, 2024 14:27
) : CrashLogging {
init {

override fun initialize() {
Copy link
Copy Markdown
Contributor

@iangmaia iangmaia Apr 23, 2024

Choose a reason for hiding this comment

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

I don't think this is a must (or if you think it makes sense), but perhaps it can be useful to throw some IllegalStateException or similar to indicate the lack of initialization when performing some key action(s).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks @iangmaia . I was going back and forth with this, but motivated by your comment, I've added such check in fbcd738 . It works only in debug - I don't want to risk throwing unhandled exception in release for "uninitialized crash logger". But in debug it'll be helpful to spot any initialization issues.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's a nice compromise, as developers using the SDK will be able to course correct sooner if they're missing something.

Thanks for considering it!

Base automatically changed from extract_crashlogging_to_module to trunk April 24, 2024 11:47
wzieba and others added 2 commits April 24, 2024 15:43
…logging/CrashLogging.kt

Co-authored-by: Ian Guedes Maia <iangmaia@users.noreply.github.com>
Comment on lines +10 to +14
internal class AndroidApplicationInfoProvider(context: Context) : ApplicationInfoProvider {
override val debuggable: Boolean = context.applicationInfo.let {
it.flags and ApplicationInfo.FLAG_DEBUGGABLE != 0
}
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's not really possible to test without setting up Robolectric or instrumantation tests. I think it's too much effort for this in this library, but I've validated this behavior on Day One Android: the app crashes in debug but not in release if reporting without initialization:

Screenshot 2024-04-24 at 15 58 18

@wzieba
Copy link
Copy Markdown
Member Author

wzieba commented Apr 24, 2024

Thanks @iangmaia , ready for the second round!

Copy link
Copy Markdown
Contributor

@iangmaia iangmaia left a comment

Choose a reason for hiding this comment

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

:shipit:

@wzieba wzieba merged commit 1bcbea5 into trunk Apr 24, 2024
@wzieba wzieba deleted the introduce_initialize_method branch April 24, 2024 14:33
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.

Provide CrashLogging#initialize method

2 participants