Ensure Zipkin shutdown correctness #2765
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2765 +/- ##
==========================================
- Coverage 91.80% 91.79% -0.02%
==========================================
Files 291 291
Lines 15528 15529 +1
==========================================
- Hits 14256 14255 -1
- Misses 869 870 +1
- Partials 403 404 +1
Continue to review full report at Codecov.
|
| err = zr.server.Serve(listener) | ||
| if err != nil { | ||
| if err != nil && err != http.ErrServerClosed { |
There was a problem hiding this comment.
See https://golang.org/pkg/net/http/#Server.Serve it never returns nil.
if errHTTP := zr.server.Serve(listene); errHTTP != http.ErrServerClosed {
host.ReportFatalError(err)
}
May worth changing the OTLP if you want to not spam the ReportFatalError logs.
There was a problem hiding this comment.
Actually it is an "and" so just unnecessary extra protection.
There was a problem hiding this comment.
Yeah, will remove the check for error nil for HTTP server. For grpc the docs imply that it can actually return nil: "Serve will return a non-nil error unless Stop or GracefulStop is called." Looking at the sources it can definitely return nil. https://github.com/grpc/grpc-go/blob/v1.36.0/server.go#L768
| @@ -99,12 +100,14 @@ func (zr *ZipkinReceiver) Start(ctx context.Context, host component.Host) error | |||
| var listener net.Listener | |||
| listener, err = zr.config.HTTPServerSettings.ToListener() | |||
| if err != nil { | |||
| host.ReportFatalError(err) | |||
| return | ||
| } | ||
| zr.shutdownCh = make(chan struct{}) |
There was a problem hiding this comment.
Use waitgroup, so an easy "refactoring" can be done and allow helper to provide this.
There was a problem hiding this comment.
Good idea... done.
pjanotti
force-pushed
the
fix-zipkin-lifecycle
branch
from
df2dedd
to
294ba99
Compare
Description:
Zipkin shutdown method was returning before its server goroutine was completed and not checking for the expected
http.ErrServerClosed. This PR ensures shutdown only returns after the goroutine completes, no errors are reported when the server is closed. Opportunistic fix: remove an improper call toReportFatalErrorfor a synchronous error when the port is already in use.Testing:
Enabled lifecycle test for Zipkin receiver.