Collect: Clean up printing stack trace call when exception occured

[nribeka]

Exception should be logged by calling: Log.e(String, String, Throwable) or some others Log's method.

In the codebase, we have a lot of call to e.printStackTrace(). Those calls should be replaced by the appropriate Log.x methods.

[nribeka]

@lognaturel or @yanokwa, do you have preference to do this as separate PR per files or separate commit per file? This could change a lot of files.

[lognaturel]

@nribeka We've struggled with this kind of change. What's your recommendation?

[nribeka]

Initial search showed 158 occurrence of printStackTrace and it's all over the place. Maybe at the minimum we go with separate commits per file? Separating per PR seems to be too much.

[nribeka]

To whoever going to do this, don't just do find and replace in the code. You need to read the context of the stacktrace and decide which Log's method will be appropriate. I don't think all should become a Log.e calls.

[lognaturel]

I was hoping you'd have some kind of magic solution we hadn't thought of! 😉 We need a process because there are a lot of these kinds of issues. A single PR with a commit per file seems ok to me.

I think @batkinson had some opinions on this kind of situation.

[nribeka]

And folks who will do this can propose a simplified catch blocks too when we're doing the same thing for each type of exception. One example is the following codes:

Since every catch block treat the exception the same way, there's no benefits of separating it into different blocks.

[Midhun07]

Can i work on this issue? @nribeka i assume that what you propose in the last message is to combine catch blocks wherever possible like the one you have shown?

[parthuest]

@Midhun07 I am working on this issue.

[shobhitagarwal1612]

@Midhun07 The real issue is that printStackTrace method has been used everywhere for catching the try blocks. printStaceTrace prints the stack trace in Android Monitor as an error because of the exception caused.

At some places eg. GoogleDriveActivity, this stack trace is not a critical exception so it unneccessarily clutters the error logs.

So instead of calling e.printStackTrace(), relevant Log method Log.d, Log.e, Log.v, Log.w, Log.i should be used. This would help filtering the logs so that relevant logs are not missed.
Also as suggested by @nribeka, multiple catch blocks which are handling the exception exactly the same way should be squashed into one catch block.

[shobhitagarwal1612]

@yanokwa @nribeka I stumbled across a great library by Jake Wharton, Timber. A logger with a small, extensible API which provides utility on top of Android's normal Log class. http://jakewharton.github.io/timber/

Pros :

• Development - Logging to logcat
• Production - Crash reporting to Crashlytics

Timber and Crashlytics can be used along with AppCrashHandler which elegently handles unhandled exceptions by restarting the app instead of showing the system dialogs displaying an error has occurred.
Read the article here link
Also addresses #655

[rijuldhir]

Is somebody already assigned on the issue? I am willing to help, we can have only 1 catch block for exceptions wherever necessary.

[parthuest]

I have tried my best to justify the task. @nribeka can you please check it in my profile and let me know if I have done something wrong.

[lognaturel]

@parthuest please submit a pull request as described in the contribution guide.

[lognaturel]

663 might actually be a nicer way to address this since it could let us address better logging in both development and production.

[Ishmeetsingh97]

Hi, @lognaturel Can i work on this issue ? I am familiar with timber.

[lognaturel]

Let's break this down to make sure we get it right. It would be good to get a first pull request JUST removing all calls on e.printStackTrace. If you'd like to do that, go ahead and comment and then take it on.

[Ishmeetsingh97]

Hi, @lognaturel Can i work on it?

[anudeepti2004]

@lognaturel : I can work on it. So basically there will be 2 PR's 1 to just remove all e.printStackTrace and 2nd one to have commits with the appropriate Log.x methods.

[lognaturel]

@Ishmeetsingh97 Since you know Timber, how about you work on adding a Tree for Firebase Crash and verify that it works.

@anudeepti2004 That's right. Go ahead and do just the removal one. Then once we have @Ishmeetsingh97's Tree ready you can go straight to using Timber methods to do the logging right.

[Ishmeetsingh97]

Okay, @lognaturel I will give this a try and let you know my developments.

[anudeepti2004]

@lognaturel : Got it. Thanks

[lognaturel]

@anudeepti2004 Once #717 is merged, if you would like to get started on replacing the log calls with Timber calls, please go ahead. Once #717 is merged you'll be able to update your local master branch and make a branch off of it. Then you can add the gradle dependency and always use the DebugTree for your verification.

This task may look simple but it's going to require carefully thinking about every existing log call and every exception catch block (and so is a good way to get to know the codebase)! You will want to make sure you are always using the appropriate logging method. Whenever there is a caught exception that is not handled, you will want to make sure that it is logged and as @nribeka pointed out above, you will find several opportunities to combine redundant catch blocks. If you have any questions as you go, please ask.

This will be an important step in being able to react more quickly to problems so thank you all for your help!

[anudeepti2004]

@lognaturel : I totally agree. This would be a great way for me to get familar with the code base. I also have a suggestion (it is totally ok if you dont agree with the suggestion :)) , there are many methods in the code that do not have any Javadoc. Since I will be going through the code base anyway for this issue, will it be ok if I update the methods with missing Javadocs with the appropriate documentation?

[lognaturel]

@anudeepti2004 It is a good idea and we definitely do need more code documentation in the long run but we should do that separately. This is already going to be a big pull request and we're less likely to introduce errors by doing one thing at a time! Accurately documenting the code will take a lot of effort and I think it will be best done as part of refactors of the different classes.

[Ishmeetsingh97]

Hi, @lognaturel I am not very similar with firebase. Can you please brief me on the issue.

[anudeepti2004]

@Ishmeetsingh97 : Have you made any progress on this issue? I have made some progress regarding the Timber logging and inorder to test them. I did the config changes and added a firebase tree. If you are not working on this issue anymore please let me know so that I can commit those changes.

[lognaturel]

That's great, @anudeepti2004. We merged in some big changes for the upcoming release -- I hope those weren't too disruptive for you. You'll want to make sure that you pull from master to get the latest. We're in feature freeze between now and Sunday so it's a great time to do this kind of across-the-codebase change.

@Ishmeetsingh97, since @anudeepti2004 knows how to do the Firebase Tree, how about you work on another issue? We've recently filed some other good ones!

[anudeepti2004]

Thanks. :)
I making some progress on the across-the-codebase change. Since there is a code freeze till Sunday. Shall I commit the changes regarding the Firebase tree and config changes (adding timber dependencies) on Monday?

[lognaturel]

@anudeepti2004 You can submit the pull request any time you want. Earlier means there's more time for review! Ideally it would be the first thing to get merged for the April release (Monday is the start of that release cycle).

[lognaturel]

@anudeepti2004 I'm going to start merging in some of the open pull requests. I'll try to avoid any that make too many logging-related changes so that you can merge the changes in without too much trouble.

[anudeepti2004]

Sure. Thanks. I apologize for the late response. I was a little busy with GSOC proposal and school work.

[lognaturel]

@anudeepti2004 Don't hesitate to submit a number of smaller pull requests if you would like. It may be easier to merge in incremental progress and avoid too many merge conflicts.

[anudeepti2004]

@lognaturel : I made a bunch of changes. I am trying to test the changes (which is not proving to be easy :)). Since I changed the google-services.json file to my Firebase account Andriod studio complains that it is unable to find field global_tracker. I am trying to resolve that (thats why I was delayed committing the changes). I want to be sure the changes are working before I do a pull request.

[lognaturel]

@anudeepti2004 How about submitting a pull request just with the Firebase Crash Tree as a first pass. I can verify that it works with the Firebase Crash account we use in production. Then you can just do your verification with the Log tree as you make the switch from Log to Tree.

[anudeepti2004]

@lognaturel : That will be great. Let me submit a pull request

[lognaturel]

Largely addressed by #821