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

confighttp.ToServer should be setting ErrorLog #11820

Open
yurishkuro opened this issue Dec 7, 2024 · 6 comments · May be fixed by #11935
Open

confighttp.ToServer should be setting ErrorLog #11820

yurishkuro opened this issue Dec 7, 2024 · 6 comments · May be fixed by #11935
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
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
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 <223565+codeboten@users.noreply.github.com>
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
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
Development

Successfully merging a pull request may close this issue.

2 participants