-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use SLF4J for library logging. #1143
Comments
Just for note: some modules depend on JCL
What if 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
We also need something like But |
How about logging as another capability? Now we use capability for metrics and for Hystrix. |
@radio-rogal sound very interesting.... hystrix is using capabilities, man I'm lossing track of things =) |
Do you wanna play around with logs + capabilities and make a proposal? May or may not be approved, but, could be nice |
I have started with Slf4jCapability + builder like @kdavisk6 Kevin wrote. It implements client and retryer wrappers so I add I am going to use the DEBUG level as it old implementation does. From logging prospective it should look the same. |
sounds awesome |
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 distinctLogConfiguration
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: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: spring-projects/spring-framework#19081While 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.
The text was updated successfully, but these errors were encountered: