Repository: open-telemetry/opamp-go
Pull request #74 (2022-05-13 18:01:27)

Eliminate StartSettings.AgentDescription
We don't need 2 ways to set AgentDescription, it is confusing.
Now the only way is to call SetAgentDescription(), which must be
called before Start() and may be called after Start() too.

Resolves https://github.com/open-telemetry/opamp-go/issues/47

codecov[bot]: # [Codecov](https://codecov.io/gh/open-telemetry/opamp-go/pull/74?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
> Merging [#74](https://codecov.io/gh/open-telemetry/opamp-go/pull/74?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (30f7323) into [main](https://codecov.io/gh/open-telemetry/opamp-go/commit/bb2e0097209d05227305e0177eaaaebadc8c8cb2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) (bb2e009) will **increase** coverage by `0.15%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   77.40%   77.55%   +0.15%     
==========================================
  Files          21       21              
  Lines        1301     1310       +9     
==========================================
+ Hits         1007     1016       +9     
  Misses        221      221              
  Partials       73       73              
```


| [Impacted Files](https://codecov.io/gh/open-telemetry/opamp-go/pull/74?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | |
|---|---|---|
| [client/httpclient.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2h0dHBjbGllbnQuZ28=) | `92.85% <100.00%> (+0.35%)` | :arrow_up: |
| [client/internal/clientcommon.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL2NsaWVudGNvbW1vbi5nbw==) | `79.03% <100.00%> (+0.34%)` | :arrow_up: |
| [client/internal/clientstate.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL2NsaWVudHN0YXRlLmdv) | `74.00% <100.00%> (+1.65%)` | :arrow_up: |
| [client/wsclient.go](https://codecov.io/gh/open-telemetry/opamp-go/pull/74/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L3dzY2xpZW50Lmdv) | `83.33% <100.00%> (+0.22%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/open-telemetry/opamp-go/pull/74?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/open-telemetry/opamp-go/pull/74?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry). Last update [bb2e009...30f7323](https://codecov.io/gh/open-telemetry/opamp-go/pull/74?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).


Patch:
diff --git a/client/client.go b/client/client.go
index 8b875ea..73f14cf 100644
--- a/client/client.go
+++ b/client/client.go
@@ -43,14 +43,16 @@ type OpAMPClient interface {
 	Stop(ctx context.Context) error
 
 	// SetAgentDescription sets attributes of the Agent. The attributes will be included
-	// in the next status report sent to the Server.
-	// May be called after Start(), in which case the attributes will be included
+	// in the next status report sent to the Server. MUST be called before Start().
+	// May be also called after Start(), in which case the attributes will be included
 	// in the next outgoing status report. This is typically used by Agents which allow
 	// their AgentDescription to change dynamically while the OpAMPClient is started.
-	// To define the initial Agent description that is included in the first status report
-	// set StartSettings.AgentDescription field.
+	// nil values are not allowed and will return an error.
 	SetAgentDescription(descr *protobufs.AgentDescription) error
 
+	// AgentDescription returns the last value successfully set by SetAgentDescription().
+	AgentDescription() *protobufs.AgentDescription
+
 	// UpdateEffectiveConfig fetches the current local effective config using
 	// GetEffectiveConfig callback and sends it to the Server.
 	UpdateEffectiveConfig(ctx context.Context) error
diff --git a/client/httpclient.go b/client/httpclient.go
index 7836ae0..f624337 100644
--- a/client/httpclient.go
+++ b/client/httpclient.go
@@ -62,6 +62,10 @@ func (c *httpClient) Stop(ctx context.Context) error {
 	return c.common.Stop(ctx)
 }
 
+func (c *httpClient) AgentDescription() *protobufs.AgentDescription {
+	return c.common.AgentDescription()
+}
+
 func (c *httpClient) SetAgentDescription(descr *protobufs.AgentDescription) error {
 	return c.common.SetAgentDescription(descr)
 }
diff --git a/client/internal/clientcommon.go b/client/internal/clientcommon.go
index 59da5f3..a0f7bf2 100644
--- a/client/internal/clientcommon.go
+++ b/client/internal/clientcommon.go
@@ -11,10 +11,11 @@ import (
 )
 
 var (
-	errAgentDescriptionMissing   = errors.New("AgentDescription is not set")
-	errRemoteConfigStatusMissing = errors.New("RemoteConfigStatus is not set")
-	errAlreadyStarted            = errors.New("already started")
-	errCannotStopNotStarted      = errors.New("cannot stop because not started")
+	ErrAgentDescriptionMissing      = errors.New("AgentDescription is nil")
+	ErrAgentDescriptionNoAttributes = errors.New("AgentDescription has no attributes defined")
+	errRemoteConfigStatusMissing    = errors.New("RemoteConfigStatus is not set")
+	errAlreadyStarted               = errors.New("already started")
+	errCannotStopNotStarted         = errors.New("cannot stop because not started")
 )
 
 // ClientCommon contains the OpAMP logic that is common between WebSocket and
@@ -52,8 +53,8 @@ func (c *ClientCommon) PrepareStart(_ context.Context, settings types.StartSetti
 		return errAlreadyStarted
 	}
 
-	if err := c.ClientSyncedState.SetAgentDescription(settings.AgentDescription); err != nil {
-		return err
+	if c.ClientSyncedState.AgentDescription() == nil {
+		return ErrAgentDescriptionMissing
 	}
 
 	if settings.RemoteConfigStatus == nil {
@@ -168,6 +169,10 @@ func (c *ClientCommon) PrepareFirstMessage(ctx context.Context) error {
 	return nil
 }
 
+func (c *ClientCommon) AgentDescription() *protobufs.AgentDescription {
+	return c.ClientSyncedState.AgentDescription()
+}
+
 // SetAgentDescription sends a status update to the Server with the new AgentDescription
 // and remembers the AgentDescription in the client state so that it can be sent
 // to the Server when the Server asks for it.
diff --git a/client/internal/clientstate.go b/client/internal/clientstate.go
index 58d0fdd..0aa9fa1 100644
--- a/client/internal/clientstate.go
+++ b/client/internal/clientstate.go
@@ -53,7 +53,11 @@ func (s *ClientSyncedState) PackageStatuses() *protobufs.PackageStatuses {
 // SetAgentDescription sets the AgentDescription in the state.
 func (s *ClientSyncedState) SetAgentDescription(descr *protobufs.AgentDescription) error {
 	if descr == nil {
-		return errAgentDescriptionMissing
+		return ErrAgentDescriptionMissing
+	}
+
+	if descr.IdentifyingAttributes == nil && descr.NonIdentifyingAttributes == nil {
+		return ErrAgentDescriptionNoAttributes
 	}
 
 	clone := proto.Clone(descr).(*protobufs.AgentDescription)
diff --git a/client/types/startsettings.go b/client/types/startsettings.go
index 90bc104..851a3ba 100644
--- a/client/types/startsettings.go
+++ b/client/types/startsettings.go
@@ -21,10 +21,6 @@ type StartSettings struct {
 	// Agent information.
 	InstanceUid string
 
-	// AgentDescription MUST be set and MUST describe the Agent. See OpAMP spec
-	// for details.
-	AgentDescription *protobufs.AgentDescription
-
 	// Callbacks that the client will call after Start() returns nil.
 	Callbacks Callbacks
 
diff --git a/client/wsclient.go b/client/wsclient.go
index 2e66456..0933c9a 100644
--- a/client/wsclient.go
+++ b/client/wsclient.go
@@ -92,6 +92,10 @@ func (c *wsClient) Stop(ctx context.Context) error {
 	return c.common.Stop(ctx)
 }
 
+func (c *wsClient) AgentDescription() *protobufs.AgentDescription {
+	return c.common.AgentDescription()
+}
+
 func (c *wsClient) SetAgentDescription(descr *protobufs.AgentDescription) error {
 	return c.common.SetAgentDescription(descr)
 }
diff --git a/internal/examples/agent/agent/agent.go b/internal/examples/agent/agent/agent.go
index da99401..dea0acb 100644
--- a/internal/examples/agent/agent/agent.go
+++ b/internal/examples/agent/agent/agent.go
@@ -84,9 +84,8 @@ func (agent *Agent) start() error {
 	agent.opampClient = client.NewWebSocket(agent.logger)
 
 	settings := types.StartSettings{
-		OpAMPServerURL:   "ws://127.0.0.1:4320/v1/opamp",
-		InstanceUid:      agent.instanceId.String(),
-		AgentDescription: agent.agentDescription,
+		OpAMPServerURL: "ws://127.0.0.1:4320/v1/opamp",
+		InstanceUid:    agent.instanceId.String(),
 		Callbacks: types.CallbacksStruct{
 			OnConnectFunc: func() {
 				agent.logger.Debugf("Connected to the server.")
@@ -109,10 +108,14 @@ func (agent *Agent) start() error {
 		},
 		RemoteConfigStatus: agent.remoteConfigStatus,
 	}
+	err := agent.opampClient.SetAgentDescription(agent.agentDescription)
+	if err != nil {
+		return err
+	}
 
 	agent.logger.Debugf("Starting OpAMP client...")
 
-	err := agent.opampClient.Start(context.Background(), settings)
+	err = agent.opampClient.Start(context.Background(), settings)
 	if err != nil {
 		return err
 	}
diff --git a/internal/examples/server/data/agent.go b/internal/examples/server/data/agent.go
index 31d0bfb..425993e 100644
--- a/internal/examples/server/data/agent.go
+++ b/internal/examples/server/data/agent.go
@@ -110,13 +110,18 @@ func (agent *Agent) updateStatusField(newStatus *protobufs.StatusReport) (agentD
 			// (or this is the first report).
 			// Make full comparison of previous and new descriptions to see if it
 			// really is different.
-			if prevStatus != nil && isEqualAgentDescr(prevStatus.AgentDescription, newStatus.AgentDescription) {
+			if prevStatus != nil && bytes.Equal(prevStatus.AgentDescription.Hash, newStatus.AgentDescription.Hash) {
 				// Agent description didn't change.
 				agentDescrChanged = false
 			} else {
 				// Yes, the description is different, update it.
-				agent.Status.AgentDescription = newStatus.AgentDescription
-				agentDescrChanged = true
+				if newStatus.AgentDescription.IdentifyingAttributes == nil &&
+					newStatus.AgentDescription.NonIdentifyingAttributes == nil {
+					// TODO: request full AgentDescription
+				} else {
+					agent.Status.AgentDescription = newStatus.AgentDescription
+					agentDescrChanged = true
+				}
 			}
 		} else {
 			// AgentDescription field is not set, which means description didn't change.


Test patch:
diff --git a/client/clientimpl_test.go b/client/clientimpl_test.go
index 776e1e2..4f53842 100644
--- a/client/clientimpl_test.go
+++ b/client/clientimpl_test.go
@@ -78,7 +78,7 @@ func eventually(t *testing.T, f func() bool) {
 	assert.Eventually(t, f, 5*time.Second, 10*time.Millisecond)
 }
 
-func prepareClient(t *testing.T, settings *types.StartSettings, c OpAMPClient) {
+func prepareSettings(t *testing.T, settings *types.StartSettings, c OpAMPClient) {
 	// Autogenerate instance id.
 	entropy := ulid.Monotonic(rand.New(rand.NewSource(99)), 0)
 	settings.InstanceUid = ulid.MustNew(ulid.Timestamp(time.Now()), entropy).String()
@@ -93,8 +93,12 @@ func prepareClient(t *testing.T, settings *types.StartSettings, c OpAMPClient) {
 		u.Scheme = "ws"
 	}
 	settings.OpAMPServerURL = u.String()
+}
 
-	settings.AgentDescription = createAgentDescr()
+func prepareClient(t *testing.T, settings *types.StartSettings, c OpAMPClient) {
+	prepareSettings(t, settings, c)
+	err := c.SetAgentDescription(createAgentDescr())
+	assert.NoError(t, err)
 }
 
 func startClient(t *testing.T, settings types.StartSettings, client OpAMPClient) {
@@ -171,6 +175,26 @@ func TestStopCancellation(t *testing.T) {
 	})
 }
 
+func TestStartNoDescription(t *testing.T) {
+	testClients(t, func(t *testing.T, client OpAMPClient) {
+		settings := createNoServerSettings()
+		prepareSettings(t, &settings, client)
+		err := client.Start(context.Background(), settings)
+		assert.EqualValues(t, err, internal.ErrAgentDescriptionMissing)
+	})
+}
+
+func TestSetInvalidAgentDescription(t *testing.T) {
+	testClients(t, func(t *testing.T, client OpAMPClient) {
+		settings := createNoServerSettings()
+		prepareSettings(t, &settings, client)
+		err := client.SetAgentDescription(nil)
+		assert.EqualValues(t, err, internal.ErrAgentDescriptionMissing)
+		err = client.SetAgentDescription(&protobufs.AgentDescription{})
+		assert.EqualValues(t, err, internal.ErrAgentDescriptionNoAttributes)
+	})
+}
+
 func TestConnectWithServer(t *testing.T) {
 	testClients(t, func(t *testing.T, client OpAMPClient) {
 		// Start a server.
@@ -362,7 +386,6 @@ func TestIncludesDetailsOnReconnect(t *testing.T) {
 				return &protobufs.EffectiveConfig{}, false, nil
 			},
 		},
-		AgentDescription: &protobufs.AgentDescription{},
 	}
 
 	settings.OpAMPServerURL = "ws://" + srv.Endpoint
@@ -545,7 +568,6 @@ func TestAgentIdentification(t *testing.T) {
 		// Start a client.
 		settings := types.StartSettings{}
 		settings.OpAMPServerURL = "ws://" + srv.Endpoint
-		settings.AgentDescription = createAgentDescr()
 		settings.Callbacks = types.CallbacksStruct{
 			OnAgentIdentificationFunc: func(
 				ctx context.Context,
@@ -708,7 +730,7 @@ func TestReportAgentDescription(t *testing.T) {
 		// ---> Server
 		srv.Expect(func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
 			// The first status report after Start must have full AgentDescription.
-			assert.True(t, proto.Equal(settings.AgentDescription, msg.StatusReport.AgentDescription))
+			assert.True(t, proto.Equal(client.AgentDescription(), msg.StatusReport.AgentDescription))
 			return &protobufs.ServerToAgent{InstanceUid: msg.InstanceUid}
 		})
 
@@ -725,7 +747,7 @@ func TestReportAgentDescription(t *testing.T) {
 
 			// The Hash field must be present and unchanged.
 			assert.NotNil(t, descr.Hash)
-			assert.EqualValues(t, settings.AgentDescription.Hash, descr.Hash)
+			assert.EqualValues(t, client.AgentDescription().Hash, descr.Hash)
 
 			// Ask client for full AgentDescription.
 			return &protobufs.ServerToAgent{
@@ -740,7 +762,7 @@ func TestReportAgentDescription(t *testing.T) {
 		srv.Expect(func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
 			// The status report must again have full AgentDescription
 			// because the Server asked for it.
-			assert.True(t, proto.Equal(settings.AgentDescription, msg.StatusReport.AgentDescription))
+			assert.True(t, proto.Equal(client.AgentDescription(), msg.StatusReport.AgentDescription))
 			return &protobufs.ServerToAgent{InstanceUid: msg.InstanceUid}
 		})
 
@@ -785,7 +807,7 @@ func TestReportEffectiveConfig(t *testing.T) {
 
 		// Client --->
 		// Trigger another status report for example by setting AgentDescription.
-		_ = client.SetAgentDescription(settings.AgentDescription)
+		_ = client.SetAgentDescription(client.AgentDescription())
 
 		// ---> Server
 		srv.Expect(func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
@@ -883,7 +905,7 @@ func verifyRemoteConfigUpdate(t *testing.T, successCase bool, expectStatus *prot
 
 		// Client --->
 		// Trigger another status report by setting AgentDescription.
-		_ = client.SetAgentDescription(settings.AgentDescription)
+		_ = client.SetAgentDescription(client.AgentDescription())
 
 		// ---> Server
 		srv.Expect(func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {