Skip to content

Modify the "Send Logs" feature #1489

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

Closed
misaochan opened this issue May 3, 2018 · 23 comments · Fixed by #1916
Closed

Modify the "Send Logs" feature #1489

misaochan opened this issue May 3, 2018 · 23 comments · Fixed by #1916
Assignees

Comments

@misaochan
Copy link
Member

misaochan commented May 3, 2018

The "send logs" feature in its current state (2.7.1) is not very useful. As mentioned at #1485 , there are two issues with it:

  1. The buffer size seems to be very small, we get relatively few lines of logs compared to what Android Studio etc can store. We likely need to increase this.
  2. We are getting a lot of unrelated logs from system etc. We need to filter our app's logs out if possible. @tanvidadu has suggested a method for this:

To filter out logcats https://stackoverflow.com/questions/17448912/using-grep-command-to-filter-logcat-in-android?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa this might help.

@maskaravivek
Copy link
Member

I just came across https://github.com/hypertrack/hyperlog-android

Has anyone used it or have an opinion on it. It looks pretty cool and easy to setup.

@misaochan
Copy link
Member Author

Copied from #1683 - In its current state, "send logs" is not very useful - the logs are not filtered for our app, and some logs that we receive don't contain anything from our app even after filtering. We need to investigate why this is happening and modify the feature (with filters) so that we can actually get useful logs from users.

@maskaravivek
Copy link
Member

As I mentioned above, we could consider Hyperlog for this purpose. It will keep storing the logs in a local file and when the user clicks on send logs we can attach that file in the email and send it.

  • It will give us the filtered logs for our application
  • The number of logs to be stored can be controlled by us
  • Its open source and looks to be quite easy to setup.

@nicolas-raoul
Copy link
Member

https://security.stackexchange.com/a/190549/634 supports the idea of writing logs to a dedicated local file.
We could make the debug app write logs to both this file and logcat.

@maskaravivek
Copy link
Member

IMO, for debug builds we already have logcat so writing to local file might not be very useful but for release build we could write just to local file. This file can then be attached while sending a mail. Am curious, isn't this what we already do?

@maskaravivek
Copy link
Member

I will start working on this soon. It would be nice to gather opinions from others about what logging/crash monitoring framework we should be using.

Here's what I am considering:

  • Hyperlog for storing logs and pushing them to a remote location
  • Implementing our own file logging tree
  • Use Fabric's crashlytics for error logging
  • Use sentry for logging and error reporting

@dbrant It would be great if you could provide us with some insights on how we should go about it so that we do not violate any of the privacy policies.

@misaochan
Copy link
Member Author

misaochan commented Aug 16, 2018

IMO, for debug builds we already have logcat so writing to local file might not be very useful but for release build we could write just to local file. This file can then be attached while sending a mail. Am curious, isn't this what we already do?

@maskaravivek Correct, I think, but for some reason none of the usual logs are showing up at all. You can take a look at the logs submitted at https://groups.google.com/forum/#!forum/commons-app-android-private to see (I believe you have access, right?), or test the "Send Logs" feature on the app and see.

For filtering and storage, hyperlog looks good to me but it would be best to wait for others to comment too.

@misaochan
Copy link
Member Author

misaochan commented Aug 16, 2018

Basically, the scope of this task is:

  • Figure out why the usual logs from our app are not being shown (specifically, in Release builds)
  • Perform filtering for our app's logs (or use hyperlog if it automatically does that)
  • Make sure storage is able to handle relatively large log file sizes (e.g. it should be able to store everything from the start of an upload to the end)

@nicolas-raoul
Copy link
Member

Crashlytics would send logs to a third-party server, which is not acceptable for privacy reasons.
With sentry I guess we would need to set up a server, which is quite challenging too in terms of getting approval and maintaining security.
Hyperlog (locally used) sounds good :-)

@maskaravivek
Copy link
Member

maskaravivek commented Aug 16, 2018

  • Hyperlog will definitively solve one issue ie. the logs sent by the users would be more useful. I will go ahead and integrate it.

  • Sentry or any other crash reporting framework might be useful if we wish to proactively monitor issues/crashes without users sending us the logs. The one advantage that I see with sentry is that the data will reside with us. Someone should be surely able to help us host it. Can tools forge be used for hosting it?

  • Enhancing stacktraces from RxJava #1708 is tangentially related to this issue and will help us get meaningful stack traces for RxJava calls. We have recently started using RxJava at a lot of places and by default, logcat doesn't show the full stack trace. Am planning on using Traceur for fixing this.

@misaochan
Copy link
Member Author

Sounds good @maskaravivek .

One more thing - we should make sure to test the implementation on prodRelease, as that is the build variant that all users will br running. I noticed that the logs that I get via Android Studio on debug builds are different from those that I get on release builds for some reason. Not just the lack of http headers either (which was done on purpose), but other logs appear to be missing.

@maskaravivek
Copy link
Member

Sure, will test on prodRelease as well.

I guess we should start obfuscating the session cookies, make the logs a bit less verbose and enable it for prod variant as well. Logs won't be very useful if the request/response isn't logged.

@maskaravivek
Copy link
Member

