Skip to content

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

Closed
yurishkuro opened this issue Dec 7, 2024 · 6 comments · Fixed by #12172
Closed

confighttp.ToServer should be setting ErrorLog #11820

yurishkuro opened this issue Dec 7, 2024 · 6 comments · Fixed by #12172
Assignees
Labels
good first issue Good for newcomers help wanted Good issue for contributors to OpenTelemetry Service to pick up

Comments

@yurishkuro
Copy link
Member

Is your feature request related to a problem? Please describe.

Standard http.Server has a field ErrorLog:

	// ErrorLog specifies an optional logger for errors accepting
	// connections, unexpected behavior from handlers, and
	// underlying FileSystem errors.
	// If nil, logging is done via the log package's standard logger.
	ErrorLog *log.Logger

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:

	errorLog, _ := zap.NewStdLogAt(params.Logger, zapcore.ErrorLevel)
	server.ErrorLog = errorLog

I think confighttp.ToServer() should do the same.

@yurishkuro yurishkuro added help wanted Good issue for contributors to OpenTelemetry Service to pick up good first issue Good for newcomers labels Dec 7, 2024
@tarun-kavipurapu
Copy link

@yurishkuro hey can i take this up

@yurishkuro
Copy link
Member Author

feel free

@tarun-kavipurapu
Copy link

tarun-kavipurapu commented Dec 9, 2024

@yurishkuro
I Implemented this in this way is this the correct pattern to Implement or should the pass ErrorLog in ServerConfig Struct
sorry but i am kind a newbie to this

	errHandler func(w http.ResponseWriter, r *http.Request, errorMsg string, statusCode int)
	decoders   map[string]func(body io.ReadCloser) (io.ReadCloser, error)
	errorLog   *log.Logger
}
func WithErrorLogger(logger *log.Logger) ToServerOption {
	return toServerOptionFunc(func(opts *toServerOptions) {
		opts.errorLog = logger
	})
}

@yurishkuro
Copy link
Member Author

there is no need for new options - the ToServer function already receives a logger via telemetry settings. It can just assign ErrorLogger.

@tarun-kavipurapu
Copy link

tarun-kavipurapu commented Dec 9, 2024

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?

@yurishkuro
Copy link
Member Author

yes. It's a bug fix, not a capability.

tarun-kavipurapu pushed a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 9, 2024
tarun-kavipurapu pushed a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 9, 2024
tarun-kavipurapu added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 9, 2024
tarun-kavipurapu added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 9, 2024
tarun-kavipurapu added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 11, 2024
tarun-kavipurapu added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 11, 2024
tarun-kavipurapu added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 11, 2024
tarun-kavipurapu added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 11, 2024
codeboten added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 12, 2024
codeboten added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 12, 2024
codeboten added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 13, 2024
codeboten added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 13, 2024
codeboten added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 13, 2024
tarun-kavipurapu added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 15, 2024
github-merge-queue bot pushed a commit that referenced this issue Dec 15, 2024
#### 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]>
mx-psi added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 16, 2024
tarun-kavipurapu added a commit to tarun-kavipurapu/opentelemetry-collector that referenced this issue Dec 17, 2024
github-merge-queue bot pushed a commit that referenced this issue Jan 23, 2025
<!--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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Good issue for contributors to OpenTelemetry Service to pick up
Projects
None yet
2 participants