From 2c6f4057a8bf86194f8bb741d003caeba2b27bfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juraci=20Paix=C3=A3o=20Kr=C3=B6hling?= Date: Fri, 28 Feb 2020 10:43:51 +0100 Subject: [PATCH] Follow-up improvements from #2060 (#2088) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Juraci Paixão Kröhling --- cmd/collector/app/collector.go | 26 ++++++++----------- .../app/processor/{span.go => interface.go} | 0 cmd/collector/app/server/grpc.go | 10 +++---- cmd/collector/app/server/http.go | 23 ++++++++-------- cmd/collector/app/server/zipkin.go | 17 ++++++------ cmd/collector/main.go | 6 +++-- cmd/flags/service.go | 2 +- 7 files changed, 40 insertions(+), 44 deletions(-) rename cmd/collector/app/processor/{span.go => interface.go} (100%) diff --git a/cmd/collector/app/collector.go b/cmd/collector/app/collector.go index 9177ad8df06..f96d1dcd59e 100644 --- a/cmd/collector/app/collector.go +++ b/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,13 +110,12 @@ 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 { @@ -126,12 +123,11 @@ func (c *Collector) Start(builderOpts *CollectorOptions) error { } 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 { diff --git a/cmd/collector/app/processor/span.go b/cmd/collector/app/processor/interface.go similarity index 100% rename from cmd/collector/app/processor/span.go rename to cmd/collector/app/processor/interface.go diff --git a/cmd/collector/app/server/grpc.go b/cmd/collector/app/server/grpc.go index af94358d1d2..c487b05cadf 100644 --- a/cmd/collector/app/server/grpc.go +++ b/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 } diff --git a/cmd/collector/app/server/http.go b/cmd/collector/app/server/http.go index 4748f6c6b44..6f1a963e0c0 100644 --- a/cmd/collector/app/server/http.go +++ b/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) + }() } diff --git a/cmd/collector/app/server/zipkin.go b/cmd/collector/app/server/zipkin.go index 0a8e116be95..c779e931171 100644 --- a/cmd/collector/app/server/zipkin.go +++ b/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)) diff --git a/cmd/collector/main.go b/cmd/collector/main.go index a9303f805ac..e67ad4447fc 100644 --- a/cmd/collector/main.go +++ b/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 }, diff --git a/cmd/flags/service.go b/cmd/flags/service.go index 567ecd49938..2381e6037b7 100644 --- a/cmd/flags/service.go +++ b/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),