Add Admin port and group all ports in one file (#1442)

* Add Admin port and group all ports in one file Signed-off-by: Yuri Shkuro <ys@uber.com> * Clean-up Signed-off-by: Yuri Shkuro <ys@uber.com> * Restore gen-assets Signed-off-by: Yuri Shkuro <ys@uber.com> * Add missing call to AddFlags; fix tests Signed-off-by: Yuri Shkuro <ys@uber.com> * Fix test Signed-off-by: Yuri Shkuro <ys@uber.com> * Add empty test file Signed-off-by: Yuri Shkuro <ys@uber.com> * Explain empty_test.go files Signed-off-by: Yuri Shkuro <ys@uber.com> * Normalize labels on deprecated CLI flags to (deprecated) use {replacement} Signed-off-by: Yuri Shkuro <ys@uber.com>
jaegertracing · Mar 27, 2019 · 95c69cc · 95c69cc
1 parent fc48919
commit 95c69cc
Show file tree

Hide file tree

Showing 25 changed files with 468 additions and 436 deletions.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -115,3 +115,25 @@ import (
 )
 ```
 
+## Testing guidelines
+
+We strive to maintain as high code coverage as possible. Since `go test` command does not generate
+code coverage information for packages that have no test files, we have a build step (`make nocover`)
+that breaks the build when such packages are discovered, with an error like this:
+
+```
+error: at least one *_test.go file must be in all directories with go files
+       so that they are counted for code coverage.
+       If no tests are possible for a package (e.g. it only defines types), create empty_test.go
+```
+
+There are conditions that cannot be tested without external dependencies, such as a function that
+creates a gocql.Session, because it requires an active connection to Cassandra database. It is
+recommended to isolate such functions in a separate package with bare minimum of code and add a
+file `.nocover` to exclude the package from coverage calculations. The file should contain
+a comment explaining why it is there, for example:
+
+```
+$ cat ./pkg/cassandra/config/.nocover
+requires connection to Cassandra
+```
diff --git a/cmd/agent/app/builder.go b/cmd/agent/app/builder.go
@@ -17,6 +17,7 @@ package app
 import (
 	"fmt"
 	"net/http"
+	"strconv"
 
 	"github.com/apache/thrift/lib/go/thrift"
 	"github.com/pkg/errors"
@@ -31,6 +32,7 @@ import (
 	"github.com/jaegertracing/jaeger/cmd/agent/app/reporter/tchannel"
 	"github.com/jaegertracing/jaeger/cmd/agent/app/servers"
 	"github.com/jaegertracing/jaeger/cmd/agent/app/servers/thriftudp"
+	"github.com/jaegertracing/jaeger/ports"
 	zipkinThrift "github.com/jaegertracing/jaeger/thrift-gen/agent"
 	jaegerThrift "github.com/jaegertracing/jaeger/thrift-gen/jaeger"
 )
@@ -40,15 +42,15 @@ const (
 	defaultMaxPacketSize = 65000
 	defaultServerWorkers = 10
 
-	defaultHTTPServerHostPort = ":5778"
-
 	jaegerModel Model = "jaeger"
 	zipkinModel       = "zipkin"
 
 	compactProtocol Protocol = "compact"
 	binaryProtocol           = "binary"
 )
 
+var defaultHTTPServerHostPort = ":" + strconv.Itoa(ports.AgentConfigServerHTTP)
+
 // Model used to distinguish the data transfer model
 type Model string
 

diff --git a/cmd/agent/app/flags.go b/cmd/agent/app/flags.go
@@ -17,8 +17,11 @@ package app
 import (
 	"flag"
 	"fmt"
+	"strconv"
 
 	"github.com/spf13/viper"
+
+	"github.com/jaegertracing/jaeger/ports"
 )
 
 const (
@@ -32,21 +35,21 @@ const (
 var defaultProcessors = []struct {
 	model    Model
 	protocol Protocol
-	hostPort string
+	port     int
 }{
-	{model: "zipkin", protocol: "compact", hostPort: ":5775"},
-	{model: "jaeger", protocol: "compact", hostPort: ":6831"},
-	{model: "jaeger", protocol: "binary", hostPort: ":6832"},
+	{model: "zipkin", protocol: "compact", port: ports.AgentZipkinThriftCompactUDP},
+	{model: "jaeger", protocol: "compact", port: ports.AgentJaegerThriftCompactUDP},
+	{model: "jaeger", protocol: "binary", port: ports.AgentJaegerThriftBinaryUDP},
 }
 
 // AddFlags adds flags for Builder.
 func AddFlags(flags *flag.FlagSet) {
-	for _, processor := range defaultProcessors {
-		prefix := fmt.Sprintf("processor.%s-%s.", processor.model, processor.protocol)
+	for _, p := range defaultProcessors {
+		prefix := fmt.Sprintf("processor.%s-%s.", p.model, p.protocol)
 		flags.Int(prefix+suffixWorkers, defaultServerWorkers, "how many workers the processor should run")
 		flags.Int(prefix+suffixServerQueueSize, defaultQueueSize, "length of the queue for the UDP server")
 		flags.Int(prefix+suffixServerMaxPacketSize, defaultMaxPacketSize, "max packet size for the UDP server")
-		flags.String(prefix+suffixServerHostPort, processor.hostPort, "host:port for the UDP server")
+		flags.String(prefix+suffixServerHostPort, ":"+strconv.Itoa(p.port), "host:port for the UDP server")
 	}
 	flags.String(
 		httpServerHostPort,

diff --git a/cmd/agent/app/reporter/tchannel/flags.go b/cmd/agent/app/reporter/tchannel/flags.go
@@ -55,15 +55,15 @@ func AddFlags(flags *flag.FlagSet) {
 	flags.String(
 		collectorHostPort,
 		"",
-		"Deprecated; comma-separated string representing host:ports of a static list of collectors to connect to directly (e.g. when not using service discovery)")
+		"(deprecated) see --"+tchannelPrefix+hostPort)
 	flags.Int(
 		discoveryMinPeers,
 		defaultMinPeers,
-		"Deprecated; if using service discovery, the min number of connections to maintain to the backend")
+		"(deprecated) see --"+tchannelPrefix+discoveryMinPeers)
 	flags.Duration(
 		discoveryConnCheckTimeout,
 		defaultConnCheckTimeout,
-		"Deprecated; sets the timeout used when establishing new connections")
+		"(deprecated) see --"+tchannelPrefix+discoveryConnCheckTimeout)
 }
 
 // InitFromViper initializes Builder with properties retrieved from Viper.

diff --git a/cmd/agent/main.go b/cmd/agent/main.go
@@ -17,60 +17,41 @@ package main
 import (
 	"fmt"
 	"io"
-	"io/ioutil"
-	"net/http"
 	"os"
-	"os/signal"
-	"syscall"
 
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
-	jMetrics "github.com/uber/jaeger-lib/metrics"
+	"github.com/uber/jaeger-lib/metrics"
 	"go.uber.org/zap"
-	"google.golang.org/grpc/grpclog"
 
 	"github.com/jaegertracing/jaeger/cmd/agent/app"
 	"github.com/jaegertracing/jaeger/cmd/agent/app/reporter"
 	"github.com/jaegertracing/jaeger/cmd/agent/app/reporter/grpc"
 	"github.com/jaegertracing/jaeger/cmd/agent/app/reporter/tchannel"
 	"github.com/jaegertracing/jaeger/cmd/flags"
 	"github.com/jaegertracing/jaeger/pkg/config"
-	"github.com/jaegertracing/jaeger/pkg/metrics"
 	"github.com/jaegertracing/jaeger/pkg/version"
+	"github.com/jaegertracing/jaeger/ports"
 )
 
 func main() {
-	var signalsChannel = make(chan os.Signal)
-	signal.Notify(signalsChannel, os.Interrupt, syscall.SIGTERM)
-
-	grpclog.SetLoggerV2(grpclog.NewLoggerV2(ioutil.Discard, os.Stderr, os.Stderr))
+	svc := flags.NewService(ports.AgentAdminHTTP)
+	svc.NoStorage = true
 
 	v := viper.New()
 	var command = &cobra.Command{
 		Use:   "jaeger-agent",
 		Short: "Jaeger agent is a local daemon program which collects tracing data.",
 		Long:  `Jaeger agent is a daemon program that runs on every host and receives tracing data submitted by Jaeger client libraries.`,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			err := flags.TryLoadConfigFile(v)
-			if err != nil {
-				return err
-			}
-
-			sFlags := new(flags.SharedFlags).InitFromViper(v)
-			logger, err := sFlags.NewLogger(zap.NewProductionConfig())
-			if err != nil {
+			if err := svc.Start(v); err != nil {
 				return err
 			}
-
-			builder := new(app.Builder).InitFromViper(v)
-			mBldr := new(metrics.Builder).InitFromViper(v)
-
-			mFactory, err := mBldr.CreateMetricsFactory("jaeger")
-			if err != nil {
-				logger.Fatal("Could not create metrics", zap.Error(err))
-			}
-			mFactory = mFactory.Namespace(jMetrics.NSOptions{Name: "agent", Tags: nil})
+			logger := svc.Logger // shortcut
+			mFactory := svc.MetricsFactory.
+				Namespace(metrics.NSOptions{Name: "jaeger"}).
+				Namespace(metrics.NSOptions{Name: "agent"})
 
 			rOpts := new(reporter.Options).InitFromViper(v)
 			tChanOpts := new(tchannel.Builder).InitFromViper(v, logger)
@@ -82,26 +63,21 @@ func main() {
 
 			// TODO illustrate discovery service wiring
 
+			builder := new(app.Builder).InitFromViper(v)
 			agent, err := builder.CreateAgent(cp, logger, mFactory)
 			if err != nil {
 				return errors.Wrap(err, "Unable to initialize Jaeger Agent")
 			}
 
-			if h := mBldr.Handler(); mFactory != nil && h != nil {
-				logger.Info("Registering metrics handler with HTTP server", zap.String("route", mBldr.HTTPRoute))
-				agent.GetServer().Handler.(*http.ServeMux).Handle(mBldr.HTTPRoute, h)
-			}
-
 			logger.Info("Starting agent")
 			if err := agent.Run(); err != nil {
 				return errors.Wrap(err, "Failed to run the agent")
 			}
-			<-signalsChannel
-			logger.Info("Shutting down")
-			if closer, ok := cp.(io.Closer); ok {
-				closer.Close()
-			}
-			logger.Info("Shutdown complete")
+			svc.RunAndThen(func() {
+				if closer, ok := cp.(io.Closer); ok {
+					closer.Close()
+				}
+			})
 			return nil
 		},
 	}
@@ -111,13 +87,11 @@ func main() {
 	config.AddFlags(
 		v,
 		command,
-		flags.AddConfigFileFlag,
-		flags.AddLoggingFlag,
+		svc.AddFlags,
 		app.AddFlags,
 		reporter.AddFlags,
 		tchannel.AddFlags,
 		grpc.AddFlags,
-		metrics.AddFlags,
 	)
 
 	if err := command.Execute(); err != nil {