Bump Sentry SDK to 4.3.0

[wzieba]

This PR bumps Sentry SDK to 4.3.0 and migrates crash logging code to Kotlin in order to achieve more straightforward integration with clients.

Resolves #50

[oguzkocer]

@wzieba I've left some comments, especially about tooling. However, I am reluctant to leave feedback about the actual implementation, because I can see that a lot of these changes are due to Java - Kotlin conversion.

I like that you're adding Kotlin support, but I think it'd have been much better to do that in a separate PR with no functional changes to the code. That way, it'd have been much easier to focus on your changes.

Furthermore, it looks like you forced pushed this branch a couple times after opening the PR. That's something I strongly discourage mainly because we can't easily see what changed since it's opened. (reviewed) In this case I haven't had started reviewing, so no harm done, but please consider adding new commits if there is anything you need to change.

On the topic of commits, this whole PR is just a single commit which makes it difficult to see how the PR has evolved. If for example you have added commits for the auto Kotlin conversion, it'd have been much easier to ignore them. We have several p2 posts that talks about writing good commit messages, it may be valuable to visit them.

---

I think some of this might be more than you asked for. I am sorry to have brought up several non-implementation topics, but I still hope that you'll find them useful.

On a personal note, I am wrapping up a very high priority project and have very limited availability for a few days. I am doing my best to carve out some time, but to speed things along, please feel free to work with other product developers for the implementation details. I can still get involved on the high level changes, but may not be able to do another line by line review until next week.

[wzieba]

Thank you a lot for a review @oguzkocer , let me explain my reasoning on high level things you mention:

I like that you're adding Kotlin support, but I think it'd have been much better to do that in a separate PR with no functional changes to the code

Sure thing, I'll do this in separate PR

Furthermore, it looks like you forced pushed this branch a couple times after opening the PR. That's something I strongly discourage mainly because we can't easily see what changed since it's opened. (reviewed)

I'm happy that no harm was done, changes I forced push were a minor ones like adding next line at the end of a file or changing lib version which I eventually reverted.

I amend commit because merge strategy used in this and other projects is merge commit which will end up with merging all of the commits in PR into target branch develop. I think many non important commits like "fixed lint" can shadow important commits and create messy git log, especially on target branch with a lot of commits.

But that's just an opinion. I understand the strategy with not amending commits and using merge commit works here and other projects so I'm ok with stop amending the commits.

On the topic of commits, this whole PR is just a single commit which makes it difficult to see how the PR has evolved.

True, thanks for pointing it out. In this case it was difficult not to do this to not block myself with e.g. Kotlin support but I understand your point. I'll split this PR into smaller ones and try to keep commit history as a more divided one.

I think some of this might be more than you asked for. I am sorry to have brought up several non-implementation topics, but I still hope that you'll find them useful.

Yes, I find them very useful. Thank you a lot for that, it'll definitely help me to work better not only on this, but other projects too.

On a personal note, I am wrapping up a very high priority project and have very limited availability for a few days. (...) I can still get involved on the high level changes.

Sure, if it's ok I'll assign to you some reviews of tools updates, feel free to review them when you have time. I'll make sure that those won't block feature parts I'm working on. Thanks for heads up!

[oguzkocer]

I amend commit because merge strategy used in this and other projects is merge commit which will end up with merging all of the commits in PR into target branch develop. I think many non important commits like "fixed lint" can shadow important commits and create messy git log, especially on target branch with a lot of commits.

I understand where you're coming from. I amend, rebase, reword commits very often, but I just do it before I open the PR. I like to review my own PR before creating it, which seems to help with these kinds of issues. Anyway, it's good to limit those extra commits as much as we can, but it shouldn't overshadow more important principles, so I suggest not worrying too much about them.

Sure, if it's ok I'll assign to you some reviews of tools updates, feel free to review them when you have time. I'll make sure that those won't block feature parts I'm working on. Thanks for heads up!

Of course and I'll do my best to get to them as quickly as I can!

[wzieba]

This will be eventually merged into the client's manifest:

This disables automated Sentry initialisation. We do this in CrashLogging#start with dsn provided by the client.
Reference: https://docs.sentry.io/platforms/android/migration/#manual-installation

[wzieba]

I've decided to propose rewriting CrashLogging to Kotlin to have a better use of Sentry's "Unified API"

So e.g. instead of

SentryEvent event = new SentryEvent();
Message message = new Message();
message = throwable.message;
event.message = message;
event.level = SentryLevel.ERROR;
event.setExtras(data);
Sentry.captureEvent(event);

val event = SentryEvent().apply {
    message = Message().apply {
        message = throwable.message
    }
    level = SentryLevel.ERROR
    setExtras(data)
}
Sentry.captureEvent(event)

