Follow-up improvements from #2060 (#2088)

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

cmd/collector/app/collector.go

@@ -28,7 +28,6 @@ import (
 	"github.com/jaegertracing/jaeger/cmd/collector/app/sampling/strategystore"
 	"github.com/jaegertracing/jaeger/cmd/collector/app/server"
 	"github.com/jaegertracing/jaeger/pkg/healthcheck"
-	"github.com/jaegertracing/jaeger/pkg/recoveryhandler"
 	"github.com/jaegertracing/jaeger/storage/spanstore"
 )
 
@@ -84,7 +83,6 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error {
 
 	c.spanProcessor = handlerBuilder.BuildSpanProcessor()
 	c.spanHandlers = handlerBuilder.BuildHandlers(c.spanProcessor)
-	recoveryHandler := recoveryhandler.NewRecoveryHandler(c.logger, true)
 
 	if tchServer, err := server.StartThriftServer(&server.ThriftServerParams{
 		ServiceName:          c.serviceName,
@@ -112,26 +110,24 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error {
 	}
 
 	if httpServer, err := server.StartHTTPServer(&server.HTTPServerParams{
-		Port:            builderOpts.CollectorHTTPPort,
-		Handler:         c.spanHandlers.JaegerBatchesHandler,
-		RecoveryHandler: recoveryHandler,
-		HealthCheck:     c.hCheck,
-		MetricsFactory:  c.metricsFactory,
-		SamplingStore:   c.strategyStore,
-		Logger:          c.logger,
+		Port:           builderOpts.CollectorHTTPPort,
+		Handler:        c.spanHandlers.JaegerBatchesHandler,
+		HealthCheck:    c.hCheck,
+		MetricsFactory: c.metricsFactory,
+		SamplingStore:  c.strategyStore,
+		Logger:         c.logger,
 	}); err != nil {
 		c.logger.Fatal("could not start the HTTP server", zap.Error(err))
 	} else {
 		c.hServer = httpServer
 	}
 
 	if zkServer, err := server.StartZipkinServer(&server.ZipkinServerParams{
-		Port:            builderOpts.CollectorZipkinHTTPPort,
-		Handler:         c.spanHandlers.ZipkinSpansHandler,
-		RecoveryHandler: recoveryHandler,
-		AllowedHeaders:  builderOpts.CollectorZipkinAllowedHeaders,
-		AllowedOrigins:  builderOpts.CollectorZipkinAllowedOrigins,
-		Logger:          c.logger,
+		Port:           builderOpts.CollectorZipkinHTTPPort,
+		Handler:        c.spanHandlers.ZipkinSpansHandler,
+		AllowedHeaders: builderOpts.CollectorZipkinAllowedHeaders,
+		AllowedOrigins: builderOpts.CollectorZipkinAllowedOrigins,
+		Logger:         c.logger,
 	}); err != nil {
 		c.logger.Fatal("could not start the Zipkin server", zap.Error(err))
 	} else {

cmd/collector/app/server/grpc.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2020 The Jaeger Authors.
+// Copyright (c) 2018 The Jaeger Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -16,15 +16,12 @@ package server
 
 import (
 	"fmt"
-	"io/ioutil"
 	"net"
-	"os"
 	"strconv"
 
 	"go.uber.org/zap"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials"
-	"google.golang.org/grpc/grpclog"
 
 	"github.com/jaegertracing/jaeger/cmd/collector/app/handler"
 	"github.com/jaegertracing/jaeger/cmd/collector/app/sampling"
@@ -46,7 +43,6 @@ type GRPCServerParams struct {
 // StartGRPCServer based on the given parameters
 func StartGRPCServer(params *GRPCServerParams) (*grpc.Server, error) {
 	var server *grpc.Server
-	grpclog.SetLoggerV2(grpclog.NewLoggerV2(ioutil.Discard, os.Stderr, os.Stderr))
 
 	if params.TLSConfig.Enabled {
 		// user requested a server with TLS, setup creds
@@ -80,14 +76,14 @@ func serveGRPC(server *grpc.Server, listener net.Listener, params *GRPCServerPar
 	api_v2.RegisterSamplingManagerServer(server, sampling.NewGRPCHandler(params.SamplingStore))
 
 	params.Logger.Info("Starting jaeger-collector gRPC server", zap.Int("grpc-port", params.Port))
-	go func(server *grpc.Server) {
+	go func() {
 		if err := server.Serve(listener); err != nil {
 			params.Logger.Error("Could not launch gRPC service", zap.Error(err))
 			if params.OnError != nil {
 				params.OnError(err)
 			}
 		}
-	}(server)
+	}()
 
 	return nil
 }

cmd/collector/app/server/http.go

@@ -27,17 +27,17 @@ import (
 	"github.com/jaegertracing/jaeger/cmd/collector/app/sampling/strategystore"
 	clientcfgHandler "github.com/jaegertracing/jaeger/pkg/clientcfg/clientcfghttp"
 	"github.com/jaegertracing/jaeger/pkg/healthcheck"
+	"github.com/jaegertracing/jaeger/pkg/recoveryhandler"
 )
 
 // HTTPServerParams to construct a new Jaeger Collector HTTP Server
 type HTTPServerParams struct {
-	Port            int
-	Handler         handler.JaegerBatchesHandler
-	RecoveryHandler func(http.Handler) http.Handler
-	SamplingStore   strategystore.StrategyStore
-	MetricsFactory  metrics.Factory
-	HealthCheck     *healthcheck.HealthCheck
-	Logger          *zap.Logger
+	Port           int
+	Handler        handler.JaegerBatchesHandler
+	SamplingStore  strategystore.StrategyStore
+	MetricsFactory metrics.Factory
+	HealthCheck    *healthcheck.HealthCheck
+	Logger         *zap.Logger
 }
 
 // StartHTTPServer based on the given parameters
@@ -72,13 +72,14 @@ func serveHTTP(server *http.Server, listener net.Listener, params *HTTPServerPar
 	})
 	cfgHandler.RegisterRoutes(r)
 
-	server.Handler = params.RecoveryHandler(r)
-	go func(listener net.Listener, hServer *http.Server) {
-		if err := hServer.Serve(listener); err != nil {
+	recoveryHandler := recoveryhandler.NewRecoveryHandler(params.Logger, true)
+	server.Handler = recoveryHandler(r)
+	go func() {
+		if err := server.Serve(listener); err != nil {
 			if err != http.ErrServerClosed {
 				params.Logger.Fatal("Could not start HTTP collector", zap.Error(err))
 			}
 		}
 		params.HealthCheck.Set(healthcheck.Unavailable)
-	}(listener, server)
+	}()
 }

cmd/collector/app/server/zipkin.go

@@ -27,17 +27,17 @@ import (
 	"github.com/jaegertracing/jaeger/cmd/collector/app/handler"
 	"github.com/jaegertracing/jaeger/cmd/collector/app/zipkin"
 	"github.com/jaegertracing/jaeger/pkg/healthcheck"
+	"github.com/jaegertracing/jaeger/pkg/recoveryhandler"
 )
 
 // ZipkinServerParams to construct a new Jaeger Collector Zipkin Server
 type ZipkinServerParams struct {
-	Port            int
-	Handler         handler.ZipkinSpansHandler
-	RecoveryHandler func(http.Handler) http.Handler
-	AllowedOrigins  string
-	AllowedHeaders  string
-	HealthCheck     *healthcheck.HealthCheck
-	Logger          *zap.Logger
+	Port           int
+	Handler        handler.ZipkinSpansHandler
+	AllowedOrigins string
+	AllowedHeaders string
+	HealthCheck    *healthcheck.HealthCheck
+	Logger         *zap.Logger
 }
 
 // StartZipkinServer based on the given parameters
@@ -74,7 +74,8 @@ func serveZipkin(server *http.Server, listener net.Listener, params *ZipkinServe
 		AllowedHeaders: headers,
 	})
 
-	server.Handler = cors.Handler(params.RecoveryHandler(r))
+	recoveryHandler := recoveryhandler.NewRecoveryHandler(params.Logger, true)
+	server.Handler = cors.Handler(recoveryHandler(r))
 	go func(listener net.Listener, server *http.Server) {
 		if err := server.Serve(listener); err != nil {
 			params.Logger.Fatal("Could not launch Zipkin server", zap.Error(err))

cmd/collector/main.go

@@ -64,7 +64,6 @@ func main() {
 			logger := svc.Logger // shortcut
 			baseFactory := svc.MetricsFactory.Namespace(metrics.NSOptions{Name: "jaeger"})
 			metricsFactory := baseFactory.Namespace(metrics.NSOptions{Name: "collector"})
-			strategyStoreFactory.InitFromViper(v)
 
 			storageFactory.InitFromViper(v)
 			if err := storageFactory.Initialize(baseFactory, logger); err != nil {
@@ -102,7 +101,10 @@ func main() {
 						logger.Error("failed to close span writer", zap.Error(err))
 					}
 				}
-				c.Close()
+
+				if err := c.Close(); err != nil {
+					logger.Error("failed to cleanly close the collector", zap.Error(err))
+				}
 			})
 			return nil
 		},

cmd/flags/service.go

@@ -93,7 +93,7 @@ func (s *Service) Start(v *viper.Viper) error {
 	newProdConfig.Sampling = nil
 	if logger, err := sFlags.NewLogger(newProdConfig); err == nil {
 		s.Logger = logger
-		grpcZap.ReplaceGrpcLogger(logger.WithOptions(
+		grpcZap.ReplaceGrpcLoggerV2(logger.WithOptions(
 			// grpclog is not consistent with the depth of call tree before it's dispatched to zap,
 			// but Skip(2) still shows grpclog as caller, while Skip(3) shows actual grpc packages.
 			zap.AddCallerSkip(3),