Refactor OpAMP bridge logging and configuration

.chloggen/refactor-bridge-config.yaml

@@ -0,0 +1,19 @@
+# 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. operator, target allocator, github action)
+component: OpAMP Bridge
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: This PR simplifies the bridge's configuration and logging by renaming and removing fields.
+
+# One or more tracking issues related to the change
+issues: [1368]
+
+# (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: |
+  `components_allowed` => `componentsAllowed`
+  :x: `protocol` which is now inferred from endpoint
+  capabilities `[]string` => `map[Capability]bool` for enhanced configuration validation

cmd/operator-opamp-bridge/agent/agent.go

@@ -17,8 +17,10 @@ package agent
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"time"
 
+	"github.com/go-logr/logr"
 	"gopkg.in/yaml.v3"
 
 	"github.com/open-telemetry/opentelemetry-operator/cmd/operator-opamp-bridge/metrics"
@@ -35,7 +37,7 @@ import (
 )
 
 type Agent struct {
-	logger types.Logger
+	logger logr.Logger
 
 	appliedKeys map[collectorKey]bool
 	startTime   uint64
@@ -47,12 +49,12 @@ type Agent struct {
 
 	opampClient         client.OpAMPClient
 	metricReporter      *metrics.MetricReporter
-	config              config.Config
+	config              *config.Config
 	applier             operator.ConfigApplier
 	remoteConfigEnabled bool
 }
 
-func NewAgent(logger types.Logger, applier operator.ConfigApplier, config config.Config, opampClient client.OpAMPClient) *Agent {
+func NewAgent(logger logr.Logger, applier operator.ConfigApplier, config *config.Config, opampClient client.OpAMPClient) *Agent {
 	agent := &Agent{
 		config:              config,
 		applier:             applier,
@@ -64,8 +66,10 @@ func NewAgent(logger types.Logger, applier operator.ConfigApplier, config config
 		opampClient:         opampClient,
 	}
 
-	agent.logger.Debugf("Agent created, id=%v, type=%s, version=%s.",
-		agent.instanceId.String(), config.GetAgentType(), config.GetAgentVersion())
+	agent.logger.V(3).Info("Agent created",
+		"instanceId", agent.instanceId.String(),
+		"agentType", config.GetAgentType(),
+		"agentVersion", config.GetAgentVersion())
 
 	return agent
 }
@@ -81,17 +85,17 @@ func (agent *Agent) getHealth() *protobufs.AgentHealth {
 
 // onConnect is called when an agent is successfully connected to a server.
 func (agent *Agent) onConnect() {
-	agent.logger.Debugf("Connected to the server.")
+	agent.logger.V(3).Info("Connected to the server.")
 }
 
 // onConnectFailed is called when an agent was unable to connect to a server.
 func (agent *Agent) onConnectFailed(err error) {
-	agent.logger.Errorf("Failed to connect to the server: %v", err)
+	agent.logger.Error(err, "failed to connect to the server")
 }
 
 // onError is called when an agent receives an error response from the server.
 func (agent *Agent) onError(err *protobufs.ServerErrorResponse) {
-	agent.logger.Errorf("Server returned an error response: %v", err.ErrorMessage)
+	agent.logger.Error(fmt.Errorf(err.GetErrorMessage()), "server returned an error response")
 }
 
 // saveRemoteConfigStatus receives a status from the server when the server sets a remote configuration.
@@ -126,24 +130,24 @@ func (agent *Agent) Start() error {
 		return err
 	}
 
-	agent.logger.Debugf("Starting OpAMP client...")
+	agent.logger.V(3).Info("Starting OpAMP client...")
 
 	err = agent.opampClient.Start(context.Background(), settings)
 	if err != nil {
 		return err
 	}
 
-	agent.logger.Debugf("OpAMP Client started.")
+	agent.logger.V(3).Info("OpAMP Client started.")
 
 	return nil
 }
 
 // updateAgentIdentity receives a new instanced Id from the remote server and updates the agent's instanceID field.
 // The meter will be reinitialized by the onMessage function.
 func (agent *Agent) updateAgentIdentity(instanceId ulid.ULID) {
-	agent.logger.Debugf("Agent identity is being changed from id=%v to id=%v",
-		agent.instanceId.String(),
-		instanceId.String())
+	agent.logger.V(3).Info("Agent identity is being changed",
+		"old instanceId", agent.instanceId.String(),
+		"new instanceid", instanceId.String())
 	agent.instanceId = instanceId
 }
 
@@ -152,14 +156,14 @@ func (agent *Agent) updateAgentIdentity(instanceId ulid.ULID) {
 func (agent *Agent) getEffectiveConfig(ctx context.Context) (*protobufs.EffectiveConfig, error) {
 	instances, err := agent.applier.ListInstances()
 	if err != nil {
-		agent.logger.Errorf("couldn't list instances", err)
+		agent.logger.Error(err, "failed to list instances")
 		return nil, err
 	}
 	instanceMap := map[string]*protobufs.AgentConfigFile{}
 	for _, instance := range instances {
 		marshaled, err := yaml.Marshal(instance)
 		if err != nil {
-			agent.logger.Errorf("couldn't marshal collector configuration", err)
+			agent.logger.Error(err, "failed to marhsal config")
 			return nil, err
 		}
 		mapKey := newCollectorKey(instance.GetName(), instance.GetNamespace())
@@ -181,7 +185,7 @@ func (agent *Agent) getEffectiveConfig(ctx context.Context) (*protobufs.Effectiv
 func (agent *Agent) initMeter(settings *protobufs.TelemetryConnectionSettings) {
 	reporter, err := metrics.NewMetricReporter(agent.logger, settings, agent.config.GetAgentType(), agent.config.GetAgentVersion(), agent.instanceId)
 	if err != nil {
-		agent.logger.Errorf("Cannot collect metrics: %v", err)
+		agent.logger.Error(err, "failed to create metric reporter")
 		return
 	}
 
@@ -244,11 +248,11 @@ func (agent *Agent) applyRemoteConfig(config *protobufs.AgentRemoteConfig) (*pro
 
 // Shutdown will stop the OpAMP client gracefully.
 func (agent *Agent) Shutdown() {
-	agent.logger.Debugf("Agent shutting down...")
+	agent.logger.V(3).Info("Agent shutting down...")
 	if agent.opampClient != nil {
 		err := agent.opampClient.Stop(context.Background())
 		if err != nil {
-			agent.logger.Errorf(err.Error())
+			agent.logger.Error(err, "failed to stop client")
 		}
 	}
 	if agent.metricReporter != nil {
@@ -265,16 +269,16 @@ func (agent *Agent) onMessage(ctx context.Context, msg *types.MessageData) {
 		var err error
 		status, err := agent.applyRemoteConfig(msg.RemoteConfig)
 		if err != nil {
-			agent.logger.Errorf(err.Error())
+			agent.logger.Error(err, "failed to apply remote config")
 		}
 		err = agent.opampClient.SetRemoteConfigStatus(status)
 		if err != nil {
-			agent.logger.Errorf(err.Error())
+			agent.logger.Error(err, "failed to set remote config status")
 			return
 		}
 		err = agent.opampClient.UpdateEffectiveConfig(ctx)
 		if err != nil {
-			agent.logger.Errorf(err.Error())
+			agent.logger.Error(err, "failed to update effective config")
 		}
 	}
 
@@ -283,7 +287,7 @@ func (agent *Agent) onMessage(ctx context.Context, msg *types.MessageData) {
 	if msg.AgentIdentification != nil {
 		newInstanceId, err := ulid.Parse(msg.AgentIdentification.NewInstanceUid)
 		if err != nil {
-			agent.logger.Errorf(err.Error())
+			agent.logger.Error(err, "couldn't parse instance UID")
 			return
 		}
 		agent.updateAgentIdentity(newInstanceId)

cmd/operator-opamp-bridge/agent/agent_test.go

@@ -22,6 +22,8 @@ import (
 	"sort"
 	"testing"
 
+	"github.com/go-logr/logr"
+	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/require"
 
 	"github.com/oklog/ulid/v2"
@@ -32,18 +34,15 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
 	"github.com/open-telemetry/opentelemetry-operator/cmd/operator-opamp-bridge/config"
-	"github.com/open-telemetry/opentelemetry-operator/cmd/operator-opamp-bridge/logger"
 	"github.com/open-telemetry/opentelemetry-operator/cmd/operator-opamp-bridge/operator"
 )
 
 var (
-	l                               = logf.Log.WithName("agent-tests")
-	clientLogger                    = logger.NewLogger(&l)
-	_            client.OpAMPClient = &mockOpampClient{}
+	l                    = logr.Discard()
+	_ client.OpAMPClient = &mockOpampClient{}
 )
 
 type mockOpampClient struct {
@@ -91,7 +90,7 @@ func (m *mockOpampClient) SetPackageStatuses(statuses *protobufs.PackageStatuses
 	return nil
 }
 
-func getFakeApplier(t *testing.T, conf config.Config) *operator.Client {
+func getFakeApplier(t *testing.T, conf *config.Config) *operator.Client {
 	schemeBuilder := runtime.NewSchemeBuilder(func(s *runtime.Scheme) error {
 		s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.OpenTelemetryCollector{}, &v1alpha1.OpenTelemetryCollectorList{})
 		metav1.AddToGroupVersion(s, v1alpha1.GroupVersion)
@@ -173,6 +172,34 @@ func TestAgent_onMessage(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "base case http",
+			fields: fields{
+				configFile: "testdata/agenthttpbasic.yaml",
+			},
+			args: args{
+				ctx: context.Background(),
+				configFile: map[string]string{
+					"good/testnamespace": "basic.yaml",
+				},
+			},
+			want: want{
+				contents: map[string][]string{
+					"good/testnamespace": {
+						"kind: OpenTelemetryCollector",
+						"name: good",
+						"namespace: testnamespace",
+						"send_batch_size: 10000",
+						"receivers: [otlp]",
+						"status:",
+					},
+				},
+				status: &protobufs.RemoteConfigStatus{
+					LastRemoteConfigHash: []byte("good/testnamespace405"),
+					Status:               protobufs.RemoteConfigStatuses_RemoteConfigStatuses_APPLIED,
+				},
+			},
+		},
 		{
 			name: "failure",
 			fields: fields{
@@ -448,11 +475,12 @@ func TestAgent_onMessage(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			mockClient := &mockOpampClient{}
-			conf, err := config.Load(tt.fields.configFile)
-			require.NoError(t, err, "should be able to load config")
+			conf := config.NewConfig(logr.Discard())
+			loadErr := config.LoadFromFile(conf, tt.fields.configFile)
+			require.NoError(t, loadErr, "should be able to load config")
 			applier := getFakeApplier(t, conf)
-			agent := NewAgent(clientLogger, applier, conf, mockClient)
-			err = agent.Start()
+			agent := NewAgent(l, applier, conf, mockClient)
+			err := agent.Start()
 			defer agent.Shutdown()
 			require.NoError(t, err, "should be able to start agent")
 			data, err := getMessageDataFromConfigFile(tt.args.configFile)
@@ -498,10 +526,16 @@ func TestAgent_onMessage(t *testing.T) {
 
 func Test_CanUpdateIdentity(t *testing.T) {
 	mockClient := &mockOpampClient{}
-	conf, err := config.Load("testdata/agent.yaml")
-	require.NoError(t, err, "should be able to load config")
+
+	fs := config.GetFlagSet(pflag.ContinueOnError)
+	configFlag := []string{"--config-file", "testdata/agent.yaml"}
+	err := fs.Parse(configFlag)
+	assert.NoError(t, err)
+	conf := config.NewConfig(logr.Discard())
+	loadErr := config.LoadFromFile(conf, "testdata/agent.yaml")
+	require.NoError(t, loadErr, "should be able to load config")
 	applier := getFakeApplier(t, conf)
-	agent := NewAgent(clientLogger, applier, conf, mockClient)
+	agent := NewAgent(l, applier, conf, mockClient)
 	err = agent.Start()
 	defer agent.Shutdown()
 	require.NoError(t, err, "should be able to start agent")

cmd/operator-opamp-bridge/agent/testdata/agent.yaml

@@ -1,15 +1,14 @@
 endpoint: ws://127.0.0.1:4320/v1/opamp
-protocol: wss
 capabilities:
-  - AcceptsRemoteConfig
-  - ReportsEffectiveConfig
-  # - AcceptsPackages
-  # - ReportsPackageStatuses
-  - ReportsOwnTraces
-  - ReportsOwnMetrics
-  - ReportsOwnLogs
-  - AcceptsOpAMPConnectionSettings
-  - AcceptsOtherConnectionSettings
-  - AcceptsRestartCommand
-  - ReportsHealth
-  - ReportsRemoteConfig
+  AcceptsRemoteConfig: true
+  ReportsEffectiveConfig: true
+  AcceptsPackages: false
+  ReportsPackageStatuses: false
+  ReportsOwnTraces: true
+  ReportsOwnMetrics: true
+  ReportsOwnLogs: true
+  AcceptsOpAMPConnectionSettings: true
+  AcceptsOtherConnectionSettings: true
+  AcceptsRestartCommand: true
+  ReportsHealth: true
+  ReportsRemoteConfig: true

cmd/operator-opamp-bridge/agent/testdata/agentbasiccomponentsallowed.yaml

@@ -1,19 +1,18 @@
 endpoint: ws://127.0.0.1:4320/v1/opamp
-protocol: wss
 capabilities:
-  - AcceptsRemoteConfig
-  - ReportsEffectiveConfig
-  # - AcceptsPackages
-  # - ReportsPackageStatuses
-  - ReportsOwnTraces
-  - ReportsOwnMetrics
-  - ReportsOwnLogs
-  - AcceptsOpAMPConnectionSettings
-  - AcceptsOtherConnectionSettings
-  - AcceptsRestartCommand
-  - ReportsHealth
-  - ReportsRemoteConfig
-components_allowed:
+  AcceptsRemoteConfig: true
+  ReportsEffectiveConfig: true
+  AcceptsPackages: false
+  ReportsPackageStatuses: false
+  ReportsOwnTraces: true
+  ReportsOwnMetrics: true
+  ReportsOwnLogs: true
+  AcceptsOpAMPConnectionSettings: true
+  AcceptsOtherConnectionSettings: true
+  AcceptsRestartCommand: true
+  ReportsHealth: true
+  ReportsRemoteConfig: true
+componentsAllowed:
   receivers:
     - otlp
   processors:

cmd/operator-opamp-bridge/agent/testdata/agentbatchnotallowed.yaml

@@ -1,19 +1,18 @@
 endpoint: ws://127.0.0.1:4320/v1/opamp
-protocol: wss
 capabilities:
-  - AcceptsRemoteConfig
-  - ReportsEffectiveConfig
-  # - AcceptsPackages
-  # - ReportsPackageStatuses
-  - ReportsOwnTraces
-  - ReportsOwnMetrics
-  - ReportsOwnLogs
-  - AcceptsOpAMPConnectionSettings
-  - AcceptsOtherConnectionSettings
-  - AcceptsRestartCommand
-  - ReportsHealth
-  - ReportsRemoteConfig
-components_allowed:
+  AcceptsRemoteConfig: true
+  ReportsEffectiveConfig: true
+  AcceptsPackages: false
+  ReportsPackageStatuses: false
+  ReportsOwnTraces: true
+  ReportsOwnMetrics: true
+  ReportsOwnLogs: true
+  AcceptsOpAMPConnectionSettings: true
+  AcceptsOtherConnectionSettings: true
+  AcceptsRestartCommand: true
+  ReportsHealth: true
+  ReportsRemoteConfig: true
+componentsAllowed:
   receivers:
     - otlp
   processors: