feign-reactive icon indicating copy to clipboard operation
feign-reactive copied to clipboard

Allow to opt-out completely from DefaultReactiveLogger

Open zrvan opened this issue 1 year ago • 2 comments

In our production environment we have rather strict requirements as to what's logged on ERROR-level. Because of this we need to implement a custom ReactiveLoggerListener, which is fine, but we also need to disable the logger-level for DefaultReactiveLogger since it will always be enabled when we build our client with something like:

WebReactiveFeign.<CustomApi>builder()
                .httpClient(httpClient)
                .addLoggerListener(new CustomReactiveLogger(clock))

The DefaultReactiveLogger is registered here: https://github.com/PlaytikaOSS/feign-reactive/blob/baba570b5039c649d5487883f3db1e183026773a/feign-reactor-core/src/main/java/reactivefeign/ReactiveFeign.java#L170 and applies because the CoreWebBuilder extends ReactiveFeign.Builder which registers the logger in its constructor. This is inconvenient to us specifically because we use ReactiveFeign in an internal library that helps with constructing the ReactiveFeign-client, the http-client and related configuration for simpler consumption in our services, but the current solution requires us to also configure specific log-suppression in the various services. It would be more convenient to allow for opting out from DefaultReactiveLogger entirely, either by builder-methods like useDefaultLoggerListener()/disableDefaultLoggerListener() or clearLoggerListeners() followed by the currently available addLoggerListener(). An alternate, perhaps clearer change would be to add a constructor argument for the desired ReactiveLoggerListener(s), requiring consumers to specify logger. It would break existing code, but in an easy-to-fix manner. I would be willing to provide a pull-request, if this is at all interesting, but I would appreciate some guidance as to which approach to prefer.

zrvan avatar Sep 12 '23 12:09 zrvan

Hi Can you add more details about this approach?

An alternate, perhaps clearer change would be to add a constructor argument for the desired ReactiveLoggerListener(s), requiring consumers to specify logger. It would break existing code, but in an easy-to-fix manner.

kptfh avatar Nov 14 '23 13:11 kptfh

The same for me. DefaultReactiveLogger always logs ERROR even if a request is retried successfully. While each ERROR is an alert for devops team. Agree with suggestion that DefaultReactiveLogger should be disabled by default.

majorovms avatar Mar 04 '24 12:03 majorovms