log request bodies in logger middleware

[stve]

I've found it helpful to see the request body in addition to request headers when using the logging middleware. This change adds logging for the body.

While adding tests, I noticed some failures in env_test which I didn't address in this PR. Are you aware of these failures?

[mislav]

See my comment in #254. I'm in favor of optional logging of body, but not by default. Maybe introduce an options hash?

Also, when you add a new feature, changing old tests is not desireable. Adding new tests is the way to go.

[stve]

I can absolutely add an options hash and will modify as you've proposed - fully agree that it makes sense to be optional. I will also restore the tests.

Do you have any thoughts on logging response bodies? That could be helpful when ParseXml/Json,etc middleware are used to transform the body.

[mislav]

If the body is a string, just dump the string. If it was a Hash/Array (e.g. parsed by ParseXml/Json middleware) the logger middleware could dump that data in a prettified version, a la pp. Just a thought.

[stve]

I just added an options hash which can accept a boolean to log both request and response bodies:

Faraday.new do |b|
  b.response :logger, ::Logger.new(STDOUT), :bodies => true
end

Or a hash to be more fine-grained:

Faraday.new do |b|
  b.response :logger, ::Logger.new(STDOUT), :bodies => { :request => true, :response => false }
end

[mhenrixon]

This is looking mighty good to me, would love to have this possibility by default!

[jpmckinney]

+1 very useful for debugging

[stve]

@mislav is there anything else you'd like to see added here? I didn't squash the commits so that you could see that I reverted the one that I changed in the first commit, happy to do so if that is holding this up.

[Nerian]

+1 Very useful

[mislav]

This all looks good, but I'm not a huge fan of the :bodies => { :request => true, :response => false } options syntax. I don't have better ideas, though, so you can throw your opinions in here. One alternative is going flat:

{ :request_body => true,
  :response_body => false
}

But that makes turning both options on is awkward. So I guess the current syntax can stay.

Tips:

1. Don't check if an object is_a? String. Check if it responds to to_str and use the return value of that method instead.
2. Don't require "pp" unconditionally at runtime. Require it only if the object doesn't respond to pretty_inspect.
3. Do object.pretty_inspect in its own method of the middleware, named e.g. pretty_inspect(object). That way someone can subclass the logger middleware and augment that method.
4. assert_match %(:husband=>\"Barney\", :wife=>\"Betty\"), @io.string is dependent on the order of Hash keys which is not guaranteed in older vesions of Ruby.

Sorry for making you do so much work. Your contribution is appreciated! ;)

[mislav]

Oh, and to get rid of Travis failures, rebase your work to master and force push to your branch again. You can squash your commits or you don't have to (it's easy do to from my end as well). I've fixed most of the failures, with the exception of an occasional glitch due to test WEBrick server choking on the volume of the requests.

[stve]

@mislav no worries. I made changes based on your suggestions and rebased against master. I decided that the require 'pp' statement belonged inside #pretty_inspect so that if anyone subclasses and reimplements it, the don't naively require 'pp' by accident. Thanks for the suggestions.

[stve]

In terms of the options syntax, what are your thoughts on supporting both options:

{ :bodies => false } # log neither
{ :bodies => true } # log both
{ :request_body => true, :response_body => false } # mixed use

That avoids the nested hash.

[mattbeedle]

+1

[rsdcastro]

+1, please merge this change. Very useful and saved me when debugging API request errors with Google API Client.

[mtarnovan]

How about instead of just passing true/false we could also specify an array of response types that should be logged ? Something like this:
{ bodies: [:json, :xml, :html, :text] }

Also, I'm not so sure about using pretty_print here. After all, if you require such verbose logging, chances are you will want to log the request verbatim.

[mtarnovan]

@mislav any plans to merge this ?

[mislav]

Yep, I still think it's useful to merge this. I haven't changed my opinion. However, I haven't gotten around to merging it yet.

Are you impatient? Are you not able to easily copy the class from this pull request and vendor and use it in your project?

For me, merging is not just a matter of hitting the Merge Button™. I have to do a final review of the code, review the quality of tests, decide which Faraday branches does the feature belong to, does it keep backwards compatibility, add missing documentation for new features, etc.

[mtarnovan]

I want to use this in production, so I preffer to wait until this is properly tested, reviewed etc. I don't want to be pushy in any way, was just curious if you plan to merge it soon, or if I should clone and merge myself.

[franc]

+1

[DougPuchalski]

@spagalloco Thanks for this 👍

[righi]

+1 Is there anything I can do to help get this into master? Logging of post bodies is extremely helpful for debugging calls I'm making to RESTful services.

[ghost]

would love to see this merged. 👍

[dolzenko]

+1, very much desired for simpler debugging.

[hem-brahmbhatt]

Looking forward to this merge

[locochris]

+1

[vasanthela]

I can understand why one would delay merging a pull request in order to properly document and test it for a release as @mislav pointed out.

For those who would like to use this feature quickly, and not mess with merging pull requests within gems, I just created my own Middleware that borrows code from this pull request: https://gist.github.com/vasanthela/7afb8916c662ba783041

It can be used in the meantime for those with immediate need for this feature, like myself.

[locochris]

Thanx @vasanthela.
This'd be great as a gem (as I need to put a gem dependency in the gemspec of my gem).
I see now that a gem already exists http://rubygems.org/gems/faraday_body_logger, although its only registered on the Response to log response bodies (and doesn't pretty print like yours).
(Perhaps you could put in a PR on that gem with your gist? Or maybe we could rebase this PR and help it get merged?)

[emilebosch]

Why isnt nobody merging this?

[sferik]

Reviewed an merged.

[emilebosch]

Wow. That was fast 💃

[mislav]

👍

[locochris]

Cheers! 🍻

[hem-brahmbhatt]

Woohoo!