Add Zap as the default error log in confighttp #11935
Conversation
dmathieu
changed the title
| @@ -478,16 +480,18 @@ func (hss *ServerConfig) ToServer(_ context.Context, host component.Host, settin | |||
| next: handler, | |||
| includeMetadata: hss.IncludeMetadata, | |||
| } | |||
| errorLog, err := zap.NewStdLogAt(settings.Logger, zapcore.ErrorLevel) | |||
There was a problem hiding this comment.
We should return errors as soon as they happen, not later.
| errorLog, err := zap.NewStdLogAt(settings.Logger, zapcore.ErrorLevel) | |
| errorLog, err := zap.NewStdLogAt(settings.Logger, zapcore.ErrorLevel) | |
| if err != nil { | |
| return nil, err | |
| } |
There was a problem hiding this comment.
the suggested change was the initial code too
but in
https://github.com/open-telemetry/opentelemetry-collector/pull/11830/files/71c16e0b293b4d6ed25146c4dd295093e672afd7#r1880527497
it was suggested to return server,err so that to avoid coverage report failures.
There was a problem hiding this comment.
Then, how about something like:
server := &http.Server{
Handler: handler,
ReadTimeout: hss.ReadTimeout,
ReadHeaderTimeout: hss.ReadHeaderTimeout,
WriteTimeout: hss.WriteTimeout,
IdleTimeout: hss.IdleTimeout,
}
errorLog, err := zap.NewStdLogAt(settings.Logger, zapcore.ErrorLevel)
if err != nil {
return server, err
}
server.ErrorLog = errorLog
return server, nilThen, we can handle multiple things that could return errors, and we also keep error return alongside its definition.
There was a problem hiding this comment.
I will check the report locally and may be make the change according to it
the command for it is make gotest-with-cover right?
Co-authored-by: Damien Mathieu <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11935 +/- ##
=======================================
Coverage 91.59% 91.59%
=======================================
Files 448 448
Lines 23759 23761 +2
=======================================
+ Hits 21761 21763 +2
Misses 1623 1623
Partials 375 375 ☔ View full report in Codecov by Sentry. |
Description
This Issue Fixes the Issue #11820
This is a continuation pull request for #11830
Fixes #11820
-Made the Zap as the default ErrorLog Supplied to the Http server