[http #96] Curl logger

[http #96] Curl logger NOTE: full credit goes to Matthias Linder for this contribution. See: http://codereview.appspot.com/6305094/

[rmistry, 2012-07-31 11:19:07]

http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... File google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java (right): http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:638: // TODO(mlinder): Fix Issue 129 -- HttpRequest should not log the Authorization-header in mlinder -> rmistry Actually we can just remove this TODO since a bug is already filed. http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:676: * @deprecated (scheduled to be removed in 1.12) Why deprecate this? or we can deprecate this and make the below new method public? http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... File google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java (right): http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:422: * Defaults to {@code true}. I wonder if this should be false by default. What does android currently do? I looked here: http://developer.android.com/reference/android/net/http/AndroidHttpClient.html but do not see a default value.

[yanivi, 2012-07-31 11:48:38]

http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... File google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java (right): http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:638: // TODO(mlinder): Fix Issue 129 -- HttpRequest should not log the Authorization-header in On 2012/07/31 11:19:07, rmistry wrote: > mlinder -> rmistry > Actually we can just remove this TODO since a bug is already filed. TODO(yanivi): will do when I get to my computer at work. http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:676: * @deprecated (scheduled to be removed in 1.12) On 2012/07/31 11:19:07, rmistry wrote: > Why deprecate this? or we can deprecate this and make the below new method > public? Why deprecate: no one is using it and it exposes internal implementation details. If we ever want to change it like we are doing now, we are forced to deprecate it and add another method. As we get closer to GA, we won't have the luxury to remove methods in the future. So we need to think very carefully about what we expose. So let me turn the question around: why expose it? I'm okay with it only if you have a valuable use case in mind. http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... File google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java (right): http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:422: * Defaults to {@code true}. On 2012/07/31 11:19:07, rmistry wrote: > I wonder if this should be false by default. What does android currently do? > I looked here: > http://developer.android.com/reference/android/net/http/AndroidHttpClient.html > but do not see a default value. As I recall It is always logged on Android. There is no way to turn it off.

[yanivi, 2012-07-31 13:17:40]

http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... File google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java (right): http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:638: // TODO(mlinder): Fix Issue 129 -- HttpRequest should not log the Authorization-header in On 2012/07/31 11:48:38, yanivi wrote: > On 2012/07/31 11:19:07, rmistry wrote: > > mlinder -> rmistry > > Actually we can just remove this TODO since a bug is already filed. > > TODO(yanivi): will do when I get to my computer at work. Done. http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... File google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java (right): http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:422: * Defaults to {@code true}. On 2012/07/31 11:48:38, yanivi wrote: > On 2012/07/31 11:19:07, rmistry wrote: > > I wonder if this should be false by default. What does android currently do? > > I looked here: > > http://developer.android.com/reference/android/net/http/AndroidHttpClient.html > > but do not see a default value. > > As I recall It is always logged on Android. There is no way to turn it off. I take it back. Looking at the implementation of AndroidHttpClient it looks like it is off by default. But keep in mind that logging is off by default in our library also because we are using CONFIG as the logging level, whereas by default only minimum WARNING level is logged. So it is only if you've set your logging level to CONFIG that the logs show. So I'm okay with true for setCurlLoggingEnabled. Does that make sense?

[rmistry, 2012-07-31 14:29:13]

LGTM http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... File google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java (right): http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java:676: * @deprecated (scheduled to be removed in 1.12) On 2012/07/31 11:48:38, yanivi wrote: > On 2012/07/31 11:19:07, rmistry wrote: > > Why deprecate this? or we can deprecate this and make the below new method > > public? > > Why deprecate: no one is using it and it exposes internal implementation > details. If we ever want to change it like we are doing now, we are forced to > deprecate it and add another method. As we get closer to GA, we won't have the > luxury to remove methods in the future. So we need to think very carefully > about what we expose. > > So let me turn the question around: why expose it? I'm okay with it only if you > have a valuable use case in mind. I see only in package users and no internal references using it. Agreed, lets not expose it. http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... File google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java (right): http://codereview.appspot.com/6447066/diff/1/google-http-client/src/main/java... google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java:422: * Defaults to {@code true}. On 2012/07/31 13:17:40, yanivi wrote: > On 2012/07/31 11:48:38, yanivi wrote: > > On 2012/07/31 11:19:07, rmistry wrote: > > > I wonder if this should be false by default. What does android currently do? > > > I looked here: > > > > http://developer.android.com/reference/android/net/http/AndroidHttpClient.html > > > but do not see a default value. > > > > As I recall It is always logged on Android. There is no way to turn it off. > > I take it back. Looking at the implementation of AndroidHttpClient it looks > like it is off by default. > > But keep in mind that logging is off by default in our library also because we > are using CONFIG as the logging level, whereas by default only minimum WARNING > level is logged. So it is only if you've set your logging level to CONFIG that > the logs show. So I'm okay with true for setCurlLoggingEnabled. > > Does that make sense? Yes, thats fine.