search

PlaytikaOSS / feign-reactive

Reactive Feign client inspired by https://github.com/OpenFeign project
Apache License 2.0
600 stars 119 forks source link

Allow to opt-out completely from DefaultReactiveLogger #624

Open zrvan opened 10 months ago

zrvan commented 10 months ago

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.

kptfh commented 8 months ago

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.

majorovms commented 4 months ago

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.