Skip to content
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.

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

Add support for CustomCapabilities and CustomMessage #209

Merged
merged 9 commits into from
Mar 14, 2024

Conversation

andykellr
Copy link
Contributor

@andykellr andykellr commented Oct 25, 2023

Implements the recent additions to the spec described in open-telemetry/opamp-spec#132

Client

CustomCapabilities are added to types.StartSettings and these are sent to the server with ReportFullState or when SetCustomCapabilities is called which will update them. There is currently no separate flag for the server to request them.

SetCustomMessage is added to the OpAMPClient interface and will add the CustomMessage to the next message and schedule a send. It reports ErrCustomCapabilityNotSupported if the Capability in the CustomMessage is not supported. This is to ensure that the client sets CustomCapabilities appropriately.

Server

The server implementation is more basic. CustomCapabilities are added to server.Settings and are sent on the first ServerToAgent message (websocket) or on every response (http). I considered adding a flag to allow the client to request them and to avoid returning them on every http response, but I decided to wait for feedback.

CustomMessage is available on the ServerToAgent message but it is up to the server implementation to set it on the message before returning from OnMessage or when calling Send.

Usage

I am using this implementation on a branch of BindPlane OP and bindplane-agent and it is working well.

@andykellr andykellr requested a review from a team October 25, 2023 03:20
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Attention: Patch coverage is 97.46835% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 74.95%. Comparing base (ce8a8dd) to head (3ba787b).
Report is 1 commits behind head on main.

Files Patch % Lines
client/internal/receivedprocessor.go 75.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #209      +/-   ##
==========================================
+ Coverage   73.45%   74.95%   +1.49%     
==========================================
  Files          25       25              
  Lines        1537     1625      +88     
==========================================
+ Hits         1129     1218      +89     
+ Misses        298      296       -2     
- Partials      110      111       +1     

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

client/client.go Outdated Show resolved Hide resolved
@andykellr
Copy link
Contributor Author

I updated the Client SetCustomMessage to return an ErrCustomMessagePending error and a channel that can be used to wait for the message to be sent if there is already a custom message on the AgentToServer message.

	// SetCustomMessage sets the custom message that will be sent to the Server. May be
	// called anytime after Start(), including from OnMessage handler.
	//
	// If the CustomMessage is nil, ErrCustomMessageMissing will be returned. If the message
	// specifies a capability that is not listed in the CustomCapabilities provided in the
	// StartSettings for the client, ErrCustomCapabilityNotSupported will be returned.
	//
	// Only one message can be set at a time. If a message has already been set, it will
	// return ErrCustomMessagePending. To ensure that it is safe to set another
	// CustomMessage, the caller should wait for the channel to be closed before attempting
	// to set another custom message.
	SetCustomMessage(message *protobufs.CustomMessage) (messageSendingChannel chan struct{}, err error)

client/client.go Outdated Show resolved Hide resolved
client/internal/nextmessage.go Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/types/startsettings.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

SetCustomMessage and the use of channel looks good to me.

client/client.go Outdated Show resolved Hide resolved
client/internal/clientcommon.go Show resolved Hide resolved
server/server.go Show resolved Hide resolved
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, using the channel returned from SendCustomMessage looks like a straightfoward way to manage concurrency based on the usage in the tests.

client/clientimpl_test.go Outdated Show resolved Hide resolved
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, only some minor questions/comments remaining.

client/client.go Outdated Show resolved Hide resolved
client/client.go Show resolved Hide resolved
client/clientimpl_test.go Outdated Show resolved Hide resolved
client/clientimpl_test.go Show resolved Hide resolved
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

@tigrannajaryan tigrannajaryan merged commit ba70a24 into open-telemetry:main Mar 14, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants