Skip to content
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

Allow to opt-out completely from DefaultReactiveLogger #624

Open
zrvan opened this issue Sep 12, 2023 · 2 comments
Open

Allow to opt-out completely from DefaultReactiveLogger #624

zrvan opened this issue Sep 12, 2023 · 2 comments

Comments

@zrvan
Copy link

zrvan commented Sep 12, 2023

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:

addLoggerListener(new DefaultReactiveLogger(Clock.systemUTC()));

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
Copy link
Collaborator

kptfh commented Nov 14, 2023

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants