Add the log formatter that is easy to override and safe to inherit

[prikha]

Description

Extract Formatter from logging middleware and enable it to be configurable.

Fixes #888

[technoweenie]

@prikha: Thank you for making your suggestion AND filing a PR with an implementation. I agree with the pain points you stated in #888, however I'm not sold on the implementation. It leaves the Logger middleware class so bare that I wonder why it's even there.

class Faraday::Response::Logger < Response::Middleware 
  def call(env)
    @formatter.request(env)
    super
  end

  def on_complete(env)
    @formatter.response(env)
  end
end

Did you consider implementing Formatter#request and Formatter#response in the Logger class itself? I think you'd get most of the benefits, without having to deal with a Logger and a Formatter class.

I suppose passing a custom Logger would require something like this (as you suggested in #888):

Faraday::Response.register_middleware logger: -> { FaradayMiddlewares::MyLogger }

conn = Faraday.new(:url => 'http://sushi.com/api_key=s3cr3t') do |faraday|
  faraday.response :logger, logger
end

That would at least let you replace all of the loggers in an application at once, but could obscure the fact that you're using a custom logger if you're just looking at a single instance of Faraday usage.

Is there some big reason that you feel the Formatter should be extracted out to a separate class that I'm missing? I think refactoring the Logger class itself would give you the same benefits. The only difference is how you specify your custom logger.

[prikha]

Hey @technoweenie thanks for taking a look. Strictly speaking there is nothing a decent programmer can't overcome(well at least in Ruby 😆). One can invent custom logging middleware and then do whatever I want there. That is true. However I would like to safely reuse some of the code from the faraday since it is already there and is documented to work in a certain way.

I see no issue in having a logging middleware just for the purpose of exposing the logging capabilities.

With the approach I suggest one can safely inherit from the current formatter. However inheriting from logging middleware implies that every change to parents call method would break it.

On the other hand custom middleware accepts a logger which is expected to have more or less trivial behaviour, like expose some methods having some signatures. Well ruby isn't a typed language, but when I see logger I set myself some expectations.

Maybe it would make sense to have request and response log formatters? That would allow code reuse and clearly separate the responsibility.

In general it would be cool to have logging extracted out of Request namespace which is way misleading. But it is way out of current discussion.

To sum up, I see #888 as an opportunity to improve the integration points of faraday, maybe we can thing of a better implementation, but this one solves faraday and SemanticLogger. Which would be a bit more painful otherwise.

[iMacTia]

To add on top of what @technoweenie already said, I'm personally happy with both solutions (a more modular logger contract, or a separate formatter). However I'd like to point out that the current implementation doesn't allow to reuse some useful methods even if you sub-class Formatter: I'm talking about methods like log_headers?, log_bodies? and apply_filters, which are currently private.
Whatever solution we go for, I'd like these helpers to be available on sub-classes so that creating my custom logger/formatter will be easier

[prikha]

@iMacTia thanks for participating. Maybe I'm not the best seller(I appreciate some help here), I am happy to tune the code to the point where we get a clean point of integration, however.

Can you clarify about the private methods? You mean it might be useful for them to be acceptable in the middleware itself? We could delegate those to the formatter from the logging middleware. Because when I subclass from the Formatter I surely get them available inside of my own implementation.

[iMacTia]

@prikha My fault, I was sure you couldn't access private method in subclasses 🙈. Please ignore me

[iMacTia]

@technoweenie Although both solutions are perfectly fine, I personally like the separation of concerns introduced with this PR as it clearly identifies the different actors in the logging process:

• Logger Middleware: Implements the middleware contract (call) and delegates the implementation to the formatter.
• Formatter: Takes request or response and format them to be printed.
• Logger: Takes the formatted request/response and prints it.

It's quite similar to how lograge works and allows to easily change one piece to change the behaviour.
Do you need to log as JSON? Change the logger.
Do you need to log only one line per request/response? Change formatter.

[technoweenie]

Great, lets go with this PR's approach 🤘

My only concern now is whether it breaks any existing APIs. Just looking at the current Logger class, we may have to keep #filter and the other private methods around in case anyone is using them. We're building up to Faraday v1.0 with compatibility with the existing community being a major consideration.

[technoweenie]

This looks good to me, but I'd like to see a test with a custom formatter in spec/faraday/response/logger_spec.rb.

[technoweenie]

Any thoughts on moving this to the top of the file instead? The Faraday Middleware Registry should prevent this from being loaded until someone uses the logger middleware for the first time.

[iMacTia]

makes sense to me

[prikha]

@technoweenie comments addressed, when we agree on the changes - I'll rebase and fix conflicts.

[technoweenie]

@prikha Thanks for addressing the comments. I commented on a nit pick in the tests, but I won't let that block the merging of this PR. We should be able to merge this once you fix the conflict.

[iMacTia]

Good to go for me as well as soon as conflict with base branch are fixed 👍 !

[prikha]

@iMacTia @technoweenie Thanks for the tip. Addressed. Give it a quick look and it's good to go.

[olleolleolle]

🌞 😍 🌞 Thank you so much for taking this all the way!