[trace] Migrate go/trace to use pflag for flag definitions (vites…

…sio#11028)

* Tidy up imports

Signed-off-by: Andrew Mason <[email protected]>

* [flagutil] Augument OptionalFlag to implement `pflag.Value`

Signed-off-by: Andrew Mason <[email protected]>

* Migrate go/trace to use pflag for flag definitions

Closes vitessio#10747.

Signed-off-by: Andrew Mason <[email protected]>

* Update help

Signed-off-by: Andrew Mason <[email protected]>

Signed-off-by: Andrew Mason <[email protected]>

go/flags/endtoend/vtctld.txt

@@ -250,8 +250,8 @@ Usage of vtctld:
       --topo_zk_tls_key string                                           the key to use to connect to the zk topo server, enables TLS
       --tracer string                                                    tracing service to use (default "noop")
       --tracing-enable-logging                                           whether to enable logging in the tracing service
-      --tracing-sampling-rate OptionalFloat64                            sampling rate for the probabilistic jaeger sampler (default 0.1)
-      --tracing-sampling-type OptionalString                             sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote' (default const)
+      --tracing-sampling-rate float                                      sampling rate for the probabilistic jaeger sampler (default 0.1)
+      --tracing-sampling-type string                                     sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote' (default "const")
       --track_schema_versions                                            When enabled, vttablet will store versions of schemas at each position that a DDL is applied and allow retrieval of the schema corresponding to a position
       --transaction-log-stream-handler string                            URL handler for streaming transactions log (default "/debug/txlog")
       --transaction_limit_by_component                                   Include CallerID.component when considering who the user is for the purpose of transaction limit.

go/flags/endtoend/vtexplain.txt

@@ -17,8 +17,6 @@ Usage of vtexplain:
       --consolidator-stream-query-size int                               Configure the stream consolidator query size in bytes. Setting to 0 disables the stream consolidator. (default 2097152)
       --consolidator-stream-total-size int                               Configure the stream consolidator total size in bytes. Setting to 0 disables the stream consolidator. (default 134217728)
       --cpu_profile string                                               deprecated: use '-pprof=cpu' instead
-      --datadog-agent-host string                                        host to send spans to. if empty, no tracing will be done
-      --datadog-agent-port string                                        port to send spans to. if empty, no tracing will be done
       --db-credentials-file string                                       db credentials file; send SIGHUP to reload this file
       --db-credentials-server string                                     db credentials server type ('file' - file implementation; 'vault' - HashiCorp Vault implementation) (default "file")
       --db-credentials-vault-addr string                                 URL to Vault server
@@ -106,7 +104,6 @@ Usage of vtexplain:
       --hot_row_protection_concurrent_transactions int                   Number of concurrent transactions let through to the txpool/MySQL for the same hot row. Should be > 1 to have enough 'ready' transactions in MySQL and benefit from a pipelining effect. (default 5)
       --hot_row_protection_max_global_queue_size int                     Global queue limit across all row (ranges). Useful to prevent that the queue can grow unbounded. (default 1000)
       --hot_row_protection_max_queue_size int                            Maximum number of BeginExecute RPCs which will be queued for the same row (range). (default 20)
-      --jaeger-agent-host string                                         host and port to send spans to. if empty, no tracing will be done
       --keep_logs duration                                               keep logs for this long (using ctime) (zero to keep forever) (default 0s)
       --keep_logs_by_mtime duration                                      keep logs for this long (using mtime) (zero to keep forever) (default 0s)
       --ks-shard-map string                                              JSON map of keyspace name -> shard name -> ShardReference object. The inner map is the same as the output of FindAllShardsInKeyspace
@@ -243,10 +240,6 @@ Usage of vtexplain:
       --topo_global_root string                                          the path of the global topology data in the global topology server
       --topo_global_server_address string                                the address of the global topology server
       --topo_implementation string                                       the topology implementation to use
-      --tracer string                                                    tracing service to use (default "noop")
-      --tracing-enable-logging                                           whether to enable logging in the tracing service
-      --tracing-sampling-rate OptionalFloat64                            sampling rate for the probabilistic jaeger sampler (default 0.1)
-      --tracing-sampling-type OptionalString                             sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote' (default const)
       --track_schema_versions                                            When enabled, vttablet will store versions of schemas at each position that a DDL is applied and allow retrieval of the schema corresponding to a position
       --transaction-log-stream-handler string                            URL handler for streaming transactions log (default "/debug/txlog")
       --transaction_limit_by_component                                   Include CallerID.component when considering who the user is for the purpose of transaction limit.

go/flags/endtoend/vtgate.txt

@@ -189,8 +189,8 @@ Usage of vtgate:
       --topo_zk_tls_key string                                           the key to use to connect to the zk topo server, enables TLS
       --tracer string                                                    tracing service to use (default "noop")
       --tracing-enable-logging                                           whether to enable logging in the tracing service
-      --tracing-sampling-rate OptionalFloat64                            sampling rate for the probabilistic jaeger sampler (default 0.1)
-      --tracing-sampling-type OptionalString                             sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote' (default const)
+      --tracing-sampling-rate float                                      sampling rate for the probabilistic jaeger sampler (default 0.1)
+      --tracing-sampling-type string                                     sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote' (default "const")
       --transaction_mode string                                          SINGLE: disallow multi-db transactions, MULTI: allow multi-db transactions with best effort commit, TWOPC: allow multi-db transactions with 2pc commit (default "MULTI")
   -v, --v Level                                                          log level for V logs
       --version                                                          print binary version

go/flags/endtoend/vtgr.txt

@@ -5,8 +5,6 @@ Usage of vtgr:
       --clusters_to_watch strings                                        Comma-separated list of keyspaces or keyspace/shards that this instance will monitor and repair. Defaults to all clusters in the topology. Example: "ks1,ks2/-80"
       --consul_auth_static_file string                                   JSON File to read the topos/tokens from.
       --cpu_profile string                                               deprecated: use '-pprof=cpu' instead
-      --datadog-agent-host string                                        host to send spans to. if empty, no tracing will be done
-      --datadog-agent-port string                                        port to send spans to. if empty, no tracing will be done
       --db_config string                                                 Full path to db config file that will be used by VTGR.
       --db_flavor string                                                 MySQL flavor override. (default "MySQL56")
       --db_port int                                                      Local mysql port, set this to enable local fast check.
@@ -40,7 +38,6 @@ Usage of vtgr:
       --grpc_server_keepalive_enforcement_policy_min_time duration       gRPC server minimum keepalive time (default 10s)
       --grpc_server_keepalive_enforcement_policy_permit_without_stream   gRPC server permit client keepalive pings even when there are no active streams (RPCs)
   -h, --help                                                             display usage and exit
-      --jaeger-agent-host string                                         host and port to send spans to. if empty, no tracing will be done
       --keep_logs duration                                               keep logs for this long (using ctime) (zero to keep forever) (default 0s)
       --keep_logs_by_mtime duration                                      keep logs for this long (using mtime) (zero to keep forever) (default 0s)
       --lameduck-period duration                                         keep running at least this long after SIGTERM before stopping (default 50ms)
@@ -98,10 +95,6 @@ Usage of vtgr:
       --topo_zk_tls_ca string                                            the server ca to use to validate servers when connecting to the zk topo server
       --topo_zk_tls_cert string                                          the cert to use to connect to the zk topo server, requires topo_zk_tls_key, enables TLS
       --topo_zk_tls_key string                                           the key to use to connect to the zk topo server, enables TLS
-      --tracer string                                                    tracing service to use (default "noop")
-      --tracing-enable-logging                                           whether to enable logging in the tracing service
-      --tracing-sampling-rate OptionalFloat64                            sampling rate for the probabilistic jaeger sampler (default 0.1)
-      --tracing-sampling-type OptionalString                             sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote' (default const)
   -v, --v Level                                                          log level for V logs
       --version                                                          print binary version
       --vmodule moduleSpec                                               comma-separated list of pattern=N settings for file-filtered logging

go/flags/endtoend/vttablet.txt

@@ -431,8 +431,8 @@ Usage of vttablet:
       --topocustomrule_path string                                       path for customrules file. Disabled if empty.
       --tracer string                                                    tracing service to use (default "noop")
       --tracing-enable-logging                                           whether to enable logging in the tracing service
-      --tracing-sampling-rate OptionalFloat64                            sampling rate for the probabilistic jaeger sampler (default 0.1)
-      --tracing-sampling-type OptionalString                             sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote' (default const)
+      --tracing-sampling-rate float                                      sampling rate for the probabilistic jaeger sampler (default 0.1)
+      --tracing-sampling-type string                                     sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote' (default "const")
       --track_schema_versions                                            When enabled, vttablet will store versions of schemas at each position that a DDL is applied and allow retrieval of the schema corresponding to a position
       --transaction-log-stream-handler string                            URL handler for streaming transactions log (default "/debug/txlog")
       --transaction_limit_by_component                                   Include CallerID.component when considering who the user is for the purpose of transaction limit.

go/flagutil/optional.go

@@ -18,18 +18,19 @@ package flagutil
 
 import (
 	"errors"
-	"flag"
 	"strconv"
+
+	"github.com/spf13/pflag"
 )
 
-// OptionalFlag augements the flag.Value interface with a method to determine
+// OptionalFlag augements the pflag.Value interface with a method to determine
 // if a flag was set explicitly on the comand-line.
 //
 // Though not part of the interface, because the return type would be different
 // for each implementation, by convention, each implementation should define a
 // Get() method to access the underlying value.
 type OptionalFlag interface {
-	flag.Value
+	pflag.Value
 	IsSet() bool
 }
 
@@ -53,7 +54,7 @@ func NewOptionalFloat64(val float64) *OptionalFloat64 {
 	}
 }
 
-// Set is part of the flag.Value interface.
+// Set is part of the pflag.Value interface.
 func (f *OptionalFloat64) Set(arg string) error {
 	v, err := strconv.ParseFloat(arg, 64)
 	if err != nil {
@@ -66,11 +67,16 @@ func (f *OptionalFloat64) Set(arg string) error {
 	return nil
 }
 
-// String is part of the flag.Value interface.
+// String is part of the pflag.Value interface.
 func (f *OptionalFloat64) String() string {
 	return strconv.FormatFloat(f.val, 'g', -1, 64)
 }
 
+// Type is part of the pflag.Value interface.
+func (f *OptionalFloat64) Type() string {
+	return "float64"
+}
+
 // Get returns the underlying float64 value of this flag. If the flag was not
 // explicitly set, this will be the initial value passed to the constructor.
 func (f *OptionalFloat64) Get() float64 {
@@ -97,18 +103,23 @@ func NewOptionalString(val string) *OptionalString {
 	}
 }
 
-// Set is part of the flag.Value interface.
+// Set is part of the pflag.Value interface.
 func (f *OptionalString) Set(arg string) error {
 	f.val = arg
 	f.set = true
 	return nil
 }
 
-// String is part of the flag.Value interface.
+// String is part of the pflag.Value interface.
 func (f *OptionalString) String() string {
 	return f.val
 }
 
+// Type is part of the pflag.Value interface.
+func (f *OptionalString) Type() string {
+	return "string"
+}
+
 // Get returns the underlying string value of this flag. If the flag was not
 // explicitly set, this will be the initial value passed to the constructor.
 func (f *OptionalString) Get() string {

go/trace/fake.go

@@ -17,9 +17,8 @@ limitations under the License.
 package trace
 
 import (
-	"io"
-
 	"context"
+	"io"
 
 	"google.golang.org/grpc"
 )

go/trace/opentracing.go

@@ -17,11 +17,10 @@ limitations under the License.
 package trace
 
 import (
+	"context"
 	"encoding/base64"
 	"encoding/json"
 
-	"context"
-
 	otgrpc "github.com/opentracing-contrib/go-grpc"
 	"github.com/opentracing/opentracing-go"
 	"google.golang.org/grpc"

go/trace/plugin_datadog.go

@@ -1,33 +1,42 @@
 package trace
 
 import (
-	"flag"
 	"fmt"
 	"io"
 
 	"github.com/opentracing/opentracing-go"
+	"github.com/spf13/pflag"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/opentracer"
 	ddtracer "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
 )
 
 var (
-	dataDogHost = flag.String("datadog-agent-host", "", "host to send spans to. if empty, no tracing will be done")
-	dataDogPort = flag.String("datadog-agent-port", "", "port to send spans to. if empty, no tracing will be done")
+	dataDogHost string
+	dataDogPort string
 )
 
+func init() {
+	// If compiled with plugin_datadaog, ensure that trace.RegisterFlags
+	// includes datadaog tracing flags.
+	pluginFlags = append(pluginFlags, func(fs *pflag.FlagSet) {
+		fs.StringVar(&dataDogHost, "datadog-agent-host", "", "host to send spans to. if empty, no tracing will be done")
+		fs.StringVar(&dataDogPort, "datadog-agent-port", "", "port to send spans to. if empty, no tracing will be done")
+	})
+}
+
 func newDatadogTracer(serviceName string) (tracingService, io.Closer, error) {
-	if *dataDogHost == "" || *dataDogPort == "" {
+	if dataDogHost == "" || dataDogPort == "" {
 		return nil, nil, fmt.Errorf("need host and port to datadog agent to use datadog tracing")
 	}
 
 	opts := []ddtracer.StartOption{
-		ddtracer.WithAgentAddr(*dataDogHost + ":" + *dataDogPort),
+		ddtracer.WithAgentAddr(dataDogHost + ":" + dataDogPort),
 		ddtracer.WithServiceName(serviceName),
 		ddtracer.WithDebugMode(true),
 		ddtracer.WithSampler(ddtracer.NewRateSampler(samplingRate.Get())),
 	}
 
-	if *enableLogging {
+	if enableLogging {
 		opts = append(opts, ddtracer.WithLogger(&traceLogger{}))
 	}
 

go/trace/plugin_jaeger.go

@@ -17,11 +17,11 @@ limitations under the License.
 package trace
 
 import (
-	"flag"
 	"io"
 	"os"
 
 	"github.com/opentracing/opentracing-go"
+	"github.com/spf13/pflag"
 	"github.com/uber/jaeger-client-go"
 	"github.com/uber/jaeger-client-go/config"
 
@@ -36,14 +36,19 @@ included but nothing Jaeger specific.
 */
 
 var (
-	agentHost    = flag.String("jaeger-agent-host", "", "host and port to send spans to. if empty, no tracing will be done")
+	agentHost    string
 	samplingType = flagutil.NewOptionalString("const")
 	samplingRate = flagutil.NewOptionalFloat64(0.1)
 )
 
 func init() {
-	flag.Var(samplingType, "tracing-sampling-type", "sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote'")
-	flag.Var(samplingRate, "tracing-sampling-rate", "sampling rate for the probabilistic jaeger sampler")
+	// If compiled with plugin_jaeger, ensure that trace.RegisterFlags includes
+	// jaeger tracing flags.
+	pluginFlags = append(pluginFlags, func(fs *pflag.FlagSet) {
+		fs.StringVar(&agentHost, "jaeger-agent-host", "", "host and port to send spans to. if empty, no tracing will be done")
+		fs.Var(samplingType, "tracing-sampling-type", "sampling strategy to use for jaeger. possible values are 'const', 'probabilistic', 'rateLimiting', or 'remote'")
+		fs.Var(samplingRate, "tracing-sampling-rate", "sampling rate for the probabilistic jaeger sampler")
+	})
 }
 
 // newJagerTracerFromEnv will instantiate a tracingService implemented by Jaeger,
@@ -74,8 +79,8 @@ func newJagerTracerFromEnv(serviceName string) (tracingService, io.Closer, error
 	}
 
 	// Allow command line args to override environment variables.
-	if *agentHost != "" {
-		cfg.Reporter.LocalAgentHostPort = *agentHost
+	if agentHost != "" {
+		cfg.Reporter.LocalAgentHostPort = agentHost
 	}
 	log.Infof("Tracing to: %v as %v", cfg.Reporter.LocalAgentHostPort, cfg.ServiceName)
 
@@ -99,7 +104,7 @@ func newJagerTracerFromEnv(serviceName string) (tracingService, io.Closer, error
 	log.Infof("Tracing sampler type %v (param: %v)", cfg.Sampler.Type, cfg.Sampler.Param)
 
 	var opts []config.Option
-	if *enableLogging {
+	if enableLogging {
 		opts = append(opts, config.Logger(&traceLogger{}))
 	} else if cfg.Reporter.LogSpans {
 		log.Warningf("JAEGER_REPORTER_LOG_SPANS was set, but --tracing-enable-logging was not; spans will not be logged")