[cmd/supervisor] Fix config merge and opamp server start order

[douglascamata]

Description

This PR fixes a bug that happens because of broken logic in the start up process when there is no explicit OpAMP server in the Supervisor's agent config. Currently it works like this:

1. The bootstrap happens, which starts the opamp server in the Supervisor using a random port. This leaves the port saved in Supervisor:

opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/supervisor.go

Lines 432 to 435 in a0044fb

	func (s *Supervisor) getBootstrapInfo() (err error) {
	_, span := s.getTracer().Start(context.Background(), "GetBootstrapInfo")
	defer span.End()
	s.opampServerPort, err = s.getSupervisorOpAMPServerPort()

2. The initial merged configuration is written to a file. It uses the opamp server port already saved in the Supervisor.

3. The "real" opamp server is started using a different random port, leading the Collector to be constantly restarted by the Supervisor because it never connects back via OpAMP.

opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/supervisor.go

Lines 713 to 720 in 929656d

	func (s *Supervisor) startOpAMPServer() error {
	s.opampServer = server.New(newLoggerFromZap(s.telemetrySettings.Logger, "opamp-server"))
	var err error
	s.opampServerPort, err = s.getSupervisorOpAMPServerPort()
	if err != nil {
	return err
	}

Testing

Added an e2e to cover this.

[evan-bradley]

Thank you for digging into this. The issue you point out and your fix both seem valid to me, but I'm not understanding why this doesn't happen when I run the Supervisor against the example server, or in any of the other E2E tests. What conditions trigger this error?

[evan-bradley]

If this is related to starting the Supervisor with only a local config and not with a remote config, could we also check the pipeline stared by the Collector by reading the files returned by createSimplePipelineCollectorConf?

[douglascamata]

Sure, will fix this.

[douglascamata]

Fixed in 54ee558 (#39949)

[evan-bradley]

Is there another way we can verify that the Collector has successfully connected to the Supervisor? I'm worried this check is a bit too broad.

[douglascamata]

@evan-bradley I thought the same as I was developing it. The only alternative way I could think to check whether the Collector connected back would require me to export a method in the Supervisor struct only to be able to use it in the e2e test. If that's okay for you I am happy to go this way.

[douglascamata]

I could improve this test without adding an extra method to the Supervisor struct in 5c21464 (#39949).

Now it's checking for the presence of the line Connected to the OpAMP server, which is there if the Collector log level is set to debug (which I now configure in the simple pipeline conf used by the test).

Let me know please whether this is good or if you prefer the exported method in the Supervisor struct.

[douglascamata]

Thank you for digging into this. The issue you point out and your fix both seem valid to me, but I'm not understanding why this doesn't happen when I run the Supervisor against the example server, or in any of the other E2E tests. What conditions trigger this error?

@evan-bradley I think what triggers this is the presence of a local configuration that is enough for the Supervisor to start the Collector on the first attempt, inside Supervisor.Start. This scenario wasn't covered before by any other e2e test. Even if it was covered, we didn't have any check confirming that the Collector could connect back to the Supervisor.

This bug doesn't happen in the scenario where no local configuration is present because the Supervisor would have to receive some remote configuration first and this path correctly composes and writes the effective configuration to the file before starting the Collector process (see lines 1342 and 1343 below):

opentelemetry-collector-contrib/cmd/opampsupervisor/supervisor/supervisor.go

Lines 1330 to 1343 in c2c11e6

	case <-s.hasNewConfig:
	s.lastHealthFromClient = nil
	if !configApplyTimeoutTimer.Stop() {
	select {
	case <-configApplyTimeoutTimer.C: // Try to drain the channel
	default:
	}
	}
	configApplyTimeoutTimer.Reset(s.config.Agent.ConfigApplyTimeout)
	s.telemetrySettings.Logger.Debug("Restarting agent due to new config")
	restartTimer.Stop()
	s.stopAgentApplyConfig()
	status, err := s.startAgent()