-
Notifications
You must be signed in to change notification settings - Fork 1.6k
confighttp.ToServer should be setting ErrorLog #11820
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
Comments
@yurishkuro hey can i take this up |
feel free |
@yurishkuro
|
there is no need for new options - the ToServer function already receives a logger via telemetry settings. It can just assign ErrorLogger. |
So you mean to say the Error Logger Should always be Zap.logger and the client should not get any option to give his own logger? |
yes. It's a bug fix, not a capability. |
Co-authored-by: Yuri Shkuro <[email protected]>
#### Description This Issue Fixes the Issue #11820 Fixes - Made the Zap as the default ErrorLog Supplied to the Http server --------- Co-authored-by: Alex Boten <[email protected]>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Added Zap as the default error logger in `confighttp.ToServer()` to ensure consistency in logging and to prevent errors from being printed using a different logger. This change configures the `ErrorLog` field of the `http.Server` to use a logger backed by Zap, which is initialized with the provided logger in `TelemetrySettings`. If the logger creation fails, the function now returns `nil, err` to prevent any undefined behavior from occurring due to an incomplete setup of the logger. <!-- Issue number if applicable --> #### Link to tracking issue Fixes #11820 <!--Describe the documentation added.--> #### Documentation Updated the changelog with information regarding the new default error logger for `confighttp`. Added details to internal documentation to reflect the change and ensure proper usage of the new logging system in server configuration.
Is your feature request related to a problem? Please describe.
Standard
http.Server
has a fieldErrorLog
:However,
confighttp.ToServer
leaves it empty, which will lead to error logs printed using a different logger.Describe the solution you'd like
In Jaeger we initialize it with a logger backed by Zap:
I think
confighttp.ToServer()
should do the same.The text was updated successfully, but these errors were encountered: