[opampsupervisor] remove health check port (open-telemetry#39908)

Author: TylerHelmuth

.chloggen/opampsupervisor-remove-health-check-port.yaml

@@ -0,0 +1,27 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: breaking
+
+# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
+component: opampsupervisor
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Remnove `agent.health_check_port`.
+
+# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
+issues: [39908]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: The opampsupervisor no longer starts the collector with a default health check extension.
+
+# If your change doesn't affect end users or the exported elements of any package,
+# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: []

cmd/opampsupervisor/e2e_test.go

@@ -292,21 +292,17 @@ func TestSupervisorStartsCollectorWithNoOpAMPServerWithNoLastRemoteConfig(t *tes
 		},
 	})
 
-	healthcheckPort, err := findRandomPort()
-	require.NoError(t, err)
-
 	s := newSupervisor(t, "healthcheck_port", map[string]string{
-		"url":              server.addr,
-		"storage_dir":      storageDir,
-		"healthcheck_port": fmt.Sprintf("%d", healthcheckPort),
-		"local_config":     filepath.Join("testdata", "collector", "nop_config.yaml"),
+		"url":          server.addr,
+		"storage_dir":  storageDir,
+		"local_config": filepath.Join("testdata", "collector", "healthcheck_config.yaml"),
 	})
 	t.Cleanup(s.Shutdown)
 	require.Nil(t, s.Start())
 
 	// Verify the collector runs eventually by pinging the healthcheck extension
 	require.Eventually(t, func() bool {
-		resp, err := http.DefaultClient.Get(fmt.Sprintf("http://localhost:%d", healthcheckPort))
+		resp, err := http.DefaultClient.Get("http://localhost:13133")
 		if err != nil {
 			t.Logf("Failed healthcheck: %s", err)
 			return false
@@ -1118,20 +1114,16 @@ func createHealthCheckCollectorConf(t *testing.T) (cfg *bytes.Buffer, hash []byt
 	templ, err := template.New("").Parse(string(colCfgTpl))
 	require.NoError(t, err)
 
-	port, err := findRandomPort()
-
 	var confmapBuf bytes.Buffer
 	err = templ.Execute(
 		&confmapBuf,
-		map[string]string{
-			"HealthCheckEndpoint": fmt.Sprintf("localhost:%d", port),
-		},
+		map[string]string{},
 	)
 	require.NoError(t, err)
 
 	h := sha256.Sum256(confmapBuf.Bytes())
 
-	return &confmapBuf, h[:], port
+	return &confmapBuf, h[:], 13133
 }
 
 // Wait for the Supervisor to connect to or disconnect from the OpAMP server
@@ -1593,8 +1585,7 @@ func TestSupervisorStopsAgentProcessWithEmptyConfigMap(t *testing.T) {
 		})
 
 	s := newSupervisor(t, "healthcheck_port", map[string]string{
-		"url":              server.addr,
-		"healthcheck_port": "12345",
+		"url": server.addr,
 	})
 
 	require.Nil(t, s.Start())
@@ -1623,7 +1614,7 @@ func TestSupervisorStopsAgentProcessWithEmptyConfigMap(t *testing.T) {
 
 	// Use health check endpoint to determine if the collector is actually running
 	require.Eventually(t, func() bool {
-		resp, err := http.DefaultClient.Get("http://localhost:12345")
+		resp, err := http.DefaultClient.Get("http://localhost:13133")
 		if err != nil {
 			t.Logf("Failed agent healthcheck request: %s", err)
 			return false

cmd/opampsupervisor/specification/README.md

@@ -167,10 +167,8 @@ agent:
     non_identifying_attributes:
       custom.attribute: "custom-value"
 
-  # The port the Collector's health check extension will be configured to use
-  health_check_port:
 
-  # The port the Supervisor will start its OpAmp server on and the Collector's
+  # The port the Supervisor will start its OpAmp server on and the Collector's 
   # OpAmp extension will connect to
   opamp_server_port:
 ```

cmd/opampsupervisor/supervisor/config/config.go

@@ -195,7 +195,6 @@ type Agent struct {
 	Description             AgentDescription  `mapstructure:"description"`
 	ConfigApplyTimeout      time.Duration     `mapstructure:"config_apply_timeout"`
 	BootstrapTimeout        time.Duration     `mapstructure:"bootstrap_timeout"`
-	HealthCheckPort         int               `mapstructure:"health_check_port"`
 	OpAMPServerPort         int               `mapstructure:"opamp_server_port"`
 	PassthroughLogs         bool              `mapstructure:"passthrough_logs"`
 	ConfigFiles             []string          `mapstructure:"config_files"`
@@ -213,10 +212,6 @@ func (a Agent) Validate() error {
 		return errors.New("agent::bootstrap_timeout must be positive")
 	}
 
-	if a.HealthCheckPort < 0 || a.HealthCheckPort > 65535 {
-		return errors.New("agent::health_check_port must be a valid port number")
-	}
-
 	if a.OpAMPServerPort < 0 || a.OpAMPServerPort > 65535 {
 		return errors.New("agent::opamp_server_port must be a valid port number")
 	}

cmd/opampsupervisor/supervisor/config/config_test.go

@@ -226,32 +226,6 @@ func TestValidate(t *testing.T) {
 			},
 			expectedError: "agent::orphan_detection_interval must be positive",
 		},
-		{
-			name: "Invalid health check port number",
-			config: Supervisor{
-				Server: OpAMPServer{
-					Endpoint: "wss://localhost:9090/opamp",
-					Headers: http.Header{
-						"Header1": []string{"HeaderValue"},
-					},
-					TLSSetting: tlsConfig,
-				},
-				Agent: Agent{
-					Executable:              "${file_path}",
-					OrphanDetectionInterval: 5 * time.Second,
-					HealthCheckPort:         65536,
-					ConfigApplyTimeout:      2 * time.Second,
-					BootstrapTimeout:        5 * time.Second,
-				},
-				Capabilities: Capabilities{
-					AcceptsRemoteConfig: true,
-				},
-				Storage: Storage{
-					Directory: "/etc/opamp-supervisor/storage",
-				},
-			},
-			expectedError: "agent::health_check_port must be a valid port number",
-		},
 		{
 			name: "Zero value health check port number",
 			config: Supervisor{
@@ -265,7 +239,6 @@ func TestValidate(t *testing.T) {
 				Agent: Agent{
 					Executable:              "${file_path}",
 					OrphanDetectionInterval: 5 * time.Second,
-					HealthCheckPort:         0,
 					ConfigApplyTimeout:      2 * time.Second,
 					BootstrapTimeout:        5 * time.Second,
 				},
@@ -290,7 +263,6 @@ func TestValidate(t *testing.T) {
 				Agent: Agent{
 					Executable:              "${file_path}",
 					OrphanDetectionInterval: 5 * time.Second,
-					HealthCheckPort:         29848,
 					ConfigApplyTimeout:      2 * time.Second,
 					BootstrapTimeout:        5 * time.Second,
 				},
@@ -564,7 +536,6 @@ agent:
   orphan_detection_interval: 10s
   config_apply_timeout: 8s
   bootstrap_timeout: 8s
-  health_check_port: 8089
   opamp_server_port: 8090
   passthrough_logs: true
 
@@ -609,7 +580,6 @@ telemetry:
 						OrphanDetectionInterval: 10 * time.Second,
 						ConfigApplyTimeout:      8 * time.Second,
 						BootstrapTimeout:        8 * time.Second,
-						HealthCheckPort:         8089,
 						OpAMPServerPort:         8090,
 						PassthroughLogs:         true,
 						Signature:               DefaultSupervisor().Agent.Signature,

cmd/opampsupervisor/supervisor/healthchecker/healthchecker.go

@@ -53,7 +53,6 @@ import (
 
 	"github.com/open-telemetry/opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/commander"
 	"github.com/open-telemetry/opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/config"
-	"github.com/open-telemetry/opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/healthchecker"
 )
 
 var (
@@ -116,8 +115,6 @@ type Supervisor struct {
 
 	startedAt time.Time
 
-	healthChecker *healthchecker.HTTPHealthChecker
-
 	// Supervisor's own config.
 	config config.Supervisor
 
@@ -143,10 +140,6 @@ type Supervisor struct {
 	// https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/21078
 	agentConfigOwnMetricsSection *atomic.Value
 
-	// agentHealthCheckEndpoint is the endpoint the Collector's health check extension
-	// will listen on for health check requests from the Supervisor.
-	agentHealthCheckEndpoint string
-
 	// Internal config state for agent use. See the [configState] struct for more details.
 	cfgState *atomic.Value
 
@@ -329,14 +322,6 @@ func (s *Supervisor) Start() error {
 		return fmt.Errorf("could not get bootstrap info from the Collector: %w", err)
 	}
 
-	healthCheckPort := s.config.Agent.HealthCheckPort
-	if healthCheckPort == 0 {
-		healthCheckPort, err = s.findRandomPort()
-		if err != nil {
-			return fmt.Errorf("could not find port for health check: %w", err)
-		}
-	}
-
 	s.opampServerPort, err = s.getSupervisorOpAMPServerPort()
 	if err != nil {
 		return fmt.Errorf("get opamp server port: %w", err)
@@ -360,8 +345,6 @@ func (s *Supervisor) Start() error {
 		return fmt.Errorf("could not find port for health check: %w", err)
 	}
 
-	s.agentHealthCheckEndpoint = fmt.Sprintf("localhost:%d", healthCheckPort)
-
 	s.telemetrySettings.Logger.Info("Supervisor starting",
 		zap.String("id", s.persistentState.InstanceID.String()))
 
@@ -1016,7 +999,6 @@ func (s *Supervisor) composeExtraLocalConfig() []byte {
 		resourceAttrs[attr.Key] = attr.Value.GetStringValue()
 	}
 	tplVars := map[string]any{
-		"Healthcheck":        s.agentHealthCheckEndpoint,
 		"ResourceAttributes": resourceAttrs,
 		"SupervisorPort":     s.opampServerPort,
 	}
@@ -1347,7 +1329,6 @@ func (s *Supervisor) startAgentCommand() (agentStartStatus, error) {
 	s.agentStartHealthCheckAttempts = 0
 	s.startedAt = time.Now()
 
-	s.healthChecker = healthchecker.NewHTTPHealthChecker(fmt.Sprintf("http://%s", s.agentHealthCheckEndpoint))
 	return agentStarting, nil
 }