-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: timber integration #464
feat: timber integration #464
Conversation
sentry-android-timber/src/main/java/io/sentry/android/timber/SentryTimberTree.kt
Outdated
Show resolved
Hide resolved
// let's take only the 1st line as this is probably the only message written by the user. | ||
// or the 1st line of the exception which is probably the same as event.title | ||
// eg java.lang.Exception: java.lang.Exception: java.lang.Exception: Some exception. | ||
// timber should offer a way of disabling it |
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.
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.
Would it make sense to put the whole integration in a folder that is called logging-integrations
or would you do this once we have more integrations?
Overall really nice job 👍
sentry-android-timber/src/main/java/io/sentry/android/timber/SentryTimberIntegration.kt
Outdated
Show resolved
Hide resolved
sentry-android-timber/src/main/java/io/sentry/android/timber/SentryTimberIntegration.kt
Show resolved
Hide resolved
import kotlin.test.assertEquals | ||
import timber.log.Timber | ||
|
||
class SentryTimberIntegrationTest { |
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.
m
: It would be nice to have an integration test for testing the whole integration with our SDK. Checking if running the following code and then logging via timber actually passes the timber events to our SDK.
SentryAndroid.init(
this,
options -> {
options.addIntegration(new SentryTimberIntegration());
});
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.
mm it's possible, but we don't do this for any other integration I guess, as what really matters is the Hub
to be called and this is already tested -> SentryTimberTreeTest
I'll check that though
sentry-android-timber/src/main/java/io/sentry/android/timber/SentryTimberTree.kt
Outdated
Show resolved
Hide resolved
sentry-android-timber/src/test/java/io/sentry/android/timber/SentryTimberTreeTest.kt
Show resolved
Hide resolved
sentry-android-timber/src/test/java/io/sentry/android/timber/SentryTimberTreeTest.kt
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.
Good job 👏
📢 Type of change
📜 Description
feat: timber integration
💡 Motivation and Context
one may raise events from Timber
💚 How did you test it?
running and unit tests
📝 Checklist
🔮 Next steps
revert manifest and self init, but we need to define the sample somewhere