Repository: open-telemetry/opamp-go
Pull request #51 (2022-01-11 03:03:17)

Avoid sending status update if the effective config is unchanged
This change implements the following OpAMP Spec requirement
https://github.com/open-telemetry/opamp-spec/blob/main/specification.md#status-reporting:

When the Agent receives a ServerToAgent message the Agent MUST NOT
send a status report unless processing of the message received from
the Server resulted in actual change of the Agent status (e.g. the
configuration of the Agent has changed).

Before this implementation the agent would unnecessarily send status
updates even though they were unnecessary since nothing was changed.


Patch:
diff --git a/client/callbacks.go b/client/callbacks.go
index 60c02b3..6e19eef 100644
--- a/client/callbacks.go
+++ b/client/callbacks.go
@@ -14,8 +14,8 @@ type CallbacksStruct struct {
 
 	OnRemoteConfigFunc func(
 		ctx context.Context,
-		config *protobufs.AgentRemoteConfig,
-	) (*protobufs.EffectiveConfig, error)
+		remoteConfig *protobufs.AgentRemoteConfig,
+	) (effectiveConfig *protobufs.EffectiveConfig, configChanged bool, err error)
 
 	OnOpampConnectionSettingsFunc func(
 		ctx context.Context,
@@ -62,12 +62,13 @@ func (c CallbacksStruct) OnError(err *protobufs.ServerErrorResponse) {
 }
 
 func (c CallbacksStruct) OnRemoteConfig(
-	ctx context.Context, remoteConfig *protobufs.AgentRemoteConfig,
-) (*protobufs.EffectiveConfig, error) {
+	ctx context.Context,
+	remoteConfig *protobufs.AgentRemoteConfig,
+) (effectiveConfig *protobufs.EffectiveConfig, configChanged bool, err error) {
 	if c.OnRemoteConfigFunc != nil {
 		return c.OnRemoteConfigFunc(ctx, remoteConfig)
 	}
-	return nil, nil
+	return nil, false, nil
 }
 
 func (c CallbacksStruct) OnOpampConnectionSettings(
diff --git a/client/internal/receiver.go b/client/internal/receiver.go
index f21889a..42cda88 100644
--- a/client/internal/receiver.go
+++ b/client/internal/receiver.go
@@ -61,7 +61,7 @@ func (r *Receiver) receiveMessage(msg *protobufs.ServerToAgent) error {
 
 func (r *Receiver) processReceivedMessage(ctx context.Context, msg *protobufs.ServerToAgent) {
 	if r.callbacks != nil {
-		reportStatus := r.rcvRemoteConfig(ctx, msg.RemoteConfig)
+		reportStatus := r.rcvRemoteConfig(ctx, msg.RemoteConfig, msg.Flags)
 
 		r.rcvConnectionSettings(ctx, msg.ConnectionSettings)
 		r.rcvAddonsAvailable(msg.AddonsAvailable)
@@ -77,15 +77,25 @@ func (r *Receiver) processReceivedMessage(ctx context.Context, msg *protobufs.Se
 	}
 }
 
-func (r *Receiver) rcvRemoteConfig(ctx context.Context, config *protobufs.AgentRemoteConfig) (reportStatus bool) {
-	effective, err := r.callbacks.OnRemoteConfig(ctx, config)
-	if err == nil {
+func (r *Receiver) rcvRemoteConfig(
+	ctx context.Context,
+	config *protobufs.AgentRemoteConfig,
+	flags protobufs.ServerToAgent_Flags,
+) (reportStatus bool) {
+	effective, changed, err := r.callbacks.OnRemoteConfig(ctx, config)
+	if err != nil {
+		return false
+	}
+
+	// Report the effective configuration if it changed or if the Server explicitly
+	// asked us to report it.
+	reportEffective := changed || (flags&protobufs.ServerToAgent_ReportEffectiveConfig != 0)
+
+	if reportEffective {
 		r.sender.UpdateNextStatus(func(statusReport *protobufs.StatusReport) {
 			statusReport.EffectiveConfig = effective
 		})
-		if effective != nil {
-			return true
-		}
+		return true
 	}
 	return false
 }
diff --git a/client/types/callbacks.go b/client/types/callbacks.go
index d5f90ca..a51a6ba 100644
--- a/client/types/callbacks.go
+++ b/client/types/callbacks.go
@@ -35,6 +35,10 @@ type Callbacks interface {
 	//
 	// The Agent should process the config and return the effective config if processing
 	// succeeded or an error if processing failed.
+	//
+	// configChanged must be set to true if as a result of applying the remote config
+	// the effective config has changed.
+	//
 	// The returned effective config or the error will be reported back to the server
 	// via StatusReport message (using EffectiveConfig and RemoteConfigStatus fields).
 	//
@@ -45,8 +49,8 @@ type Callbacks interface {
 	// remote config received.
 	OnRemoteConfig(
 		ctx context.Context,
-		config *protobufs.AgentRemoteConfig,
-	) (*protobufs.EffectiveConfig, error)
+		remoteConfig *protobufs.AgentRemoteConfig,
+	) (effectiveConfig *protobufs.EffectiveConfig, configChanged bool, err error)
 
 	// OnOpampConnectionSettings is called when the agent receives an OpAMP
 	// connection settings offer from the server. Typically the settings can specify
diff --git a/internal/examples/agent/agent/agent.go b/internal/examples/agent/agent/agent.go
index 8dbe1b7..42b06c9 100644
--- a/internal/examples/agent/agent/agent.go
+++ b/internal/examples/agent/agent/agent.go
@@ -183,14 +183,15 @@ func (agent *Agent) composeEffectiveConfig() *protobufs.EffectiveConfig {
 }
 
 func (agent *Agent) onRemoteConfig(
-	ctx context.Context,
-	config *protobufs.AgentRemoteConfig,
-) (*protobufs.EffectiveConfig, error) {
-	err := agent.applyRemoteConfig(config)
+	_ context.Context,
+	remoteConfig *protobufs.AgentRemoteConfig,
+) (effectiveConfig *protobufs.EffectiveConfig, configChanged bool, err error) {
+	configChanged, err = agent.applyRemoteConfig(remoteConfig)
 	if err != nil {
-		return nil, err
+		return nil, false, err
 	}
-	return agent.composeEffectiveConfig(), nil
+
+	return agent.composeEffectiveConfig(), configChanged, nil
 }
 
 type agentConfigFileItem struct {
@@ -214,9 +215,9 @@ func (a agentConfigFileSlice) Len() int {
 	return len(a)
 }
 
-func (agent *Agent) applyRemoteConfig(config *protobufs.AgentRemoteConfig) error {
+func (agent *Agent) applyRemoteConfig(config *protobufs.AgentRemoteConfig) (configChanged bool, err error) {
 	if config == nil {
-		return nil
+		return false, nil
 	}
 
 	agent.logger.Debugf("Received remote config from server, hash=%x.", config.ConfigHash)
@@ -224,7 +225,7 @@ func (agent *Agent) applyRemoteConfig(config *protobufs.AgentRemoteConfig) error
 	// Begin with local config. We will later merge received configs on top of it.
 	var k = koanf.New(".")
 	if err := k.Load(rawbytes.Provider([]byte(localConfig)), yaml.Parser()); err != nil {
-		return err
+		return false, err
 	}
 
 	orderedConfigs := agentConfigFileSlice{}
@@ -256,11 +257,11 @@ func (agent *Agent) applyRemoteConfig(config *protobufs.AgentRemoteConfig) error
 		var k2 = koanf.New(".")
 		err := k2.Load(rawbytes.Provider(item.file.Body), yaml.Parser())
 		if err != nil {
-			return fmt.Errorf("cannot parse config named %s: %v", item.name, err)
+			return false, fmt.Errorf("cannot parse config named %s: %v", item.name, err)
 		}
 		err = k.Merge(k2)
 		if err != nil {
-			return fmt.Errorf("cannot merge config named %s: %v", item.name, err)
+			return false, fmt.Errorf("cannot merge config named %s: %v", item.name, err)
 		}
 	}
 
@@ -271,16 +272,18 @@ func (agent *Agent) applyRemoteConfig(config *protobufs.AgentRemoteConfig) error
 	}
 
 	newEffectiveConfig := string(effectiveConfigBytes)
+	configChanged = false
 	if agent.effectiveConfig != newEffectiveConfig {
 		agent.logger.Debugf("Effective config changed. Need to report to server.")
 		agent.effectiveConfig = newEffectiveConfig
 		hash := sha256.Sum256(effectiveConfigBytes)
 		agent.effectiveConfigHash = hash[:]
+		configChanged = true
 	}
 
 	agent.remoteConfigHash = config.ConfigHash
 
-	return nil
+	return configChanged, nil
 }
 
 func (agent *Agent) Shutdown() {


Test patch:
diff --git a/client/clientimpl_test.go b/client/clientimpl_test.go
index 04d7558..583f5b9 100644
--- a/client/clientimpl_test.go
+++ b/client/clientimpl_test.go
@@ -271,15 +271,16 @@ func TestFirstStatusReport(t *testing.T) {
 				atomic.AddInt64(&connected, 1)
 			},
 			OnRemoteConfigFunc: func(
-				ctx context.Context, config *protobufs.AgentRemoteConfig,
-			) (*protobufs.EffectiveConfig, error) {
+				ctx context.Context,
+				config *protobufs.AgentRemoteConfig,
+			) (effectiveConfig *protobufs.EffectiveConfig, configChanged bool, err error) {
 				// Verify that the client received exactly the remote config that
 				// the server sent.
 				assert.True(t, proto.Equal(remoteConfig, config))
 				atomic.AddInt64(&remoteConfigReceived, 1)
 				return &protobufs.EffectiveConfig{
 					ConfigMap: remoteConfig.Config,
-				}, nil
+				}, true, nil
 			},
 		},
 		AgentDescription: &protobufs.AgentDescription{},