OpenFeign / feign

Feign makes writing java http clients easier
Apache License 2.0
9.52k stars 1.93k forks source link

Provide proper integration with SLF4J #738

Open dinomite opened 6 years ago

dinomite commented 6 years ago

Feign currently has it's own implementation for logging through the Logger class. This provides informative messages and configurable levels of detail about requests, from just the request method/URL & response, to including the headers, or even the entire body. This configuration applies to all requests for the client.

Feign also provides feign-slf4j which will emit those log messages via SLF4J at the debug level.

First, the slf4j integration doesn't provide any output in environments where SLF4J is set to any level less than debug (most production environments I've interacted with only log info and above).

Second, the logging is all or nothing—there is no way to see more detail for requests that result in an error.

Logging in Feign would be much more useful if it could provide output tailored to different log levels that SLF4J defines, e.g. headers at debug, request method & URL at info, retries at warn, and failures at error. This would allow users existing SLF4J configurations to tailor Feign's output as appropriate for the environment.

I do not think this can be accomplished in the existing framework provided by Logger, as it does not know what level the underlying logging implementation is configured for nor does it provide logging level information to the underlying implementation along with messages. I propose adding slf4j-api to feign-core and using it for logging throughout Feign.

I realize that this is a big request, as feign-core currently has no required dependencies, but SLF4J is incredibly widely used and its addition would prove a great utility for Feign deployments in production environments.

spencergibb commented 6 years ago

Spring doesn't have a hard dependency on SLF4J. Changes to the Logger interface to provide similar functionality could be useful.

dinomite commented 6 years ago

True—thinking more the logger interface could pass Feign's level to the log() method and the implementation of feign.Logger could decide what native log level to emit.

kdavisk6 commented 6 years ago

The Logger can be extended to support any logging framework you like, so we can look into adding more logger implementations for things like log4j or commons-logging, if folks like. This is the preferred way to extend Feign. If we merge slf4j into core, than we are forcing users to use it. I'd prefer to let users use what they are comfortable with and not necessarily prescribe one approach over another.

Regarding more flexibility, I agree that the current use of Logger is controlled completely by the internals of Feign itself, right down to what is logged at what level. It is more of a window into Feign, than something that can be used for auditing or troubleshooting.

I would help me a lot if you could provide specific use cases you are looking to support.

dinomite commented 6 years ago

@kdavisk6 I want to see the request URL & method at INFO, headers at DEBUG, and any retry activity at WARN (with failures at ERROR).