the same applies to e.g. User. IMO it's quite more readable.

Also 2 of 3 clients of Tracks are mostly in Kotlin so it's easier to assert nullability and mutability

[wzieba]

The idea for this "service" is to have some mechanism for frequent end-to-end check of the Sentry integration. It's not a test, even if it's in androidTest directory.

This way we're to run a lot of tests on test Sentry project. I believe this will be helpful also for future work.

Demo

Kapture.2021-03-15.at.21.33.48.mp4

[oguzkocer]

I am not sure how I feel about this. I can understand the idea, but it feels odd to me to do it like this. Since I can't provide a meaningful counterpoint not to do it, I guess I'll just leave a couple suggestions in case we keep it:

• The Service postfix is a reserved keyword in Android and it really doesn't describe what this class does. So, I suggest using a more descriptive without the Service in it.
• We should document how to use this class and explain why it's in the project in the first place.
• waitForEventToBeSent doesn't seem to do anything. I understand it's communicating that the event will take time to appear, but I don't think it should actually cause a delayed finish unless we are taking an action after the delay. We should just have the delay communicated in the documentation I mentioned.
• We shouldn't run these tests in CI. We have a quota in Sentry and they really don't provide any value unless we are actively testing the Sentry integration.

---

While typing the suggestions, I realized why I feel uneasy about this. I think this makes a lot more sense as an example app instead of an instrumentation test. We don't verify anything in the tests, so they provide no value as an automated test. We could potentially add a way to check if the Sentry event is there (using their API), but I don't see enough value in that.

Instead, I think we can have a very simple app within this repo that simply adds Tracks library to it and have buttons in the UI that calls these different methods. I can definitely get behind something like that.

[wzieba]

The Service postfix is a reserved keyword in Android

Sure, I'll rename that to not cause any confusion

We should document how to use this class and explain why it's in the project in the first place.

What kind of documentation do you think of? More live Javadoc like, or some instructions on P2 or README on Github? What would suit here the best?

waitForEventToBeSent doesn't seem to do anything (...) We should just have the delay communicated in the documentation I mentioned.

waitForEventToBeSent does something and it's locking a main thread to not allow the test app to close immediately after test execution. Otherwise, the events would not be sent because test app will close before HTTP request from Sentry will have a chance to be sent. Mentioning this delay in documentation won't change anything.

We shouldn't run these tests in CI

And we don't do this, do we? Current CI task is ./gradlew --stacktrace testRelease and it does not have any dependencies on connectedAndroidTest which runs instrumented tests on Android. If we would run this test on CI, even in this build it would fail as there's no setup for the emulator to run on CI.

---

I think this makes a lot more sense as an example app instead of an instrumentation test.

I get your point that this class is not a test and I don't want to make this a test.

I'd opt against having sample app for testing Sentry. Sample app causes a lot of additional overhead like keeping dependencies up to date or writing UI and platform code. Also, typical testing flow will require more effort with rebuilding the app and manually pressing UI buttons. I think this mechanism I've proposed is faster to work with and easier to maintain. To test if everything works fine after changes, developers will only need to run methods from one class.

Do those additional arguments convince you to leave this mechanism (I intentionally avoid word test)?

[malinajirka]

What kind of documentation do you think of? More live Javadoc like, or some instructions on P2 or README on Github? What would suit here the best?

I personally think a regular comment (javadoc) should be enough.

waitForEventToBeSent does something and it's locking a main thread to not allow the test app to close immediately after test execution. Otherwise, the events would not be sent because test app will close before HTTP request from Sentry will have a chance to be sent. Mentioning this delay in documentation won't change anything.

Thanks for the clarification! Could you please elaborate on this part "Mentioning this delay in documentation won't change anything.". I personally feel like explaining in a comment why the delay is needed might help other devs/future-us. Wdyt?

I get your point that this class is not a test and I don't want to make this a test.

Could you please elaborate a bit more on this? I feel like every class which is annotated with "RunWith" is considered as a test by Gradle. For example, if you run "./gradlew cAT" it'll run even methods annotated with @test in this class, right? I completely agree with Oguz that keeping this as a test might bite us in the future. Having said that, I understand your point of view, that having this "tool" might come handy.
We might want to consider adding "@ignore" annotation and a comment explaining that this test is disabled so it's not run automatically because it's a manual test that needs the developer to manually check Sentry that everything works as expected. It's far from optimal, I know. Wdyt?

And we don't do this, do we? Current CI task is ./gradlew --stacktrace testRelease and it does not have any dependencies on connectedAndroidTest which runs instrumented tests on Android. If we would run this test on CI, even in this build it would fail as there's no setup for the emulator to run on CI.

This is the current state, but we might want to start running connected tests on a CI in the future. I'm worried no-one will realize they need to manually suppress this test from running. Wdyt?

---

When it comes to the discussion "app" vs "test". I personally don't mind either as long as it's documented - javadoc or readme(for sample app). I think the additional overhead with setting up and maintaining the app will be very low. For me one nice advantage of the app is that anyone who opens the project can see it. As oppose to the test - IMO very few people go through the test suite to find if it contains something interesting. On the other hand, I agree that nice thing about the test is that it runs automatically and is quick - you don't need to manually click on buttons.

You both made some good points and no decision is final and we can always iterate on it if we find that it doesn't work for us.

[wzieba]

Could you please elaborate on this part "Mentioning this delay in documentation won't change anything."

What I meant by writing this is that this method is not about "communicating that the event will take time to appear". It's a vital part of sending a message as tests run on the test app (I mean: created for tests) and for every test execution we've got a new run of the app. After each test is finished app is closed so without locking the main thread (=pausing test for a few seconds) Sentry won't have time to be sent.

Rephrasing this statement to be more descriptive: "Knowing about delay won't resolve any issue here. It's not about informing developer about something - this pause is a must". So this is what I meant: just knowing that there's a delay won't happen that this will work.

every class which is annotated with "RunWith" is considered as a test by Gradle

You're right, by saying "this class is not a test" I meant that it's not a test in a formal way, not that this is not a test by Gradle standards.

We might want to consider adding "@ignore" annotation and a comment explaining that this test is disabled

That's a good idea, I'm starting to drifting into a sample app solution though (more on that in the latter part of this comment) so it might not be needed.

---

Thank you for your input on that ""service"" idea. I can see that this idea raises a lot of concerns so eventually, it might not be a good fit.

Although I'm not in love with the idea of the sample app (I just have aversion to buttons :) ) I'll prepare one to avoid any confusion now and in the future.

[wzieba]

If it's okay for you, I've created an issue #69 and I will work on creating a sample app on a separate pull request to not shadow changes regarding Sentry bump.

I think it's okay to leave two options: automated sending of events using Android's instrumentation tests framework and a sample app.

We might want to consider adding "@ignore" annotation and a comment explaining that this test is disabled

After reconsidering I'd like to suggest not adding @Ignore to this "test". A scenario when it'll run alongside "real instrumentation tests" is not a case for now (and I don't suspect it'll change soon) and if it will happen - adding @Ignore is a little effort. Is that okay or you'd prefer to add @Ignore now?

I use this method quite often and it'd be a bummer to have to add and remove @Ignore on every interaction with git.

[malinajirka]

Rephrasing this statement to be more descriptive: "Knowing about delay won't resolve any issue here. It's not about informing developer about something - this pause is a must". So this is what I meant: just knowing that there's a delay won't happen that this will work.

I should have read the comments more carefully 😞 . Thanks for the explanation!

I think it's okay to leave two options: automated sending of events using Android's instrumentation tests framework and a sample app.
After reconsidering I'd like to suggest not adding @ignore to this "test". A scenario when it'll run alongside "real instrumentation tests" is not a case for now (and I don't suspect it'll change soon) and if it will happen - adding @ignore is a little effort. Is that okay or you'd prefer to add @ignore now?
I use this method quite often and it'd be a bummer to have to add and remove @ignore on every interaction with git.

I'm just a bit worried that whoever starts running tests on a CI probably won't even know that they should prevent these specific tests from running. I'm also worried about the precedens - if we use it here, it'll be much easier to use the same approach again in other repos.
The fact that it's a bit cumbersome is imo the price for using a tool which is not meant to be used for "tests" which require a human interaction. However, I'm wondering if we could perhaps have some setting in gradle.properties instead of using @ignore. Wdyt @wzieba @oguzkocer ?

[wzieba]

The fact that it's a bit cumbersome is imo the price for using a tool which is not meant to be used for "tests" which require a human interaction

I agree with this and previous points - still, I believe it's a very helpful tool to test this library e2e.

I think I've found a compromise (IMO quite neat!) in 8d74cf2 . Current behavior:

• Running ./gradlew connectedAndroidTest results with No tests found
• Running SendEventsToSentry from Android Studio will run this test as we explicitly ask to run this class

WDYT? Does it work for you this way?

[malinajirka]

TIL about 'notClass' argument! LGTM ;)

[wzieba]

TIL about 'notClass' argument!

Me too! From this article if you're interested diving into more details: https://medium.com/stepstone-tech/exploring-androidjunitrunner-filtering-options-df26d30b4f60

[wzieba]

@oguzkocer @jkmassel just to confirm - this PR is ready for re-review. Please take a look when you have time. Thanks!

