[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.