Make it possible to set capabilities #366

jaronoff97 · 2025-03-24T20:21:34Z

Closes #365

jaronoff97 · 2025-03-24T20:27:42Z

For context i need this to solve open-telemetry/opentelemetry-operator#3822. Introducing leader election to the bridge for HA would mean one of the bridge pods would need to change its capabilities while running. I could accomplish this by shutting down and then re-starting the opamp client, but that's very heavy and unnecessary IMO.

codecov · 2025-03-24T20:39:40Z

Codecov Report

Attention: Patch coverage is 81.81818% with 16 lines in your changes missing coverage. Please review.

Project coverage is 80.14%. Comparing base (4b62964) to head (a130307).
Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
client/internal/clientcommon.go	81.15%	8 Missing and 5 partials ⚠️
client/internal/clientstate.go	76.92%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #366      +/-   ##
==========================================
+ Coverage   80.10%   80.14%   +0.04%     
==========================================
  Files          25       26       +1     
  Lines        2423     2549     +126     
==========================================
+ Hits         1941     2043     +102     
- Misses        374      391      +17     
- Partials      108      115       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

andykellr · 2025-03-24T22:36:17Z

client/httpclient.go

@@ -10,6 +10,8 @@ import (
 	"github.com/open-telemetry/opamp-go/protobufs"
 )

+var _ OpAMPClient = &httpClient{}


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

Suggested change

var _ OpAMPClient = &httpClient{}

var _ OpAMPClient = (*httpClient)(nil)

andykellr · 2025-03-24T22:37:04Z

client/wsclient.go

@@ -22,6 +22,8 @@ const (
 	defaultShutdownTimeout = 5 * time.Second
 )

+var _ OpAMPClient = &wsClient{}


Suggested change

var _ OpAMPClient = &wsClient{}

var _ OpAMPClient = (*wsClient)(nil)

andykellr

LGTM

tigrannajaryan · 2025-03-25T14:48:39Z

The spec does not say whether this is a support operation (to change capabilities after they are first reported). Let me think about it.

tigrannajaryan

Blocking temporarily, give me a bit time to think about the implications of this.

andykellr · 2025-03-25T14:52:43Z

The spec does not say whether this is a support operation (to change capabilities after they are first reported). Let me think about it.

Sounds good. I held off on merging until you had a chance to look at it.

My reading of the spec was that since it was not specifically mentioned or prohibited that it would be ok to allow it. Some servers may expect this to stay the same for the life of the connection (based on the existing go implementation and not a requirement of the spec) but since it is passed with every message I think the server should be able to adjust as needed.

tigrannajaryan · 2025-03-25T15:02:13Z

I don't think we can make this blanket change.

There are capabilities which are checked at the Start() and the corresponding invariants for the capabilities are checked at the same time.

