[extension/opamp] Make reported service.instance.id always match Collector resource attributes

.chloggen/opamp-matching-service-instance-id.yaml

@@ -0,0 +1,43 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: 'bug_fix'
+
+# The name of the component, or a single word describing the area of concern, (e.g. receiver/filelog)
+component: extension/opamp
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Decorrelate `service.instance.id` and OpAMP `instance_uid`
+
+# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
+issues: [46495]
+
+# (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: |
+  Previously:
+  - the `service.instance.id` reported in the AgentDescription was based on the OpAMP instance UID
+  - the instance UID was typically set based on the `service.instance.id` from the Collector resource attributes
+  - it could be overriden using the `instance_uid` configuration of the OpAMP extension
+
+  This meant that the reported `service.instance.id` did not always match the Collector resource attributes,
+  which is a problem for correlation, and that server implementations got used to the typical case of
+  `service.instance.id` and `instance_uid` matching, despite there being no guarantee of this.
+
+  Now:
+  - the reported value of `service.instance.id` always matches the Collector resource attributes
+  - the instance UID is either taken from the `instance_uid` configuration or generated randomly.
+
+  This means that the two values can never be expected to match, unless both configurations are explicitly set to the same value.
+  That is what the OpAMP supervisor does, which means its behavior is unaffected.
+
+
+# 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: [user]

cmd/opampsupervisor/e2e_test.go

@@ -1234,36 +1234,32 @@ func TestSupervisorAgentDescriptionConfigApplies(t *testing.T) {
 		t.Fatal("Failed to get agent description after 5 seconds")
 	}
 
-	expectedDescription := &protobufs.AgentDescription{
-		IdentifyingAttributes: []*protobufs.KeyValue{
-			stringKeyValue("client.id", "my-client-id"),
-			stringKeyValue("service.instance.id", uuid.UUID(ad.InstanceUid).String()),
-			stringKeyValue("service.name", command),
-			stringKeyValue("service.version", version),
-		},
-		NonIdentifyingAttributes: []*protobufs.KeyValue{
-			stringKeyValue("env", "prod"),
-			stringKeyValue("host.arch", runtime.GOARCH),
-			stringKeyValue("host.name", host),
-			stringKeyValue("os.type", runtime.GOOS),
-		},
+	expectedIdentifyingAttributes := map[string]string{
+		"client.id":           "my-client-id",
+		"service.instance.id": uuid.UUID(ad.InstanceUid).String(),
+		"service.name":        command,
+		"service.version":     version,
 	}
-
-	require.Subset(t, ad.AgentDescription.IdentifyingAttributes, expectedDescription.IdentifyingAttributes)
-	require.Subset(t, ad.AgentDescription.NonIdentifyingAttributes, expectedDescription.NonIdentifyingAttributes)
+	expectedNonIdentifyingAttributes := map[string]string{
+		"env":       "prod",
+		"host.arch": runtime.GOARCH,
+		"host.name": host,
+		"os.type":   runtime.GOOS,
+	}
+	actualIdentifyingAttributes := keyValuesToStringMap(ad.AgentDescription.IdentifyingAttributes)
+	require.Subset(t, actualIdentifyingAttributes, expectedIdentifyingAttributes)
+	actualNonIdentifyingAttributes := keyValuesToStringMap(ad.AgentDescription.NonIdentifyingAttributes)
+	require.Subset(t, actualNonIdentifyingAttributes, expectedNonIdentifyingAttributes)
 
 	time.Sleep(250 * time.Millisecond)
 }
 
-func stringKeyValue(key, val string) *protobufs.KeyValue {
-	return &protobufs.KeyValue{
-		Key: key,
-		Value: &protobufs.AnyValue{
-			Value: &protobufs.AnyValue_StringValue{
-				StringValue: val,
-			},
-		},
+func keyValuesToStringMap(kvs []*protobufs.KeyValue) map[string]string {
+	out := make(map[string]string, len(kvs))
+	for _, kv := range kvs {
+		out[kv.Key] = kv.Value.GetStringValue()
 	}
+	return out
 }
 
 // Creates a Collector config that reads and writes logs to files and provides

cmd/opampsupervisor/supervisor/supervisor.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"crypto/tls"
 	_ "embed"
+	"encoding/hex"
 	"errors"
 	"fmt"
 	"net"
@@ -483,34 +484,23 @@ func (s *Supervisor) getBootstrapInfo() (err error) {
 		},
 		onMessage: func(_ serverTypes.Connection, message *protobufs.AgentToServer) *protobufs.ServerToAgent {
 			response := &protobufs.ServerToAgent{}
+
+			if !slices.Equal(message.GetInstanceUid(), s.persistentState.InstanceID[:]) {
+				done <- fmt.Errorf(
+					"the Collector's instance ID (%s) does not match with the instance ID set by the Supervisor (%s): %w",
+					hex.EncodeToString(message.GetInstanceUid()),
+					s.persistentState.InstanceID.String(),
+					errNonMatchingInstanceUID,
+				)
+				return response
+			}
+
 			if message.GetAvailableComponents() != nil {
 				s.setAvailableComponents(message.AvailableComponents)
 			}
 
 			if message.AgentDescription != nil {
-				instanceIDSeen := false
 				s.setAgentDescription(message.AgentDescription)
-				identAttr := message.AgentDescription.IdentifyingAttributes
-
-				for _, attr := range identAttr {
-					if attr.Key == string(conventions.ServiceInstanceIDKey) {
-						if attr.Value.GetStringValue() != s.persistentState.InstanceID.String() {
-							done <- fmt.Errorf(
-								"the Collector's instance ID (%s) does not match with the instance ID set by the Supervisor (%s): %w",
-								attr.Value.GetStringValue(),
-								s.persistentState.InstanceID.String(),
-								errNonMatchingInstanceUID,
-							)
-							return response
-						}
-						instanceIDSeen = true
-					}
-				}
-
-				if !instanceIDSeen {
-					done <- errors.New("the Collector did not specify an instance ID in its AgentDescription message")
-					return response
-				}
 			}
 
 			// agent description must be defined

cmd/opampsupervisor/supervisor/supervisor_test.go

@@ -554,7 +554,8 @@ service:
                 - stderr
             output_paths:
                 - stdout
-        resource: null
+        resource:
+            service.instance.id: 018fee23-4a51-7303-a441-73faed7d9deb
 `
 
 		remoteConfig := &protobufs.AgentRemoteConfig{
@@ -655,7 +656,8 @@ service:
                 - stderr
             output_paths:
                 - stdout
-        resource: null
+        resource:
+            service.instance.id: 018fee23-4a51-7303-a441-73faed7d9deb
 `
 
 		remoteConfig := &protobufs.AgentRemoteConfig{
@@ -841,7 +843,8 @@ service:
                 - stderr
             output_paths:
                 - stdout
-        resource: null
+        resource:
+            service.instance.id: 018fee23-4a51-7303-a441-73faed7d9deb
 `
 
 		// store the initial remote config message so the supervisor is initialized with it
@@ -1722,6 +1725,7 @@ service:
                             endpoint: localhost-metrics
                             protocol: http/protobuf
         resource:
+            service.instance.id: 018fee23-4a51-7303-a441-73faed7d9deb
             service.name: otelcol
         traces:
             processors:
@@ -1841,6 +1845,9 @@ service:
                 - nop
             receivers:
                 - nop
+    telemetry:
+        resource:
+            service.instance.id: 018fee23-4a51-7303-a441-73faed7d9deb
 `
 	s := Supervisor{
 		persistentState: &persistentState{
@@ -1884,6 +1891,9 @@ service:
                 - nop
             receivers:
                 - nop
+    telemetry:
+        resource:
+            service.instance.id: 018fee23-4a51-7303-a441-73faed7d9deb
 `
 	s := Supervisor{
 		persistentState: &persistentState{

cmd/opampsupervisor/supervisor/templates/opampextension.yaml

@@ -13,3 +13,6 @@ extensions:
 
 service:
   extensions: [opamp]
+  telemetry:
+    resource:
+      "service.instance.id": "{{.InstanceUid}}"

cmd/opampsupervisor/supervisor/testdata/merged_effective_config.yaml

@@ -47,4 +47,5 @@ service:
       error_output_paths: ["stderr"]
       output_paths: ["stdout"]
     resource:
+      service.instance.id: 00000000-0000-0000-0000-000000000000
       service.name: otelcol

cmd/opampsupervisor/supervisor/testdata/merged_local_config.yaml

@@ -36,4 +36,5 @@ service:
       error_output_paths: ["stderr"]
       output_paths: ["stdout"]
     resource:
+      service.instance.id: 00000000-0000-0000-0000-000000000000
       service.name: otelcol

cmd/opampsupervisor/testdata/collector/effective_config.yaml

@@ -40,4 +40,5 @@ service:
             error_output_paths: ["stderr"]
             output_paths: ["stdout"]
         resource:
+            service.instance.id: 00000000-0000-0000-0000-000000000000
             service.name: otelcol

extension/opampextension/opamp_agent.go

@@ -56,9 +56,10 @@ type opampAgent struct {
 	cfg    *Config
 	logger *zap.Logger
 
-	agentType     string
-	agentVersion  string
-	resourceAttrs map[string]string
+	agentType       string
+	agentVersion    string
+	agentInstanceID string
+	resourceAttrs   map[string]string
 
 	instanceID uuid.UUID
 
@@ -296,25 +297,27 @@ func newOpampAgent(cfg *Config, set extension.Settings) (*opampAgent, error) {
 		agentVersion = sv.AsString()
 	}
 
-	uid, err := uuid.NewV7()
-	if err != nil {
-		return nil, fmt.Errorf("could not generate uuidv7: %w", err)
+	agentInstanceID := ""
+
+	if sid, ok := set.Resource.Attributes().Get(string(conventions.ServiceInstanceIDKey)); ok {
+		agentInstanceID = sid.Str()
 	}
 
+	var uid uuid.UUID
 	if cfg.InstanceUID != "" {
+		var err error
 		uid, err = parseInstanceIDString(cfg.InstanceUID)
 		if err != nil {
 			return nil, fmt.Errorf("could not parse configured instance id: %w", err)
 		}
 	} else {
-		sid, ok := set.Resource.Attributes().Get(string(conventions.ServiceInstanceIDKey))
-		if ok {
-			uid, err = uuid.Parse(sid.AsString())
-			if err != nil {
-				return nil, err
-			}
+		var err error
+		uid, err = uuid.NewV7()
+		if err != nil {
+			return nil, fmt.Errorf("could not generate uuidv7: %w", err)
 		}
 	}
+
 	resourceAttrs := make(map[string]string, set.Resource.Attributes().Len())
 	set.Resource.Attributes().Range(func(k string, v pcommon.Value) bool {
 		resourceAttrs[k] = v.Str()
@@ -327,6 +330,7 @@ func newOpampAgent(cfg *Config, set extension.Settings) (*opampAgent, error) {
 		logger:                   set.Logger,
 		agentType:                agentType,
 		agentVersion:             agentVersion,
+		agentInstanceID:          agentInstanceID,
 		instanceID:               uid,
 		capabilities:             cfg.Capabilities,
 		opampClient:              opampClient,
@@ -377,7 +381,7 @@ func (o *opampAgent) createAgentDescription() error {
 	description := getOSDescription(o.logger)
 
 	ident := []*protobufs.KeyValue{
-		stringKeyValue(string(conventions.ServiceInstanceIDKey), o.instanceID.String()),
+		stringKeyValue(string(conventions.ServiceInstanceIDKey), o.agentInstanceID),
 		stringKeyValue(string(conventions.ServiceNameKey), o.agentType),
 		stringKeyValue(string(conventions.ServiceVersionKey), o.agentVersion),
 	}

extension/opampextension/opamp_agent_test.go

@@ -62,7 +62,7 @@ func TestNewOpampAgentAttributes(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "otelcol-distro", o.agentType)
 	assert.Equal(t, "distro.0", o.agentVersion)
-	assert.Equal(t, "f8999bc1-4c9b-4619-9bae-7f009d2411ec", o.instanceID.String())
+	assert.Equal(t, "f8999bc1-4c9b-4619-9bae-7f009d2411ec", o.agentInstanceID)
 	assert.NoError(t, o.Shutdown(t.Context()))
 }