To get started I have added Traceur (#1832).

@misaochan
Copy link
Member Author

Thanks @maskaravivek ! I agree completely with obfuscating the session cookies, it will make it much easier for people to paste logs as well, without having to ask @sivaraam or someone with custom visibility privileges to create a Phab paste for us. And once they are obfuscated, we can indeed enable http logging in release builds as well. (@nicolas-raoul correct me if I'm wrong?)

Would it be at all possible to do that as part of this task?

@maskaravivek
Copy link
Member

Yes, I would take it up as part of this task itself because getting request/response would be quite important.

Just to add, Tgr granted custom privileges to me as well and I can the members/collaborators of commons app to the list so that we can share logs without obfuscating the cookies. :)

@misaochan
Copy link
Member Author

Please do, thanks!

@sivaraam
Copy link
Member

Thanks @maskaravivek ! I agree completely with obfuscating the session cookies, it will make it much easier for people to paste logs as well, without having to ask @sivaraam or someone with custom visibility privileges to create a Phab paste for us

Just FYI, I too do not have permissions to create pastes with a custom policy. I was just pointing out the importance of keeping bad eyes away from logs that have not been obfuscated yet.

@maskaravivek
Copy link
Member

Have run into an issue while using HyperLog. Every usage of Log or Timber will have to be replaced by HyperLog.d.

To avoid this will go ahead and plant a FileLoggingTree to Timber.

@dbrant
Copy link
Collaborator

dbrant commented Aug 26, 2018

@maskaravivek @misaochan Sorry for the late reply!
I'm afraid I have very little experience with these third-party logging solutions. Whichever framework you choose, I would only stress that you restrict your transmitted logs to include only information that's absolutely essential, and make sure that all logs are thoroughly anonymized.

I generally like to encourage a greater emphasis on QA, continual dogfooding, and reducing complexity (i.e. refactoring existing code to use unified APIs), rather than adding further complexity just for the purpose of capturing more detailed logs in the field. Based on a quick glance at the current state of the Commons app, it looks like there's a rather high amount of complexity and dependencies (to the point of requiring multidex? 😮)

For example, for the purpose of displaying images, you seem to be using Fresco and Glide and Picasso in different places, literally all three major competing image libraries for Android. This introduces a huge amount of bloat, as well as a threefold increase in the surface area for bugs to occur. Not to derail this particular thread, but my earnest recommendation would be to take a step back, establish some best practices for the architecture of the product, and work towards refactoring all of the code to adhere to each of these best practices, one at a time.

For instance:

  • Decide on a single image library (e.g. Fresco) and use it throughout the app.
  • Decide whether to use RxAndroid or plain AsyncTasks, and use them consistently (preferably RxAndroid), but don't use both.
  • Stop using the long-deprecated org.mediawiki:api library, and use Retrofit (with RxAndroid) for your network calls.
  • Evaluate whether you really need all the third-party components and libraries that you're depending on. They are all potential sources of bugs. (For example fr.avianey.com.viewpagerindicator or in.yuvi:http.fluent, both of which have been abandoned for six years)
  • Try not to use third-party libraries that promise to "simplify" things, like a library that "simplifies" populating a RecyclerView, or a library that "simplifies" asking for runtime permissions. They are all sources of bugs, which are out of your control.
  • Explain these best practices to newcomer volunteers, and reject pull requests that increase complexity unnecessarily.

Once you do this, the bugs that seem puzzling today will become more and more obvious, or will simply disappear automatically. Sorry for the long-windedness (and apologies if I'm overstepping), and let me know if I can help in any way.

@maskaravivek
Copy link
Member

Thanks @dbrant for your insights. We really appreciate you taking out time to respond in such detail. Your observations are accurate and we admit that the the quality of code is not consistent throughout the app.

Personally, I didn't emphasise on few of the points(image library, SVG, 3rd party libraries) and let those pieces of code creep in our codebase. We will strive to follow better code practices to adhere to these points. IMO earlier we had very few contributors and we were always eager to accept any contribution if it fixed an issue or added a feature. With the number of active contributors we currently have, we could be a bit more stringent about following the best practices.

Also, we are in the process of adopting RxAndroid and Retrofit completely over the course of next few months. In the next 3-4 months we would be picking up a lot of tech debt items (#1683) and will hopefully get rid of some of the legacy code.

@dbrant Great to see that you have raised PRs to fix some of the issues mentioned above.

I was thinking of creating tasks for the rest of the things and the contributors can help us with of these things. Opinions? @misaochan @neslihanturan

@misaochan
Copy link
Member Author

misaochan commented Aug 27, 2018

@dbrant Thanks so much for the detailed advice! IMO you are absolutely right re: the unnecessarily complex web of dependencies, our use of legacy APIs, and our lack of best practices. I will create a new issue for this discussion, please feel free to join it at #1863 . :)

I do think it is still probably worth getting Send Logs working for the time being, as a complete architecture/dependency overhaul is likely to take a lot of resources and a fairly long time (I'm guessing we will not be able to get it sorted until at least mid 2019, and even that is dependent on getting our grant proposal accepted). And in the meantime, we would still need to handle urgent bugs that pop up. So I guess I would view Send Logs as the tourniquet, but the architecture/dependency overhaul as the long-term treatment plan. ;)

Re: anonymization - The logs will only be visible to people who have already signed a NDA with WMF, so I was under the impression that anonymization will not be as crucial in that case (although of course we should still try to restrict the information sent to that which is necessary). Am I mistaken in this?

@sivaraam
Copy link
Member

Re: anonymization - The logs will only be visible to people who have already signed a NDA with WMF, so I was under the impression that anonymization will not be as crucial in that case (although of course we should still try to restrict the information sent to that which is necessary). Am I mistaken in this?

I think the people who use the app aren't aware of this, are they? Also, regardless of whether the people are aware about NDA or not, everyone would be more confident to send data (including logs) when they are made aware that only the required data is collected and the reason for collecting particular data is clearly explained (just like how the Wikipedia app does for their usage data collection). I suppose a FAQ would be a nice to have, just like how Wikipedia app has one.

@misaochan
Copy link
Member Author

We can probably mention that briefly in the "Send logs" subtext, which the user will see before they tap the button to send them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants