Skip to content

Make it possible to set capabilities #366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Make it possible to set capabilities #366

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
adcb1be
Make it possible to set capabilities
jaronoff97 Mar 24, 2025
cf4a028
formatting
jaronoff97 Mar 24, 2025
21667d9
update prototype
jaronoff97 Mar 27, 2025
3f61b9c
update from new spec changes
jaronoff97 Apr 16, 2025
4b81c39
Update to use setcapabilities and deprecate the old method
jaronoff97 May 26, 2025
a130307
update from feedback
jaronoff97 Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,17 @@ type OpAMPClient interface {
// This method is subject to agent status compression - if components is not
// different from the cached agent state, this method is a no-op.
SetAvailableComponents(components *protobufs.AvailableComponents) error

// SetCapabilities updates the set of capabilities that the client supports.
// These capabilities will be communicated to the server in the next message.
//
// This method can be called at any time after Start(), including from within
// an OnMessage handler, to dynamically update the set of supported capabilities.
// The updated capabilities will be sent to the server in the next outgoing message.
//
// The capabilities parameter must not be nil; passing a nil value will result in an error.
//
// For more details, refer to the OpAMP specification:
// https://github.com/open-telemetry/opamp-spec/blob/main/specification.md#agenttoservercapabilities
SetCapabilities(capabilities *protobufs.AgentCapabilities) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This results in inconsistent way of setting capabilities depending on when you want to do it: before Start() requires setting a field in StartSettings, after Start() requires calling this method.

For functionality that MUST be set before Start() and MAY be set after Start() we previously used only a method. See for example SetAgentDescription().

Should we do the same for SetCapabilities() and deprecate Capabilities field from StartSettings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I think that makes sense to me. @andykellr does that sound alright to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaronoff97 I am not sure if you updated the PR to make this change. Is it ready for another review round?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was waiting on andy's review and when you said "we decided to go ahead with this feature" I thought it was dismissing this comment. I see now, i'll update the PR based on what you said above in this thread.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no problem, I just wasn't sure if I need to take another look.
I would like @andykellr's opinion too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaronoff97 in case I wasn't clear, I am waiting for you to tell me if this is ready for another round of review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was waiting on andy's opinion here, but either way i can make your requested changes from above before its ready for more review. apologies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaronoff97 sounds good, thanks. Ping me when you update the PR, I will take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tigrannajaryan i had two questions for the implementation below, but i think this should be good! Thank you for your patience.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach is fine with me.

}
230 changes: 228 additions & 2 deletions client/clientimpl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (

const retryAfterHTTPHeader = "Retry-After"

var coreCapabilities = protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus

func createAgentDescr() *protobufs.AgentDescription {
agentDescr := &protobufs.AgentDescription{
IdentifyingAttributes: []*protobufs.KeyValue{
Expand Down Expand Up @@ -129,6 +131,12 @@ func prepareClient(t *testing.T, settings *types.StartSettings, c OpAMPClient) {
prepareSettings(t, settings, c)
err := c.SetAgentDescription(createAgentDescr())
assert.NoError(t, err)
// We ignore the error here.
if settings.Capabilities != 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i had to ignore the error here because a later test depended on the checks happening in the start client

c.SetCapabilities(&settings.Capabilities)
} else {
c.SetCapabilities(&coreCapabilities)
}
}

func startClient(t *testing.T, settings types.StartSettings, client OpAMPClient) {
Expand Down Expand Up @@ -218,13 +226,26 @@ func TestStopCancellation(t *testing.T) {

func TestStartNoDescription(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
setErr := client.SetCapabilities(&coreCapabilities)
assert.NoError(t, setErr)
settings := createNoServerSettings()
prepareSettings(t, &settings, client)
err := client.Start(context.Background(), settings)
assert.EqualValues(t, err, internal.ErrAgentDescriptionMissing)
})
}

func TestStartNoCapabilities(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
setErr := client.SetAgentDescription(createAgentDescr())
require.NoError(t, setErr)
settings := createNoServerSettings()
prepareSettings(t, &settings, client)
err := client.Start(context.Background(), settings)
assert.EqualValues(t, internal.ErrCapabilitiesNotSet, err)
})
}

func TestSetInvalidAgentDescription(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
settings := createNoServerSettings()
Expand Down Expand Up @@ -611,7 +632,8 @@ func TestSetEffectiveConfig(t *testing.T) {

// Now change the config.
sendConfig.ConfigMap.ConfigMap["key2"] = &protobufs.AgentConfigFile{}
_ = client.UpdateEffectiveConfig(context.Background())
updateErr := client.UpdateEffectiveConfig(context.Background())
require.NoError(t, updateErr)

// Verify change is delivered.
eventually(
Expand Down Expand Up @@ -1621,7 +1643,6 @@ func TestMissingPackagesStateProvider(t *testing.T) {
protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
}
prepareClient(t, &settings, client)

assert.ErrorIs(t, client.Start(context.Background(), settings), internal.ErrPackagesStateProviderNotSet)

// Start a client.
Expand Down Expand Up @@ -1738,6 +1759,98 @@ func TestOfferUpdatedVersion(t *testing.T) {
})
}

func TestSetCapabilities(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
// Start a Server.
srv := internal.StartMockServer(t)
srv.EnableExpectMode()

var clientRcvCustomMessage atomic.Value

// Start a client.
settings := types.StartSettings{
OpAMPServerURL: "ws://" + srv.Endpoint,
Callbacks: types.Callbacks{
OnMessage: func(ctx context.Context, msg *types.MessageData) {
clientRcvCustomMessage.Store(msg.CustomMessage)
},
},
Capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig,
}
prepareClient(t, &settings, client)

// Client --->
assert.NoError(t, client.Start(context.Background(), settings))

// ---> Server
srv.Expect(func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
assert.EqualValues(t, 0, msg.SequenceNum)
// The first status report after Start must have the ReportsStatus.
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus != 0)
// The first status report after Start must have the ReportsEffectiveConfig.
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig != 0)
// The first status report after Start must not  have the AcceptsRemoteConfig.
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_AcceptsRemoteConfig == 0)
return &protobufs.ServerToAgent{
InstanceUid: msg.InstanceUid,
}
})

