Skip to content

Commit 94aefb2

Browse files
jaronoff97jaronoff97
authored
Refactor OpAMP bridge logging and configuration (open-telemetry#2196)
1 parent 63a1f78 commit 94aefb2

21 files changed

+674
-235
lines changed

‎.chloggen/refactor-bridge-config.yaml

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
2+
change_type: breaking
3+
4+
# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
5+
component: OpAMP Bridge
6+
7+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
8+
note: This PR simplifies the bridge's configuration and logging by renaming and removing fields.
9+
10+
# One or more tracking issues related to the change
11+
issues: [1368]
12+
13+
# (Optional) One or more lines of additional information to render under the primary note.
14+
# These lines will be padded with 2 spaces and then inserted directly into the document.
15+
# Use pipe (|) for multiline entries.
16+
subtext: |
17+
`components_allowed` => `componentsAllowed`
18+
:x: `protocol` which is now inferred from endpoint
19+
capabilities `[]string` => `map[Capability]bool` for enhanced configuration validation

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

+26-22
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,10 @@ package agent
1717
import (
1818
"bytes"
1919
"context"
20+
"fmt"
2021
"time"
2122

23+
"github.com/go-logr/logr"
2224
"gopkg.in/yaml.v3"
2325

2426
"github.com/open-telemetry/opentelemetry-operator/cmd/operator-opamp-bridge/metrics"
@@ -35,7 +37,7 @@ import (
3537
)
3638

3739
type Agent struct {
38-
logger types.Logger
40+
logger logr.Logger
3941

4042
appliedKeys map[collectorKey]bool
4143
startTime uint64
@@ -47,12 +49,12 @@ type Agent struct {
4749

4850
opampClient client.OpAMPClient
4951
metricReporter *metrics.MetricReporter
50-
config config.Config
52+
config *config.Config
5153
applier operator.ConfigApplier
5254
remoteConfigEnabled bool
5355
}
5456

55-
func NewAgent(logger types.Logger, applier operator.ConfigApplier, config config.Config, opampClient client.OpAMPClient) *Agent {
57+
func NewAgent(logger logr.Logger, applier operator.ConfigApplier, config *config.Config, opampClient client.OpAMPClient) *Agent {
5658
agent := &Agent{
5759
config: config,
5860
applier: applier,
@@ -64,8 +66,10 @@ func NewAgent(logger types.Logger, applier operator.ConfigApplier, config config
6466
opampClient: opampClient,
6567
}
6668

67-
agent.logger.Debugf("Agent created, id=%v, type=%s, version=%s.",
68-
agent.instanceId.String(), config.GetAgentType(), config.GetAgentVersion())
69+
agent.logger.V(3).Info("Agent created",
70+
"instanceId", agent.instanceId.String(),
71+
"agentType", config.GetAgentType(),
72+
"agentVersion", config.GetAgentVersion())
6973

7074
return agent
7175
}
@@ -81,17 +85,17 @@ func (agent *Agent) getHealth() *protobufs.AgentHealth {
8185

8286
// onConnect is called when an agent is successfully connected to a server.
8387
func (agent *Agent) onConnect() {
84-
agent.logger.Debugf("Connected to the server.")
88+
agent.logger.V(3).Info("Connected to the server.")
8589
}
8690

8791
// onConnectFailed is called when an agent was unable to connect to a server.
8892
func (agent *Agent) onConnectFailed(err error) {
89-
agent.logger.Errorf("Failed to connect to the server: %v", err)
93+
agent.logger.Error(err, "failed to connect to the server")
9094
}
9195

9296
// onError is called when an agent receives an error response from the server.
9397
func (agent *Agent) onError(err *protobufs.ServerErrorResponse) {
94-
agent.logger.Errorf("Server returned an error response: %v", err.ErrorMessage)
98+
agent.logger.Error(fmt.Errorf(err.GetErrorMessage()), "server returned an error response")
9599
}
96100

97101
// saveRemoteConfigStatus receives a status from the server when the server sets a remote configuration.
@@ -126,24 +130,24 @@ func (agent *Agent) Start() error {
126130
return err
127131
}
128132

129-
agent.logger.Debugf("Starting OpAMP client...")
133+
agent.logger.V(3).Info("Starting OpAMP client...")
130134

131135
err = agent.opampClient.Start(context.Background(), settings)
132136
if err != nil {
133137
return err
134138
}
135139

136-
agent.logger.Debugf("OpAMP Client started.")
140+
agent.logger.V(3).Info("OpAMP Client started.")
137141

138142
return nil
139143
}
140144

141145
// updateAgentIdentity receives a new instanced Id from the remote server and updates the agent's instanceID field.
142146
// The meter will be reinitialized by the onMessage function.
143147
func (agent *Agent) updateAgentIdentity(instanceId ulid.ULID) {
144-
agent.logger.Debugf("Agent identity is being changed from id=%v to id=%v",
145-
agent.instanceId.String(),
146-
instanceId.String())
148+
agent.logger.V(3).Info("Agent identity is being changed",
149+
"old instanceId", agent.instanceId.String(),
150+
"new instanceid", instanceId.String())
147151
agent.instanceId = instanceId
148152
}
149153

@@ -152,14 +156,14 @@ func (agent *Agent) updateAgentIdentity(instanceId ulid.ULID) {
152156
func (agent *Agent) getEffectiveConfig(ctx context.Context) (*protobufs.EffectiveConfig, error) {
153157
instances, err := agent.applier.ListInstances()
154158
if err != nil {
155-
agent.logger.Errorf("couldn't list instances", err)
159+
agent.logger.Error(err, "failed to list instances")
156160
return nil, err
157161
}
158162
instanceMap := map[string]*protobufs.AgentConfigFile{}
159163
for _, instance := range instances {
160164
marshaled, err := yaml.Marshal(instance)
161165
if err != nil {
162-
agent.logger.Errorf("couldn't marshal collector configuration", err)
166+
agent.logger.Error(err, "failed to marhsal config")
163167
return nil, err
164168
}
165169
mapKey := newCollectorKey(instance.GetName(), instance.GetNamespace())
@@ -181,7 +185,7 @@ func (agent *Agent) getEffectiveConfig(ctx context.Context) (*protobufs.Effectiv
181185
func (agent *Agent) initMeter(settings *protobufs.TelemetryConnectionSettings) {
182186
reporter, err := metrics.NewMetricReporter(agent.logger, settings, agent.config.GetAgentType(), agent.config.GetAgentVersion(), agent.instanceId)
183187
if err != nil {
184-
agent.logger.Errorf("Cannot collect metrics: %v", err)
188+
agent.logger.Error(err, "failed to create metric reporter")
185189
return
186190
}
187191

@@ -244,11 +248,11 @@ func (agent *Agent) applyRemoteConfig(config *protobufs.AgentRemoteConfig) (*pro
244248

245249
// Shutdown will stop the OpAMP client gracefully.
246250
func (agent *Agent) Shutdown() {
247-
agent.logger.Debugf("Agent shutting down...")
251+
agent.logger.V(3).Info("Agent shutting down...")
248252
if agent.opampClient != nil {
249253
err := agent.opampClient.Stop(context.Background())
250254
if err != nil {
251-
agent.logger.Errorf(err.Error())
255+
agent.logger.Error(err, "failed to stop client")
252256
}
253257
}
254258
if agent.metricReporter != nil {
@@ -265,16 +269,16 @@ func (agent *Agent) onMessage(ctx context.Context, msg *types.MessageData) {
265269
var err error
266270
status, err := agent.applyRemoteConfig(msg.RemoteConfig)
267271
if err != nil {
268-
agent.logger.Errorf(err.Error())
272+
agent.logger.Error(err, "failed to apply remote config")
269273
}
270274
err = agent.opampClient.SetRemoteConfigStatus(status)
271275
if err != nil {
272-
agent.logger.Errorf(err.Error())
276+
agent.logger.Error(err, "failed to set remote config status")
273277
return
274278
}
275279
err = agent.opampClient.UpdateEffectiveConfig(ctx)
276280
if err != nil {
277-
agent.logger.Errorf(err.Error())
281+
agent.logger.Error(err, "failed to update effective config")
278282
}
279283
}
280284

@@ -283,7 +287,7 @@ func (agent *Agent) onMessage(ctx context.Context, msg *types.MessageData) {
283287
if msg.AgentIdentification != nil {
284288
newInstanceId, err := ulid.Parse(msg.AgentIdentification.NewInstanceUid)
285289
if err != nil {
286-
agent.logger.Errorf(err.Error())
290+
agent.logger.Error(err, "couldn't parse instance UID")
287291
return
288292
}
289293
agent.updateAgentIdentity(newInstanceId)

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

+47-13
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import (
2222
"sort"
2323
"testing"
2424

25+
"github.com/go-logr/logr"
26+
"github.com/spf13/pflag"
2527
"github.com/stretchr/testify/require"
2628

2729
"github.com/oklog/ulid/v2"
@@ -32,18 +34,15 @@ import (
3234
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3335
"k8s.io/apimachinery/pkg/runtime"
3436
"sigs.k8s.io/controller-runtime/pkg/client/fake"
35-
logf "sigs.k8s.io/controller-runtime/pkg/log"
3637

3738
"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
3839
"github.com/open-telemetry/opentelemetry-operator/cmd/operator-opamp-bridge/config"
39-
"github.com/open-telemetry/opentelemetry-operator/cmd/operator-opamp-bridge/logger"
4040
"github.com/open-telemetry/opentelemetry-operator/cmd/operator-opamp-bridge/operator"
4141
)
4242

4343
var (
44-
l = logf.Log.WithName("agent-tests")
45-
clientLogger = logger.NewLogger(&l)
46-
_ client.OpAMPClient = &mockOpampClient{}
44+
l = logr.Discard()
45+
_ client.OpAMPClient = &mockOpampClient{}
4746
)
4847

4948
type mockOpampClient struct {
@@ -91,7 +90,7 @@ func (m *mockOpampClient) SetPackageStatuses(statuses *protobufs.PackageStatuses
9190
return nil
9291
}
9392

94-
func getFakeApplier(t *testing.T, conf config.Config) *operator.Client {
93+
func getFakeApplier(t *testing.T, conf *config.Config) *operator.Client {
9594
schemeBuilder := runtime.NewSchemeBuilder(func(s *runtime.Scheme) error {
9695
s.AddKnownTypes(v1alpha1.GroupVersion, &v1alpha1.OpenTelemetryCollector{}, &v1alpha1.OpenTelemetryCollectorList{})
9796
metav1.AddToGroupVersion(s, v1alpha1.GroupVersion)
@@ -173,6 +172,34 @@ func TestAgent_onMessage(t *testing.T) {
173172
},
174173
},
175174
},
175+
{
176+
name: "base case http",
177+
fields: fields{
178+
configFile: "testdata/agenthttpbasic.yaml",
179+
},
180+
args: args{
181+
ctx: context.Background(),
182+
configFile: map[string]string{
183+
"good/testnamespace": "basic.yaml",
184+
},
185+
},
186+
want: want{
187+
contents: map[string][]string{
188+
"good/testnamespace": {
189+
"kind: OpenTelemetryCollector",
190+
"name: good",
191+
"namespace: testnamespace",
192+
"send_batch_size: 10000",
193+
"receivers: [otlp]",
194+
"status:",
195+
},
196+
},
197+
status: &protobufs.RemoteConfigStatus{
198+
LastRemoteConfigHash: []byte("good/testnamespace405"),
199+
Status: protobufs.RemoteConfigStatuses_RemoteConfigStatuses_APPLIED,
200+
},
201+
},
202+
},
176203
{
177204
name: "failure",
178205
fields: fields{
@@ -448,11 +475,12 @@ func TestAgent_onMessage(t *testing.T) {
448475
for _, tt := range tests {
449476
t.Run(tt.name, func(t *testing.T) {
450477
mockClient := &mockOpampClient{}
451-
conf, err := config.Load(tt.fields.configFile)
452-
require.NoError(t, err, "should be able to load config")
478+
conf := config.NewConfig(logr.Discard())
479+
loadErr := config.LoadFromFile(conf, tt.fields.configFile)
480+
require.NoError(t, loadErr, "should be able to load config")
453481
applier := getFakeApplier(t, conf)
454-
agent := NewAgent(clientLogger, applier, conf, mockClient)
455-
err = agent.Start()
482+
agent := NewAgent(l, applier, conf, mockClient)
483+
err := agent.Start()
456484
defer agent.Shutdown()
457485
require.NoError(t, err, "should be able to start agent")
458486
data, err := getMessageDataFromConfigFile(tt.args.configFile)
@@ -498,10 +526,16 @@ func TestAgent_onMessage(t *testing.T) {
498526

499527
func Test_CanUpdateIdentity(t *testing.T) {
500528
mockClient := &mockOpampClient{}
501-
conf, err := config.Load("testdata/agent.yaml")
502-
require.NoError(t, err, "should be able to load config")
529+
530+
fs := config.GetFlagSet(pflag.ContinueOnError)
531+
configFlag := []string{"--config-file", "testdata/agent.yaml"}
532+
err := fs.Parse(configFlag)
533+
assert.NoError(t, err)
534+
conf := config.NewConfig(logr.Discard())
535+
loadErr := config.LoadFromFile(conf, "testdata/agent.yaml")
536+
require.NoError(t, loadErr, "should be able to load config")
503537
applier := getFakeApplier(t, conf)
504-
agent := NewAgent(clientLogger, applier, conf, mockClient)
538+
agent := NewAgent(l, applier, conf, mockClient)
505539
err = agent.Start()
506540
defer agent.Shutdown()
507541
require.NoError(t, err, "should be able to start agent")

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

+12-13
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
11
endpoint: ws://127.0.0.1:4320/v1/opamp
2-
protocol: wss
32
capabilities:
4-
- AcceptsRemoteConfig
5-
- ReportsEffectiveConfig
6-
# - AcceptsPackages
7-
# - ReportsPackageStatuses
8-
- ReportsOwnTraces
9-
- ReportsOwnMetrics
10-
- ReportsOwnLogs
11-
- AcceptsOpAMPConnectionSettings
12-
- AcceptsOtherConnectionSettings
13-
- AcceptsRestartCommand
14-
- ReportsHealth
15-
- ReportsRemoteConfig
3+
AcceptsRemoteConfig: true
4+
ReportsEffectiveConfig: true
5+
AcceptsPackages: false
6+
ReportsPackageStatuses: false
7+
ReportsOwnTraces: true
8+
ReportsOwnMetrics: true
9+
ReportsOwnLogs: true
10+
AcceptsOpAMPConnectionSettings: true
11+
AcceptsOtherConnectionSettings: true
12+
AcceptsRestartCommand: true
13+
ReportsHealth: true
14+
ReportsRemoteConfig: true

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

+13-14
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,18 @@
11
endpoint: ws://127.0.0.1:4320/v1/opamp
2-
protocol: wss
32
capabilities:
4-
- AcceptsRemoteConfig
5-
- ReportsEffectiveConfig
6-
# - AcceptsPackages
7-
# - ReportsPackageStatuses
8-
- ReportsOwnTraces
9-
- ReportsOwnMetrics
10-
- ReportsOwnLogs
11-
- AcceptsOpAMPConnectionSettings
12-
- AcceptsOtherConnectionSettings
13-
- AcceptsRestartCommand
14-
- ReportsHealth
15-
- ReportsRemoteConfig
16-
components_allowed:
3+
AcceptsRemoteConfig: true
4+
ReportsEffectiveConfig: true
5+
AcceptsPackages: false
6+
ReportsPackageStatuses: false
7+
ReportsOwnTraces: true
8+
ReportsOwnMetrics: true
9+
ReportsOwnLogs: true
10+
AcceptsOpAMPConnectionSettings: true
11+
AcceptsOtherConnectionSettings: true
12+
AcceptsRestartCommand: true
13+
ReportsHealth: true
14+
ReportsRemoteConfig: true
15+
componentsAllowed:
1716
receivers:
1817
- otlp
1918
processors:

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

+13-14
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,18 @@
11
endpoint: ws://127.0.0.1:4320/v1/opamp
2-
protocol: wss
32
capabilities:
4-
- AcceptsRemoteConfig
5-
- ReportsEffectiveConfig
6-
# - AcceptsPackages
7-
# - ReportsPackageStatuses
8-
- ReportsOwnTraces
9-
- ReportsOwnMetrics
10-
- ReportsOwnLogs
11-
- AcceptsOpAMPConnectionSettings
12-
- AcceptsOtherConnectionSettings
13-
- AcceptsRestartCommand
14-
- ReportsHealth
15-
- ReportsRemoteConfig
16-
components_allowed:
3+
AcceptsRemoteConfig: true
4+
ReportsEffectiveConfig: true
5+
AcceptsPackages: false
6+
ReportsPackageStatuses: false
7+
ReportsOwnTraces: true
8+
ReportsOwnMetrics: true
9+
ReportsOwnLogs: true
10+
AcceptsOpAMPConnectionSettings: true
11+
AcceptsOtherConnectionSettings: true
12+
AcceptsRestartCommand: true
13+
ReportsHealth: true
14+
ReportsRemoteConfig: true
15+
componentsAllowed:
1716
receivers:
1817
- otlp
1918
processors:

0 commit comments

Comments
 (0)