OpenFeign / feign

Feign makes writing java http clients easier
Apache License 2.0
9.35k stars 1.91k forks source link

Use SLF4J for library logging. #1143

Open kdavisk6 opened 4 years ago

kdavisk6 commented 4 years ago

Logging in Feign today is managed entirely behind our Logger abstraction. This wrapper is a simplistic form of logging facade similar to Commons Logging and SLF4J.

We should consider adopting a logging facade and replace the Logger with a more distinct LogConfiguration or other such abstraction. This change will allow users to control with logging subsystem they want to use without the need to configure Feign separately.

This will allow us to use the logger facade instances throughout the library, increasing our ability to log additional internal details while removing our current requirement to implement logger implementation specific Logger instances.

Here is an example of what the resulting changes to Feign.Builder could look like:

Feign.builder()
   .log(LogConfiguration.Builder()
      .withRequest()
      .withResponse()
      .withHeaders()
      .build())
   .build();

In addition, configuring what information is logged during Feign operations will become more explicit, leading to increased understanding and ease of use.

The potential draw backs here are that whatever facade we choose will require users to include that dependency in their project and we will need to include documentation on how to manage conflicts with Commons Logging for users whose projects intersect/conflict. This information is readily available however and shouldn't be considered a major blocker to this approach.


The alternative is to adopt something similar to what Spring has done, which is to maintain the Logger abstraction and place ourselves in the middle, co-opting the detection of the logging framework and adapt accordingly. More information can be found here: https://github.com/spring-projects/spring-framework/issues/19081

While this does provide the most flexibility, it does mean that we will be explicitly choosing Commons Logging over SLF4J as SLF4J is more opinionated and expresses that users will need to configure SLF4J and the appropriate bridges. Also, it does require that we, the Feign maintainers, also maintain our own mini-facade. For this reason alone, I feel that the above suggestion of using SLF4J and adding it as a dependency, it worthwhile.

vitalijr2 commented 2 years ago

Just for note: some modules depend on JCL


What if log() will get varargs like:

log(HEADERS, REQUEST, RESPONSE)

In my opinion that it allows to configure the logging but the builder is more hard-coded way.


As far as I understand

withRequest(), withResponse() and withHeaders() we could use simultaniuosly and in any order.

We also need something like withBasic().

But withBasic() should re-write configuration or vice versa should be ignored if any other with...() was used earlier.

vitalijr2 commented 2 years ago

How about logging as another capability?

Now we use capability for metrics and for Hystrix.

velo commented 2 years ago

@radio-rogal sound very interesting.... hystrix is using capabilities, man I'm lossing track of things =)

velo commented 2 years ago

Do you wanna play around with logs + capabilities and make a proposal? May or may not be approved, but, could be nice

vitalijr2 commented 2 years ago

I have started with Slf4jCapability + builder like @kdavisk6 Kevin wrote. It implements client and retryer wrappers so I add withRetryer too.

I am going to use the DEBUG level as it old implementation does. From logging prospective it should look the same.

velo commented 2 years ago

sounds awesome