newCapabilities := protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig |
protobufs.AgentCapabilities_AgentCapabilities_AcceptsRemoteConfig |
protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus
err := client.SetCapabilities(&newCapabilities)
assert.NoError(t, err)

// ---> Server
srv.Expect(func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
// Check ReportsStatus is still true.
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus != 0)
// ReportsEffectiveConfig should no longer be present.
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig != 0)
// AcceptsRemoteConfig should now be present
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_AcceptsRemoteConfig != 0)

// Send a custom message response and ask client for full state again.
return &protobufs.ServerToAgent{
InstanceUid: msg.InstanceUid,
Flags: uint64(protobufs.ServerToAgentFlags_ServerToAgentFlags_ReportFullState),
}
})

newCapabilities = protobufs.AgentCapabilities_AgentCapabilities_AcceptsRestartCommand |
protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig |
protobufs.AgentCapabilities_AgentCapabilities_AcceptsRemoteConfig |
protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus
newSetErr := client.SetCapabilities(&newCapabilities)
assert.NoError(t, newSetErr)

// ---> Server
srv.Expect(func(msg *protobufs.AgentToServer) *protobufs.ServerToAgent {
// Check ReportsStatus is still true.
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_ReportsStatus != 0)
// ReportsEffectiveConfig should present.
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_ReportsEffectiveConfig != 0)
// AcceptsRemoteConfig should now be present
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_AcceptsRemoteConfig != 0)
// AcceptsRestartCommand should now be present
assert.True(t, protobufs.AgentCapabilities(msg.Capabilities)&protobufs.AgentCapabilities_AgentCapabilities_AcceptsRestartCommand != 0)
// Send a custom message response and ask client for full state again.
return &protobufs.ServerToAgent{
InstanceUid: msg.InstanceUid,
Flags: uint64(protobufs.ServerToAgentFlags_ServerToAgentFlags_ReportFullState),
}
})

// Shutdown the Server.
srv.Close()

// Shutdown the client.
err = client.Stop(context.Background())
assert.NoError(t, err)
})
}

func TestReportCustomCapabilities(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
// Start a Server.
Expand Down Expand Up @@ -2407,6 +2520,119 @@ func TestSetAvailableComponents(t *testing.T) {
}
}

