migrate distributed tracing flags off experimental prefix

Signed-off-by: Omer Aplatony <[email protected]>

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
 	}

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

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)
+			}
+		})
+	}
+}