For example, in PrepareStart() we verify that if AcceptsPackages is set then PackagesStateProvider is also provided. Later in receivedProcessor we rely on this invariant. If we break the invariant (which you can easily do via SetCapabilities) then receivedProcessor will attempt to use a nil PackagesStateProvider (which will either crash or error, I didn't look further).

At least from implementation perspective this is not a change we can make.

I am also not sure conceptually it works for every single capability, to allow changing them on the fly.

Before we make this blanket change I would like to see the analysis which explain why it is OK to change each particular capability on the fly (I am not sure that is true). Then we will need to make sure the implementation is ready for that (it is currently not). We will also need to update the spec to explain that this is an allowed mode of operation.

As an alternate, is there a particular capability that you need to change after Start()? we can look at supporting just that.

jaronoff97 · 2025-03-25T15:04:43Z

Yes, i did notice that. I think we could do instantiation on demand though based on the capabilities being set. Initially, I just need to be able to enable AcceptsRemoteConfig, and I'd be okay limiting the SetCapabilities to only allow flipping that on and off initially, adding in more allowed changes in the future?

tigrannajaryan · 2025-03-25T16:52:22Z

I think AcceptsRemoteConfig is doable. I don't see any special invariants for this capability in the codebase.

We can do this:

Modify OpAMP spec to explicitly call out that the agent may send a different set of capabilities in subsequent AgentToServer messages after the initial one. Explain that only AcceptsRemoteConfig can change.
Restrict SetCapabilities to only allow certain capabilities to be changed after Start. For now it will only allow AcceptsRemoteConfig. We will need to vet all new capabilities one by one if we want to allow changing them.

jaronoff97 · 2025-04-16T16:20:51Z

@tigrannajaryan made the updates, i have one open question on this

jaronoff97 · 2025-04-16T16:21:34Z

client/internal/clientcommon.go

+// QUESTION: DO we want this?
+// SetPackagesStateProvider allows the confgiguration of the packages state provider after start.
+// func (c *ClientCommon) SetPackagesStateProvider(packagesStateProvider types.PackagesStateProvider) error {


Right now there would be no way for a client to set the package state provider if they wanted to modify the package capabilities. Should this be exposed to allow for this?

I am not sure I understand. Why would the client need to set the state provider after the Start? It can be set before Start, but can return a different set of packages anytime.

tigrannajaryan · 2025-04-21T17:56:50Z

client/client.go

+	//
+	// 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


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?

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

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

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.

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

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

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.

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

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

This approach is fine with me.

We decided to go ahead with this feature.

jaronoff97 · 2025-05-26T15:24:23Z

client/clientimpl_test.go

@@ -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 {


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

jaronoff97 · 2025-05-26T15:25:24Z

client/internal/clientcommon.go

-	if err := c.ClientSyncedState.SetCapabilities(&capabilities); err != nil {
-		return err
+	// Deprecated: Use client.SetCapabilities() instead.
+	if settings.Capabilities != 0 {


@tigrannajaryan this technically could be a breaking change if someone isn't setting any capabilities, should I just default to setting reports status here and then in the future we remove this?

I think we should emit a warning in the log if SetCapabilities is not called before Start. Later, we can make it an error. Then finally some time after that we remove Settings.Capabilities field.

In any of these 3 states, ReportsStatus should always be automatically added, regardless of how the Capabilities are set (via Settings or via method call).

@tigrannajaryan i do make sure reportsstatus is always set (I do the add in this method as well in the clientcommon setcapabilities), but I'll add in a log right now.

@tigrannajaryan should be all good now. PTAL 🙇

Make it possible to set capabilities

adcb1be

jaronoff97 requested a review from a team as a code owner March 24, 2025 20:21

formatting

cf4a028

jaronoff97 mentioned this pull request Mar 24, 2025

Bridge HA mode open-telemetry/opentelemetry-operator#3847

Draft

andykellr reviewed Mar 24, 2025

View reviewed changes

andykellr approved these changes Mar 24, 2025

View reviewed changes

tigrannajaryan previously requested changes Mar 25, 2025

View reviewed changes

jaronoff97 mentioned this pull request Mar 25, 2025

Add spec change for updating capabilities after initial message open-telemetry/opamp-spec#217

Merged

jaronoff97 added 2 commits March 26, 2025 22:39

update prototype

21667d9

update from new spec changes

3f61b9c

jaronoff97 commented Apr 16, 2025

View reviewed changes

jaronoff97 requested review from andykellr and tigrannajaryan April 16, 2025 18:06

tigrannajaryan reviewed Apr 21, 2025

View reviewed changes

jaronoff97 requested a review from tigrannajaryan May 2, 2025 14:38

Update to use setcapabilities and deprecate the old method

4b81c39

jaronoff97 commented May 26, 2025

View reviewed changes

andykellr approved these changes Jun 2, 2025

View reviewed changes

update from feedback

a130307

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

	var _ OpAMPClient = &wsClient{}
	var _ OpAMPClient = (*wsClient)(nil)

Make it possible to set capabilities #366

Are you sure you want to change the base?

Make it possible to set capabilities #366

Uh oh!

Conversation

jaronoff97 commented Mar 24, 2025

Uh oh!

jaronoff97 commented Mar 24, 2025

Uh oh!

codecov bot commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andykellr left a comment

Choose a reason for hiding this comment

Uh oh!

tigrannajaryan commented Mar 25, 2025

Uh oh!

tigrannajaryan left a comment

Choose a reason for hiding this comment

Uh oh!

andykellr commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tigrannajaryan commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaronoff97 commented Mar 25, 2025

Uh oh!

tigrannajaryan commented Mar 25, 2025

Uh oh!

jaronoff97 commented Apr 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 24, 2025 •

edited

Loading

andykellr commented Mar 25, 2025 •

edited

Loading

tigrannajaryan commented Mar 25, 2025 •

edited

Loading