[wzieba]

Here's also change I'd like to border. Previously we didn't send event if library was in debug variant but I'd like to leave this decision only to client. There are cases when we want to send event in debug (testing)

[wzieba]

This map has to be a mutable one, otherwise, we'll have a crash in runtime as Sentry SDK tries to edit an immutable map (which is a default in Kotlin).

I've reported an issue on Sentry's Github, more details and reproduction repository is there if you're interested: getsentry/sentry-java#1357

[malinajirka]

Thanks for the changes @wzieba! The tests look definitely better now ;). I've left some questions but none of them is blocking - I'm just curious about your thoughts.

[malinajirka]

I'm wondering if we really need to introduce this interface. Can't we simply work with SentryErrorTrackerProxyImpl?

1. The interface is dependant on Sentry anyway, so it won't help us if we ever decide to replace Sentry with another library.
2. Mockito allows us to mock even classes so this interface imo won't bring any value when it comes to testing.
3. Kotlin compiler is smart, so it won't re-compile files which don't need to be re-compiled (as oppose to Java compiler which afaik always recompiles all affected dependencies).

All in all I'm wondering if we are not just introducing an unnecessary boilerplate. Wdyt?

[wzieba]

It was introduced to make testing easier (creating a fake object). As I've followed your suggestion in #51 (comment), I've refactored tests to use mocks so eventually I've removed this interface.

Answering your questions:

1. Totally right, this interface didn't abstract idea of ErrorTracker in general
2. If we use Mockito - that's true. For Fakes, we wouldn't be able to write test
3. Are you referring to compile avoidance? This was not a reason for writing this interface, as Kotlin doesn't support it. Editing SentryErrorTrackerProxy**Impl** would cause recompilation of CrashLogging in Kotlin (wouldn't if it would be Java). Or do you refer to the incremental compilation? If so, I'm not sure if I understand how it works: maybe the internal implementation of SentryErrorTrackerProxy**Impl** changes won't cause recompilation of CrashLogging?

[malinajirka]

Are you referring to compile avoidance? This was not a reason for writing this interface, as Kotlin doesn't support it. Editing SentryErrorTrackerProxyImpl would cause recompilation of CrashLogging in Kotlin (wouldn't if it would be Java). Or do you refer to the incremental compilation? If so, I'm not sure if I understand how it works: maybe the internal implementation of SentryErrorTrackerProxyImpl changes won't cause recompilation of CrashLogging?

I don't have a deep understanding of compilers. I just remember a talk where they compared java vs kotlin compiler. One of the points was that introducing interfaces in java to speed up incremental compilation can help in Java but is not necessary in Kotlin, since kotlin compiler is smart enough.

This was not a reason for writing this interface, as Kotlin doesn't support it.

Hmm, I didn't know that Kotlin doesn't support it. I've read this article and it seems that it supports it. Or do you think it's different for java+kotlin projects? I'm just curious, feel free to ignore this comment if you don't know from the top of your head.

[wzieba]

Setting BuildConfig.DEBUG, which I changed from in 58e2187, did not make sense as I forgot that this is not project-included library, so BuildType of project won't match BuildType of library. We release release variant on Jitpack so we would always had setDebug(false) in clients.

I've updated CrashLoggingDataProvider to provide wether client wants to see logs or not.

[malinajirka]

Ohh, great catch! 👏

[malinajirka]

Thanks for all the improvements @wzieba!

I think this PR is almost ready to be merged. One last thing I noticed is that the notClass argument for JUnit takes affect even when I run the tests from Android Studio. It seems that AS runs the following command

adb shell am instrument -w -m -e notClass com.automattic.android.tracks.crashlogging.SendEventsToSentry   
-e debug false -e class 'com.automattic.android.tracks.crashlogging.SendEventsToSentry'
 com.automattic.android.tracks.test/androidx.test.runner.AndroidJUnitRunner

which results in a crash since I'm trying to run SendEventsToSentry tests but at the same time telling it that these tests should be ignored. I'm on Android Studio 4.1.2. Can you think of a reason why it doesn't behave this way with your setup?

[wzieba]

Thank you @malinajirka for this check! As we discussed on DMs, I had to have an old run configuration when I was checking how it works for me. Good point with noticing that, it could cause confusion for future users. In aa536f4 I:

• Added run configuration with Include Extra Params from Gradle build file unchecked
• Added small "troubleshooting" part into comment in SendEventsToSentry about making sure to uncheck Include Extra Params from Gradle build file

[wzieba]

For the second commit since your last review (6a4998a) @malinajirka I've added a new comment in this thread: #51 (comment)

[malinajirka]

All works as expected now. Thanks @wzieba !