commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
1.02k stars 1.23k forks source link

Modify the "Send Logs" feature #1489

Closed misaochan closed 6 years ago

misaochan commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

nicolas-raoul commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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:

@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 commented 6 years ago

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 commented 6 years ago

Basically, the scope of this task is:

nicolas-raoul commented 6 years ago

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 commented 6 years ago
misaochan commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

misaochan commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Please do, thanks!

sivaraam commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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:

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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