From cd9d85217cf45ce78befaf8203f8c2cce029aa47 Mon Sep 17 00:00:00 2001 From: Omer Aplatony Date: Sun, 22 Dec 2024 17:10:24 +0200 Subject: [PATCH] migrate distributed tracing flags off experimental prefix Signed-off-by: Omer Aplatony --- server/embed/config.go | 53 ++++++++--- server/etcdmain/config.go | 12 +++ server/etcdmain/config_test.go | 168 +++++++++++++++++++++++++++++++++ 3 files changed, 218 insertions(+), 15 deletions(-) diff --git a/server/embed/config.go b/server/embed/config.go index 3a382056834..55ff881d9e5 100644 --- a/server/embed/config.go +++ b/server/embed/config.go @@ -133,7 +133,11 @@ var ( // This is the mapping from the non boolean `experimental-` to the new flags. // TODO: delete in v3.7 experimentalNonBoolFlagMigrationMap = map[string]string{ - "experimental-compact-hash-check-time": "compact-hash-check-time", + "experimental-compact-hash-check-time": "compact-hash-check-time", + "experimental-distributed-tracing-address": "distributed-tracing-address", + "experimental-distributed-tracing-service-name": "distributed-tracing-service-name", + "experimental-distributed-tracing-instance-id": "distributed-tracing-instance-id", + "experimental-distributed-tracing-sampling-rate": "distributed-tracing-sampling-rate", } ) @@ -408,23 +412,33 @@ type Config struct { ListenMetricsUrls []url.URL ListenMetricsUrlsJSON string `json:"listen-metrics-urls"` - // ExperimentalEnableDistributedTracing indicates if experimental tracing using OpenTelemetry is enabled. + // ExperimentalEnableDistributedTracing enables distributed tracing using OpenTelemetry protocol. + // Deprecated in v3.6 and will be decommissioned in v3.7. + // TODO: delete in v3.7 ExperimentalEnableDistributedTracing bool `json:"experimental-enable-distributed-tracing"` - // ExperimentalDistributedTracingAddress is the address of the OpenTelemetry Collector. - // Can only be set if ExperimentalEnableDistributedTracing is true. + // ExperimentalDistributedTracingAddress is the address of OpenTelemetry collector. + // Deprecated in v3.6 and will be decommissioned in v3.7. + // TODO: delete in v3.7 ExperimentalDistributedTracingAddress string `json:"experimental-distributed-tracing-address"` // ExperimentalDistributedTracingServiceName is the name of the service. - // Can only be used if ExperimentalEnableDistributedTracing is true. + // Deprecated in v3.6 and will be decommissioned in v3.7. + // TODO: delete in v3.7 ExperimentalDistributedTracingServiceName string `json:"experimental-distributed-tracing-service-name"` // ExperimentalDistributedTracingServiceInstanceID is the ID key of the service. - // This ID must be unique, as helps to distinguish instances of the same service - // that exist at the same time. - // Can only be used if ExperimentalEnableDistributedTracing is true. + // Deprecated in v3.6 and will be decommissioned in v3.7. + // TODO: delete in v3.7 ExperimentalDistributedTracingServiceInstanceID string `json:"experimental-distributed-tracing-instance-id"` - // ExperimentalDistributedTracingSamplingRatePerMillion is the number of samples to collect per million spans. - // Defaults to 0. + // ExperimentalDistributedTracingSamplingRatePerMillion is the number of samples. + // Deprecated in v3.6 and will be decommissioned in v3.7. + // TODO: delete in v3.7 ExperimentalDistributedTracingSamplingRatePerMillion int `json:"experimental-distributed-tracing-sampling-rate"` + EnableDistributedTracing bool `json:"enable-distributed-tracing"` + DistributedTracingAddress string `json:"distributed-tracing-address"` + DistributedTracingServiceName string `json:"distributed-tracing-service-name"` + DistributedTracingServiceInstanceID string `json:"distributed-tracing-instance-id"` + DistributedTracingSamplingRatePerMillion int `json:"distributed-tracing-sampling-rate"` + // Logger is logger options: currently only supports "zap". // "capnslog" is removed in v3.5. Logger string `json:"logger"` @@ -654,6 +668,11 @@ func (cfg *Config) AddFlags(fs *flag.FlagSet) { fs.DurationVar(&cfg.GRPCKeepAliveTimeout, "grpc-keepalive-timeout", cfg.GRPCKeepAliveTimeout, "Additional duration of wait before closing a non-responsive connection (0 to disable).") fs.BoolVar(&cfg.SocketOpts.ReusePort, "socket-reuse-port", cfg.SocketOpts.ReusePort, "Enable to set socket option SO_REUSEPORT on listeners allowing rebinding of a port already in use.") fs.BoolVar(&cfg.SocketOpts.ReuseAddress, "socket-reuse-address", cfg.SocketOpts.ReuseAddress, "Enable to set socket option SO_REUSEADDR on listeners allowing binding to an address in `TIME_WAIT` state.") + fs.BoolVar(&cfg.EnableDistributedTracing, "enable-distributed-tracing", false, "Enable distributed tracing using OpenTelemetry protocol") + fs.StringVar(&cfg.DistributedTracingAddress, "distributed-tracing-address", ExperimentalDistributedTracingAddress, "Address for distributed tracing used for OpenTelemetry Tracing") + fs.StringVar(&cfg.DistributedTracingServiceName, "distributed-tracing-service-name", ExperimentalDistributedTracingServiceName, "Configures service name for distributed tracing") + fs.StringVar(&cfg.DistributedTracingServiceInstanceID, "distributed-tracing-instance-id", "", "Configures service instance ID for distributed tracing") + fs.IntVar(&cfg.DistributedTracingSamplingRatePerMillion, "distributed-tracing-sampling-rate", 0, "Number of samples to collect per million spans") fs.Var(flags.NewUint32Value(cfg.MaxConcurrentStreams), "max-concurrent-streams", "Maximum concurrent streams that each client can open at a time.") @@ -755,11 +774,11 @@ func (cfg *Config) AddFlags(fs *flag.FlagSet) { fs.StringVar(&cfg.Metrics, "metrics", cfg.Metrics, "Set level of detail for exported metrics, specify 'extensive' to include server side grpc histogram metrics") // experimental distributed tracing - fs.BoolVar(&cfg.ExperimentalEnableDistributedTracing, "experimental-enable-distributed-tracing", false, "Enable experimental distributed tracing using OpenTelemetry Tracing.") - fs.StringVar(&cfg.ExperimentalDistributedTracingAddress, "experimental-distributed-tracing-address", ExperimentalDistributedTracingAddress, "Address for distributed tracing used for OpenTelemetry Tracing (if enabled with experimental-enable-distributed-tracing flag).") - fs.StringVar(&cfg.ExperimentalDistributedTracingServiceName, "experimental-distributed-tracing-service-name", ExperimentalDistributedTracingServiceName, "Configures service name for distributed tracing to be used to define service name for OpenTelemetry Tracing (if enabled with experimental-enable-distributed-tracing flag). 'etcd' is the default service name. Use the same service name for all instances of etcd.") - fs.StringVar(&cfg.ExperimentalDistributedTracingServiceInstanceID, "experimental-distributed-tracing-instance-id", "", "Configures service instance ID for distributed tracing to be used to define service instance ID key for OpenTelemetry Tracing (if enabled with experimental-enable-distributed-tracing flag). There is no default value set. This ID must be unique per etcd instance.") - fs.IntVar(&cfg.ExperimentalDistributedTracingSamplingRatePerMillion, "experimental-distributed-tracing-sampling-rate", 0, "Number of samples to collect per million spans for OpenTelemetry Tracing (if enabled with experimental-enable-distributed-tracing flag).") + fs.BoolVar(&cfg.ExperimentalEnableDistributedTracing, "experimental-enable-distributed-tracing", false, "Enable distributed tracing using OpenTelemetry protocol. "+"Deprecated in v3.6 and will be decommissioned in v3.7. Use --enable-distributed-tracing instead.") + fs.StringVar(&cfg.ExperimentalDistributedTracingAddress, "experimental-distributed-tracing-address", ExperimentalDistributedTracingAddress, "Address for distributed tracing. Deprecated in v3.6 and will be decommissioned in v3.7. Use --distributed-tracing-address instead.") + fs.StringVar(&cfg.ExperimentalDistributedTracingServiceName, "experimental-distributed-tracing-service-name", ExperimentalDistributedTracingServiceName, "Service name for distributed tracing. Deprecated in v3.6 and will be decommissioned in v3.7. Use --distributed-tracing-service-name instead.") + fs.StringVar(&cfg.ExperimentalDistributedTracingServiceInstanceID, "experimental-distributed-tracing-instance-id", "", "Service instance ID for distributed tracing. Deprecated in v3.6 and will be decommissioned in v3.7. Use --distributed-tracing-instance-id instead.") + fs.IntVar(&cfg.ExperimentalDistributedTracingSamplingRatePerMillion, "experimental-distributed-tracing-sampling-rate", 0, "Samples per million for distributed tracing. Deprecated in v3.6 and will be decommissioned in v3.7. Use --distributed-tracing-sampling-rate instead.") // auth fs.StringVar(&cfg.AuthToken, "auth-token", cfg.AuthToken, "Specify auth token specific options.") @@ -1012,6 +1031,10 @@ func (cfg *Config) Validate() error { } } + if cfg.FlagsExplicitlySet["experimental-enable-distributed-tracing"] && cfg.FlagsExplicitlySet["enable-distributed-tracing"] { + return fmt.Errorf("cannot set --experimental-enable-distributed-tracing and --enable-distributed-tracing at the same time") + } + if err := cfg.setupLogging(); err != nil { return err } diff --git a/server/etcdmain/config.go b/server/etcdmain/config.go index ca651e1669c..0ca3ed672e7 100644 --- a/server/etcdmain/config.go +++ b/server/etcdmain/config.go @@ -65,6 +65,11 @@ var ( "experimental-compact-hash-check-enabled": "--experimental-compact-hash-check-enabled is deprecated in 3.6 and will be decommissioned in 3.7. Use '--feature-gates=CompactHashCheck=true' instead.", "experimental-compact-hash-check-time": "--experimental-compact-hash-check-time is deprecated in 3.6 and will be decommissioned in 3.7. Use '--compact-hash-check-time' instead.", "experimental-txn-mode-write-with-shared-buffer": "--experimental-txn-mode-write-with-shared-buffer is deprecated in v3.6 and will be decommissioned in v3.7. Use '--feature-gates=TxnModeWriteWithSharedBuffer=true' instead.", + "experimental-enable-distributed-tracing": "--experimental-enable-distributed-tracing is deprecated in 3.6 and will be decommissioned in 3.7. Use --enable-distributed-tracing instead.", + "experimental-distributed-tracing-address": "--experimental-distributed-tracing-address is deprecated in 3.6 and will be decommissioned in 3.7. Use --distributed-tracing-address instead.", + "experimental-distributed-tracing-service-name": "--experimental-distributed-tracing-service-name is deprecated in 3.6 and will be decommissioned in 3.7. Use --distributed-tracing-service-name instead.", + "experimental-distributed-tracing-instance-id": "--experimental-distributed-tracing-instance-id is deprecated in 3.6 and will be decommissioned in 3.7. Use --distributed-tracing-instance-id instead.", + "experimental-distributed-tracing-sampling-rate": "--experimental-distributed-tracing-sampling-rate is deprecated in 3.6 and will be decommissioned in 3.7. Use --distributed-tracing-sampling-rate instead.", } ) @@ -195,6 +200,13 @@ func (cfg *config) parse(arguments []string) error { } } } + if cfg.ec.ExperimentalEnableDistributedTracing { + cfg.ec.EnableDistributedTracing = true + cfg.ec.DistributedTracingAddress = cfg.ec.ExperimentalDistributedTracingAddress + cfg.ec.DistributedTracingServiceName = cfg.ec.ExperimentalDistributedTracingServiceName + cfg.ec.DistributedTracingServiceInstanceID = cfg.ec.ExperimentalDistributedTracingServiceInstanceID + cfg.ec.DistributedTracingSamplingRatePerMillion = cfg.ec.ExperimentalDistributedTracingSamplingRatePerMillion + } // now logger is set up return err diff --git a/server/etcdmain/config_test.go b/server/etcdmain/config_test.go index b834f3d33ea..7faefef60ad 100644 --- a/server/etcdmain/config_test.go +++ b/server/etcdmain/config_test.go @@ -653,3 +653,171 @@ func validateClusteringFlags(t *testing.T, cfg *config) { t.Errorf("advertise-client-urls = %v, want %v", cfg.ec.AdvertiseClientUrls, wcfg.ec.AdvertiseClientUrls) } } + +// TODO: delete in v3.7 +func TestDistributedTracingFlagMigration(t *testing.T) { + testCases := []struct { + name string + args []string // command line arguments + yamlConfig string // YAML configuration content + expectValues func(*config) error // function to verify expected values + expectErr bool + }{ + { + name: "use experimental flags", + args: []string{ + "--experimental-enable-distributed-tracing=true", + "--experimental-distributed-tracing-address=localhost:4318", + "--experimental-distributed-tracing-service-name=test-service", + "--experimental-distributed-tracing-instance-id=instance-1", + "--experimental-distributed-tracing-sampling-rate=1000", + }, + expectValues: func(cfg *config) error { + if !cfg.ec.EnableDistributedTracing { + return fmt.Errorf("expected EnableDistributedTracing to be true") + } + if cfg.ec.DistributedTracingAddress != "localhost:4318" { + return fmt.Errorf("expected DistributedTracingAddress to be localhost:4318, got %s", cfg.ec.DistributedTracingAddress) + } + if cfg.ec.DistributedTracingServiceName != "test-service" { + return fmt.Errorf("expected DistributedTracingServiceName to be test-service, got %s", cfg.ec.DistributedTracingServiceName) + } + if cfg.ec.DistributedTracingServiceInstanceID != "instance-1" { + return fmt.Errorf("expected DistributedTracingServiceInstanceID to be instance-1, got %s", cfg.ec.DistributedTracingServiceInstanceID) + } + if cfg.ec.DistributedTracingSamplingRatePerMillion != 1000 { + return fmt.Errorf("expected DistributedTracingSamplingRatePerMillion to be 1000, got %d", cfg.ec.DistributedTracingSamplingRatePerMillion) + } + return nil + }, + }, + { + name: "use regular flags", + args: []string{ + "--enable-distributed-tracing=true", + "--distributed-tracing-address=localhost:4319", + "--distributed-tracing-service-name=new-service", + "--distributed-tracing-instance-id=instance-2", + "--distributed-tracing-sampling-rate=2000", + }, + expectValues: func(cfg *config) error { + if !cfg.ec.EnableDistributedTracing { + return fmt.Errorf("expected EnableDistributedTracing to be true") + } + if cfg.ec.DistributedTracingAddress != "localhost:4319" { + return fmt.Errorf("expected DistributedTracingAddress to be localhost:4319, got %s", cfg.ec.DistributedTracingAddress) + } + if cfg.ec.DistributedTracingServiceName != "new-service" { + return fmt.Errorf("expected DistributedTracingServiceName to be new-service, got %s", cfg.ec.DistributedTracingServiceName) + } + if cfg.ec.DistributedTracingServiceInstanceID != "instance-2" { + return fmt.Errorf("expected DistributedTracingServiceInstanceID to be instance-2, got %s", cfg.ec.DistributedTracingServiceInstanceID) + } + if cfg.ec.DistributedTracingSamplingRatePerMillion != 2000 { + return fmt.Errorf("expected DistributedTracingSamplingRatePerMillion to be 2000, got %d", cfg.ec.DistributedTracingSamplingRatePerMillion) + } + return nil + }, + }, + { + name: "config file with experimental flags", + yamlConfig: ` +experimental-enable-distributed-tracing: true +experimental-distributed-tracing-address: "localhost:4318" +experimental-distributed-tracing-service-name: "test-service" +experimental-distributed-tracing-instance-id: "instance-1" +experimental-distributed-tracing-sampling-rate: 1000 +`, + expectValues: func(cfg *config) error { + if !cfg.ec.EnableDistributedTracing { + return fmt.Errorf("expected EnableDistributedTracing to be true") + } + if cfg.ec.DistributedTracingAddress != "localhost:4318" { + return fmt.Errorf("expected DistributedTracingAddress to be localhost:4318, got %s", cfg.ec.DistributedTracingAddress) + } + if cfg.ec.DistributedTracingServiceName != "test-service" { + return fmt.Errorf("expected DistributedTracingServiceName to be test-service, got %s", cfg.ec.DistributedTracingServiceName) + } + if cfg.ec.DistributedTracingServiceInstanceID != "instance-1" { + return fmt.Errorf("expected DistributedTracingServiceInstanceID to be instance-1, got %s", cfg.ec.DistributedTracingServiceInstanceID) + } + if cfg.ec.DistributedTracingSamplingRatePerMillion != 1000 { + return fmt.Errorf("expected DistributedTracingSamplingRatePerMillion to be 1000, got %d", cfg.ec.DistributedTracingSamplingRatePerMillion) + } + return nil + }, + }, + { + name: "config file with regular flags", + yamlConfig: ` +enable-distributed-tracing: true +distributed-tracing-address: "localhost:4319" +distributed-tracing-service-name: "new-service" +distributed-tracing-instance-id: "instance-2" +distributed-tracing-sampling-rate: 2000 +`, + expectValues: func(cfg *config) error { + if !cfg.ec.EnableDistributedTracing { + return fmt.Errorf("expected EnableDistributedTracing to be true") + } + if cfg.ec.DistributedTracingAddress != "localhost:4319" { + return fmt.Errorf("expected DistributedTracingAddress to be localhost:4319, got %s", cfg.ec.DistributedTracingAddress) + } + if cfg.ec.DistributedTracingServiceName != "new-service" { + return fmt.Errorf("expected DistributedTracingServiceName to be new-service, got %s", cfg.ec.DistributedTracingServiceName) + } + if cfg.ec.DistributedTracingServiceInstanceID != "instance-2" { + return fmt.Errorf("expected DistributedTracingServiceInstanceID to be instance-2, got %s", cfg.ec.DistributedTracingServiceInstanceID) + } + if cfg.ec.DistributedTracingSamplingRatePerMillion != 2000 { + return fmt.Errorf("expected DistributedTracingSamplingRatePerMillion to be 2000, got %d", cfg.ec.DistributedTracingSamplingRatePerMillion) + } + return nil + }, + }, + { + name: "config file with both flags is error", + yamlConfig: ` +experimental-enable-distributed-tracing: true +enable-distributed-tracing: true +`, + expectErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cfg := newConfig() + + if tc.yamlConfig != "" { + tmpfile, err := os.CreateTemp("", "etcd-test-*.yaml") + if err != nil { + t.Fatal(err) + } + defer os.Remove(tmpfile.Name()) + + if err := os.WriteFile(tmpfile.Name(), []byte(tc.yamlConfig), 0600); err != nil { + t.Fatal(err) + } + + tc.args = []string{"--config-file=" + tmpfile.Name()} + } + + err := cfg.parse(tc.args) + if tc.expectErr { + if err == nil { + t.Error("expected error but got nil") + } + return + } + + if err != nil { + t.Fatal(err) + } + + if err := tc.expectValues(cfg); err != nil { + t.Error(err) + } + }) + } +}