-
Notifications
You must be signed in to change notification settings - Fork 749
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
Potential debug logging even when DEBUG is False #452
Comments
Thanks for the report! This is actually working as intended, though the behavior may be surprising. The log is logging details about the HTTP response; the default behavior is to do this only when there are slow responses, because those may be a sign of a client/server issue or misuse of Volley; we feel that it is on balance more helpful to enable this by default than require a special setting to do so, so that if a user/developer complains about slow app behavior, this information is readily available. The "|| DEBUG" is to enable this logging on all requests, not just slow ones. So we wouldn't want to change this to "&&". If you really don't want this logging to ever happen, one simple solution that wouldn't require library changes would be to add a Proguard config to |
If this is working as intended, then perhaps is there a way to remove the surprise for developers who are trying to do the right thing when they attempt to disable debug logging in production builds? Is there a good place to clarify this in the documentation? From what I've seen with published apps, it might be asking too much for most developers to know that you need to add that Proguard config to disable all of the debug messages. There is the potential to leak sensitive data to the log since the entire request string is logged, not just information about the response. There are apps in the PlayStore that use GET requests with access tokens, IDs, etc. and these do end up in the logs. Somehow it should be easy for developers to discover how to disable ALL debug-level logging for a library. |
Fair points - I'm raising them internally to decide on next steps. |
OK. Agreed that logging URLs shouldn't be done by default. I don't feel that the slow request logging is providing enough value to be worth keeping if it means having to add an API to opt into it. And I don't think it's useful to log without the URL or worth going down the road of trying to figure out other identifying information to log. Better to just let applications apply their own monitoring however they see fit, with their own thresholds for what is "slow". And probably using a more structured mechanism than a log line. Will leave concrete suggestions on the PR, but at a high level I think we should just log this line if VolleyLog.DEBUG is true and remove the concept of "slow logging" altogether. |
I agree. When developing an app, "slow" might mean different things depending on what is expected and the log message does include the response time. |
According to the internal comments and other usage, this message should only be logged when DEBUG is True. The log entry can potentially contain sensitive data in the case where authentication tokens are part of a GET request and so it should be possible for the user of Volley to disable logging by setting DEBUG to False even in the case where the network request is "slow." Fixes #452
Thanks. If this is needed before the next release, see these instructions for how to depend on a SNAPSHOT build. We aim to keep our master branch stable. |
core/src/main/java/com/android/volley/toolbox/NetworkUtility.java logs the message request when the network response is "slow" even when DEBUG is turned off since the conditional statement uses || rather than && when checking whether the message should be logged.
The text was updated successfully, but these errors were encountered: