Skip to content

Commit b7beae5

Browse files
tigrannajaryantigrannajaryan
authored
Breaking: Replace Callbacks interface by Callbacks struct (#324)
The interface has the following downsides: - Impossible to define non-trivial default behavior. Here is an example where it was needed: #269 (comment) - Adding new callbacks requires expanding the interface, which is a breaking change for existing client users. Getting rid of the interface and keeping just a struct for callbacks solves both problems: - Arbitrarily complex default behavior can be now defined on the struct if the user does not provide the particular callback func. - Adding new callback funcs is not a braking change, existing users won't be affected.
1 parent f7d56df commit b7beae5

File tree

10 files changed

+195
-255
lines changed

10 files changed

+195
-255
lines changed

‎client/clientimpl_test.go

+36-36
Original file line numberDiff line numberDiff line change
@@ -167,8 +167,8 @@ func TestOnConnectFail(t *testing.T) {
167167
testClients(t, func(t *testing.T, client OpAMPClient) {
168168
var connectErr atomic.Value
169169
settings := createNoServerSettings()
170-
settings.Callbacks = types.CallbacksStruct{
171-
OnConnectFailedFunc: func(ctx context.Context, err error) {
170+
settings.Callbacks = types.Callbacks{
171+
OnConnectFailed: func(ctx context.Context, err error) {
172172
connectErr.Store(err)
173173
},
174174
}
@@ -244,8 +244,8 @@ func TestConnectWithServer(t *testing.T) {
244244
// Start a client.
245245
var connected int64
246246
settings := types.StartSettings{
247-
Callbacks: types.CallbacksStruct{
248-
OnConnectFunc: func(ctx context.Context) {
247+
Callbacks: types.Callbacks{
248+
OnConnect: func(ctx context.Context) {
249249
atomic.StoreInt64(&connected, 1)
250250
},
251251
},
@@ -282,12 +282,12 @@ func TestConnectWithServer503(t *testing.T) {
282282
var clientConnected int64
283283
var connectErr atomic.Value
284284
settings := types.StartSettings{
285-
Callbacks: types.CallbacksStruct{
286-
OnConnectFunc: func(ctx context.Context) {
285+
Callbacks: types.Callbacks{
286+
OnConnect: func(ctx context.Context) {
287287
atomic.StoreInt64(&clientConnected, 1)
288288
assert.Fail(t, "Client should not be able to connect")
289289
},
290-
OnConnectFailedFunc: func(ctx context.Context, err error) {
290+
OnConnectFailed: func(ctx context.Context, err error) {
291291
connectErr.Store(err)
292292
},
293293
},
@@ -484,11 +484,11 @@ func TestFirstStatusReport(t *testing.T) {
484484
// Start a client.
485485
var connected, remoteConfigReceived int64
486486
settings := types.StartSettings{
487-
Callbacks: types.CallbacksStruct{
488-
OnConnectFunc: func(ctx context.Context) {
487+
Callbacks: types.Callbacks{
488+
OnConnect: func(ctx context.Context) {
489489
atomic.AddInt64(&connected, 1)
490490
},
491-
OnMessageFunc: func(ctx context.Context, msg *types.MessageData) {
491+
OnMessage: func(ctx context.Context, msg *types.MessageData) {
492492
// Verify that the client received exactly the remote config that
493493
// the Server sent.
494494
assert.True(t, proto.Equal(remoteConfig, msg.RemoteConfig))
@@ -537,8 +537,8 @@ func TestIncludesDetailsOnReconnect(t *testing.T) {
537537

538538
var connected int64
539539
settings := types.StartSettings{
540-
Callbacks: types.CallbacksStruct{
541-
OnConnectFunc: func(ctx context.Context) {
540+
Callbacks: types.Callbacks{
541+
OnConnect: func(ctx context.Context) {
542542
atomic.AddInt64(&connected, 1)
543543
},
544544
},
@@ -589,8 +589,8 @@ func TestSetEffectiveConfig(t *testing.T) {
589589
// Start a client.
590590
sendConfig := createEffectiveConfig()
591591
settings := types.StartSettings{
592-
Callbacks: types.CallbacksStruct{
593-
GetEffectiveConfigFunc: func(ctx context.Context) (*protobufs.EffectiveConfig, error) {
592+
Callbacks: types.Callbacks{
593+
GetEffectiveConfig: func(ctx context.Context) (*protobufs.EffectiveConfig, error) {
594594
return sendConfig, nil
595595
},
596596
},
@@ -822,8 +822,8 @@ func TestServerOfferConnectionSettings(t *testing.T) {
822822

823823
// Start a client.
824824
settings := types.StartSettings{
825-
Callbacks: types.CallbacksStruct{
826-
OnMessageFunc: func(ctx context.Context, msg *types.MessageData) {
825+
Callbacks: types.Callbacks{
826+
OnMessage: func(ctx context.Context, msg *types.MessageData) {
827827
assert.True(t, proto.Equal(metricsSettings, msg.OwnMetricsConnSettings))
828828
assert.True(t, proto.Equal(tracesSettings, msg.OwnTracesConnSettings))
829829
assert.True(t, proto.Equal(logsSettings, msg.OwnLogsConnSettings))
@@ -834,7 +834,7 @@ func TestServerOfferConnectionSettings(t *testing.T) {
834834
atomic.AddInt64(&gotOtherSettings, 1)
835835
},
836836

837-
OnOpampConnectionSettingsFunc: func(
837+
OnOpampConnectionSettings: func(
838838
ctx context.Context, settings *protobufs.OpAMPConnectionSettings,
839839
) error {
840840
assert.True(t, proto.Equal(opampSettings, settings))
@@ -891,8 +891,8 @@ func TestClientRequestConnectionSettings(t *testing.T) {
891891

892892
// Start a client.
893893
settings := types.StartSettings{
894-
Callbacks: types.CallbacksStruct{
895-
OnOpampConnectionSettingsFunc: func(
894+
Callbacks: types.Callbacks{
895+
OnOpampConnectionSettings: func(
896896
ctx context.Context, settings *protobufs.OpAMPConnectionSettings,
897897
) error {
898898
assert.True(t, proto.Equal(opampSettings, settings))
@@ -1073,8 +1073,8 @@ func TestReportEffectiveConfig(t *testing.T) {
10731073
// Start a client.
10741074
settings := types.StartSettings{
10751075
OpAMPServerURL: "ws://" + srv.Endpoint,
1076-
Callbacks: types.CallbacksStruct{
1077-
GetEffectiveConfigFunc: func(ctx context.Context) (*protobufs.EffectiveConfig, error) {
1076+
Callbacks: types.Callbacks{
1077+
GetEffectiveConfig: func(ctx context.Context) (*protobufs.EffectiveConfig, error) {
10781078
return clientEffectiveConfig, nil
10791079
},
10801080
},
@@ -1139,8 +1139,8 @@ func verifyRemoteConfigUpdate(t *testing.T, successCase bool, expectStatus *prot
11391139
// Start a client.
11401140
settings := types.StartSettings{
11411141
OpAMPServerURL: "ws://" + srv.Endpoint,
1142-
Callbacks: types.CallbacksStruct{
1143-
OnMessageFunc: func(ctx context.Context, msg *types.MessageData) {
1142+
Callbacks: types.Callbacks{
1143+
OnMessage: func(ctx context.Context, msg *types.MessageData) {
11441144
if msg.RemoteConfig != nil {
11451145
if successCase {
11461146
client.SetRemoteConfigStatus(
@@ -1355,8 +1355,8 @@ func verifyUpdatePackages(t *testing.T, testCase packageTestCase) {
13551355
// Start a client.
13561356
settings := types.StartSettings{
13571357
OpAMPServerURL: "ws://" + srv.Endpoint,
1358-
Callbacks: types.CallbacksStruct{
1359-
OnMessageFunc: onMessageFunc,
1358+
Callbacks: types.Callbacks{
1359+
OnMessage: onMessageFunc,
13601360
},
13611361
PackagesStateProvider: localPackageState,
13621362
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages |
@@ -1542,8 +1542,8 @@ func TestMissingCapabilities(t *testing.T) {
15421542

15431543
// Start a client.
15441544
settings := types.StartSettings{
1545-
Callbacks: types.CallbacksStruct{
1546-
OnMessageFunc: func(ctx context.Context, msg *types.MessageData) {
1545+
Callbacks: types.Callbacks{
1546+
OnMessage: func(ctx context.Context, msg *types.MessageData) {
15471547
// These fields must not be set since we did not define the capabilities to accept them.
15481548
assert.Nil(t, msg.RemoteConfig)
15491549
assert.Nil(t, msg.OwnLogsConnSettings)
@@ -1552,7 +1552,7 @@ func TestMissingCapabilities(t *testing.T) {
15521552
assert.Nil(t, msg.OtherConnSettings)
15531553
assert.Nil(t, msg.PackagesAvailable)
15541554
},
1555-
OnOpampConnectionSettingsFunc: func(
1555+
OnOpampConnectionSettings: func(
15561556
ctx context.Context, settings *protobufs.OpAMPConnectionSettings,
15571557
) error {
15581558
assert.Fail(t, "should not be called since capability is not set to accept it")
@@ -1613,7 +1613,7 @@ func TestMissingPackagesStateProvider(t *testing.T) {
16131613
testClients(t, func(t *testing.T, client OpAMPClient) {
16141614
// Start a client.
16151615
settings := types.StartSettings{
1616-
Callbacks: types.CallbacksStruct{},
1616+
Callbacks: types.Callbacks{},
16171617
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages |
16181618
protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
16191619
}
@@ -1624,7 +1624,7 @@ func TestMissingPackagesStateProvider(t *testing.T) {
16241624
// Start a client.
16251625
localPackageState := internal.NewInMemPackagesStore()
16261626
settings = types.StartSettings{
1627-
Callbacks: types.CallbacksStruct{},
1627+
Callbacks: types.Callbacks{},
16281628
PackagesStateProvider: localPackageState,
16291629
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages,
16301630
}
@@ -1634,7 +1634,7 @@ func TestMissingPackagesStateProvider(t *testing.T) {
16341634

16351635
// Start a client.
16361636
settings = types.StartSettings{
1637-
Callbacks: types.CallbacksStruct{},
1637+
Callbacks: types.Callbacks{},
16381638
PackagesStateProvider: localPackageState,
16391639
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
16401640
}
@@ -1668,8 +1668,8 @@ func TestOfferUpdatedVersion(t *testing.T) {
16681668
// Start a client.
16691669
settings := types.StartSettings{
16701670
OpAMPServerURL: "ws://" + srv.Endpoint,
1671-
Callbacks: types.CallbacksStruct{
1672-
OnMessageFunc: onMessageFunc,
1671+
Callbacks: types.Callbacks{
1672+
OnMessage: onMessageFunc,
16731673
},
16741674
PackagesStateProvider: localPackageState,
16751675
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages |
@@ -1747,8 +1747,8 @@ func TestReportCustomCapabilities(t *testing.T) {
17471747
// Start a client.
17481748
settings := types.StartSettings{
17491749
OpAMPServerURL: "ws://" + srv.Endpoint,
1750-
Callbacks: types.CallbacksStruct{
1751-
OnMessageFunc: func(ctx context.Context, msg *types.MessageData) {
1750+
Callbacks: types.Callbacks{
1751+
OnMessage: func(ctx context.Context, msg *types.MessageData) {
17521752
clientRcvCustomMessage.Store(msg.CustomMessage)
17531753
},
17541754
},
@@ -1843,7 +1843,7 @@ func TestReportCustomCapabilities(t *testing.T) {
18431843
func TestSendCustomMessage(t *testing.T) {
18441844
testClients(t, func(t *testing.T, client OpAMPClient) {
18451845
settings := types.StartSettings{
1846-
Callbacks: types.CallbacksStruct{},
1846+
Callbacks: types.Callbacks{},
18471847
}
18481848
prepareClient(t, &settings, client)
18491849
clientCustomCapabilities := &protobufs.CustomCapabilities{

‎client/internal/clientcommon.go

+1-4
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,7 @@ func (c *ClientCommon) PrepareStart(
132132

133133
// Prepare callbacks.
134134
c.Callbacks = settings.Callbacks
135-
if c.Callbacks == nil {
136-
// Make sure it is always safe to call Callbacks.
137-
c.Callbacks = types.CallbacksStruct{}
138-
}
135+
c.Callbacks.SetDefaults()
139136

140137
if c.Capabilities&protobufs.AgentCapabilities_AgentCapabilities_ReportsHeartbeat != 0 && settings.HeartbeatInterval != nil {
141138
if err := c.sender.SetHeartbeatInterval(*settings.HeartbeatInterval); err != nil {

‎client/internal/httpsender_test.go

+14-12
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,12 @@ import (
1212
"testing"
1313
"time"
1414

15+
"github.com/stretchr/testify/assert"
16+
1517
"github.com/open-telemetry/opamp-go/client/types"
1618
sharedinternal "github.com/open-telemetry/opamp-go/internal"
1719
"github.com/open-telemetry/opamp-go/internal/testhelpers"
1820
"github.com/open-telemetry/opamp-go/protobufs"
19-
"github.com/stretchr/testify/assert"
2021
)
2122

2223
func TestHTTPSenderRetryForStatusTooManyRequests(t *testing.T) {
@@ -46,10 +47,10 @@ func TestHTTPSenderRetryForStatusTooManyRequests(t *testing.T) {
4647
}},
4748
}
4849
})
49-
sender.callbacks = types.CallbacksStruct{
50-
OnConnectFunc: func(ctx context.Context) {
50+
sender.callbacks = types.Callbacks{
51+
OnConnect: func(ctx context.Context) {
5152
},
52-
OnConnectFailedFunc: func(ctx context.Context, _ error) {
53+
OnConnectFailed: func(ctx context.Context, _ error) {
5354
},
5455
}
5556
sender.url = url
@@ -163,10 +164,10 @@ func TestHTTPSenderRetryForFailedRequests(t *testing.T) {
163164
}},
164165
}
165166
})
166-
sender.callbacks = types.CallbacksStruct{
167-
OnConnectFunc: func(ctx context.Context) {
167+
sender.callbacks = types.Callbacks{
168+
OnConnect: func(ctx context.Context) {
168169
},
169-
OnConnectFailedFunc: func(ctx context.Context, _ error) {
170+
OnConnectFailed: func(ctx context.Context, _ error) {
170171
},
171172
}
172173
sender.url = url
@@ -197,7 +198,8 @@ func TestRequestInstanceUidFlagReset(t *testing.T) {
197198
ctx, cancel := context.WithCancel(context.Background())
198199

199200
sender := NewHTTPSender(&sharedinternal.NopLogger{})
200-
sender.callbacks = types.CallbacksStruct{}
201+
sender.callbacks = types.Callbacks{}
202+
sender.callbacks.SetDefaults()
201203

202204
// Set the RequestInstanceUid flag on the tracked state to request the server for a new ID to use.
203205
clientSyncedState := &ClientSyncedState{}
@@ -248,8 +250,8 @@ func TestPackageUpdatesInParallel(t *testing.T) {
248250

249251
var messages atomic.Int32
250252
var mux sync.Mutex
251-
sender.callbacks = types.CallbacksStruct{
252-
OnMessageFunc: func(ctx context.Context, msg *types.MessageData) {
253+
sender.callbacks = types.Callbacks{
254+
OnMessage: func(ctx context.Context, msg *types.MessageData) {
253255
err := msg.PackageSyncer.Sync(ctx)
254256
assert.NoError(t, err)
255257
messages.Add(1)
@@ -320,8 +322,8 @@ func TestPackageUpdatesWithError(t *testing.T) {
320322
localPackageState := types.PackagesStateProvider(nil)
321323
var messages atomic.Int32
322324
var mux sync.Mutex
323-
sender.callbacks = types.CallbacksStruct{
324-
OnMessageFunc: func(ctx context.Context, msg *types.MessageData) {
325+
sender.callbacks = types.Callbacks{
326+
OnMessage: func(ctx context.Context, msg *types.MessageData) {
325327
// Make sure the call to Sync will return an error due to a nil PackageStateProvider
326328
err := msg.PackageSyncer.Sync(ctx)
327329
assert.Error(t, err)

0 commit comments

Comments
 (0)