func TestValidateCapabilities(t *testing.T) {
testCases := []struct {
name string
capabilities protobufs.AgentCapabilities
setupFunc func(t *testing.T, client OpAMPClient)
expectedError error
}{
{
name: "ReportsHealth capability without health",
capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsHealth,
setupFunc: func(t *testing.T, client OpAMPClient) {
// Do not set health
},
expectedError: internal.ErrHealthMissing,
},
{
name: "ReportsHealth capability with health",
capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsHealth,
setupFunc: func(t *testing.T, client OpAMPClient) {
err := client.SetHealth(&protobufs.ComponentHealth{})
require.NoError(t, err)
},
expectedError: nil,
},
{
name: "ReportsAvailableComponents capability without available components",
capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsAvailableComponents,
setupFunc: func(t *testing.T, client OpAMPClient) {
// Do not set available components
},
expectedError: internal.ErrAvailableComponentsMissing,
},
{
name: "ReportsAvailableComponents capability with available components",
capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsAvailableComponents,
setupFunc: func(t *testing.T, client OpAMPClient) {
err := client.SetAvailableComponents(generateTestAvailableComponents())
require.NoError(t, err)
},
expectedError: nil,
},
{
name: "AcceptsPackages capability without PackagesStateProvider",
capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages,
setupFunc: func(t *testing.T, client OpAMPClient) {
// Do not set PackagesStateProvider
},
expectedError: internal.ErrPackagesStateProviderNotSet,
},
// {
// name: "AcceptsPackages capability with PackagesStateProvider",
// capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages,
// setupFunc: func(t *testing.T, client OpAMPClient) {
// client.SetPackageStatuses(statuses *protobufs.PackageStatuses)
// },
// expectedError: nil,
// },
{
name: "ReportsPackageStatuses capability without PackagesStateProvider",
capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
setupFunc: func(t *testing.T, client OpAMPClient) {
// Do not set PackagesStateProvider
},
expectedError: internal.ErrPackagesStateProviderNotSet,
},
// {
// name: "ReportsPackageStatuses capability with PackagesStateProvider",
// capabilities: protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
// setupFunc: func(t *testing.T, client OpAMPClient) {
// client.(*ClientCommon).PackagesStateProvider = internal.NewInMemPackagesStore()
// },
// expectedError: nil,
// },
{
name: "AcceptsPackages and ReportsPackageStatuses capabilities without PackagesStateProvider",
capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages | protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
setupFunc: func(t *testing.T, client OpAMPClient) {
// Do not set PackagesStateProvider
},
expectedError: internal.ErrPackagesStateProviderNotSet,
},
// {
// name: "AcceptsPackages and ReportsPackageStatuses capabilities with PackagesStateProvider",
// capabilities: protobufs.AgentCapabilities_AgentCapabilities_AcceptsPackages | protobufs.AgentCapabilities_AgentCapabilities_ReportsPackageStatuses,
// setupFunc: func(t *testing.T, client OpAMPClient) {
// client.(*ClientCommon).PackagesStateProvider = internal.NewInMemPackagesStore()
// },
// expectedError: nil,
// },
{
name: "No capabilities set",
capabilities: protobufs.AgentCapabilities_AgentCapabilities_Unspecified,
setupFunc: func(t *testing.T, client OpAMPClient) {
// No setup needed
},
expectedError: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
testClients(t, func(t *testing.T, client OpAMPClient) {
// Setup the client state as per the test case
tc.setupFunc(t, client)

// Validate capabilities
err := client.SetCapabilities(&tc.capabilities)
assert.Equal(t, tc.expectedError, err)
})
})
}
}

func generateTestAvailableComponents() *protobufs.AvailableComponents {
return &protobufs.AvailableComponents{
Hash: []byte("fake-hash"),
Expand Down
9 changes: 8 additions & 1 deletion client/httpclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"github.com/open-telemetry/opamp-go/protobufs"
)

var _ OpAMPClient = &httpClient{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We use both styles in the code, but this one is more common.

Suggested change
var _ OpAMPClient = &httpClient{}
var _ OpAMPClient = (*httpClient)(nil)


// httpClient is an OpAMP Client implementation for plain HTTP transport.
// See specification: https://github.com/open-telemetry/opamp-spec/blob/main/specification.md#plain-http-transport
type httpClient struct {
Expand Down Expand Up @@ -125,6 +127,11 @@ func (c *httpClient) SetAvailableComponents(components *protobufs.AvailableCompo
return c.common.SetAvailableComponents(components)
}

// SetCapabilities implements OpAMPClient.
func (c *httpClient) SetCapabilities(capabilities *protobufs.AgentCapabilities) error {
return c.common.SetCapabilities(capabilities)
}

func (c *httpClient) runUntilStopped(ctx context.Context) {
// Start the HTTP sender. This will make request/responses with retries for
// failures and will wait with configured polling interval if there is nothing
Expand All @@ -135,7 +142,7 @@ func (c *httpClient) runUntilStopped(ctx context.Context) {
c.common.Callbacks,
&c.common.ClientSyncedState,
c.common.PackagesStateProvider,
c.common.Capabilities,
c.common.ClientSyncedState.Capabilities(),
&c.common.PackageSyncMutex,
)
}
Expand Down
Loading
Loading