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

Use appropriate log level for errors #2025

Closed
kheardCB opened this issue Feb 8, 2023 · 4 comments · Fixed by #2044
Closed

Use appropriate log level for errors #2025

kheardCB opened this issue Feb 8, 2023 · 4 comments · Fixed by #2044
Assignees
Labels
help wanted We will welcome a contribution Type: Enhancement New feature or request
Milestone

Comments

@kheardCB
Copy link
Contributor

kheardCB commented Feb 8, 2023

In the HttpForwarder, the log level for errors is Information. Can this be changed to Error? This may be an issue in other log messages in other files, but this is the main one I noticed.

Background:
I am using Serilog for logging and setting the MinimumLevel override for "Microsoft.AspNetCore" and "Yarp.ReverseProxy.Forwarder" to Warning. This accompanied with the "UseSerilogRequestLogging" option allows the service to omit one log line per request. In this setup, any errors are not being logged because they are set as Information, not Error.

I would gladly make the change myself and create a Pull Request, if that would be helpful.

@kheardCB kheardCB added the Type: Bug Something isn't working label Feb 8, 2023
@Tratcher
Copy link
Member

Tratcher commented Feb 8, 2023

The main concern here is that we don't want to log anything at the Error level that is caused by an external entity that you can't do anything about, that floods logs unnecessarily. Errors should be actionable within the current app. Timeouts, disconnects, etc. are all expected within normal proxy operations.

To look at the proxy results and decide which ones you want to react to check here:
https://github.com/microsoft/reverse-proxy/blob/029e731a95c08a77af37700b1ab6ecd99e694d83/docs/docfx/articles/middleware.md#error-handling

https://github.com/microsoft/reverse-proxy/blob/79a7b806688311fd2cca2ca67dc7cc418c7925ab/docs/docfx/articles/direct-forwarding.md#error-handling

@karelz
Copy link
Member

karelz commented Feb 14, 2023

Triage:

  • Error level should be only for YARP problems that are actionable
  • Warning level should be for any unsuccessful proxying, things YARP is able to recover from - e.g. destination going out, client-side problems with network - see list in code.
    • Note: Warn level is turned on by default by templates
  • Information/Verbose level should be for all successful log entries.

We'd accept changing this log to Warning.

It is a one-line change in

private static readonly Action<ILogger, ForwarderError, string, Exception> _proxyError = LoggerMessage.Define<ForwarderError, string>(
LogLevel.Information,

@kheardCB are you interested in submitting a PR for it?

Simple to do, brings value and more sanity. We should do it in 2.x.

@karelz karelz added Type: Enhancement New feature or request help wanted We will welcome a contribution and removed Type: Bug Something isn't working labels Feb 14, 2023
@karelz karelz added this to the YARP 2.x milestone Feb 14, 2023
@kheardCB
Copy link
Contributor Author

@karelz Sure, I can submit a PR.

@karelz
Copy link
Member

karelz commented Feb 21, 2023

Great, looking forward to it @kheardCB!

Let us know if you hit any roadblocks, we will be happy to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted We will welcome a contribution Type: Enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants