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 CustomMessage capability #132

Merged
merged 21 commits into from
Oct 20, 2023

Conversation

andykellr
Copy link
Contributor

See #49

If we agree on the direction of this, I plan to do the following:

  • updates to this opamp-spec PR:
    • add protos to the opamp.proto file
  • updates to the opamp-go repo in a separate PR:
    • add OnCustomMessage method to the client Callbacks interfaces
    • add SendCustomMessage to the OpAMPClient
    • add implementation to the client and server
    • add tests for that implementation

@andykellr
Copy link
Contributor Author

@tigrannajaryan please review and I will update and fill in the rest. Thanks!

@andykellr
Copy link
Contributor Author

Related to this, should we enforce line-length in .markdownlint.yaml? My editor wraps at 120. I noticed most of the existing lines are between 80-90.

specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

Related to this, should we enforce line-length in .markdownlint.yaml? My editor wraps at 120. I noticed most of the existing lines are between 80-90.

Yes, please keep the line wraps similar to the rest of the spec (I think it is at 90).

@andykellr
Copy link
Contributor Author

I made significant changes to this proposal and have some time to work on the implementation. @tigrannajaryan please have a look.

Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

Had one question, overall I'm a fan of this change. It would be great for the future to support custom messages to allow the protocol to be extended upon more easily. For the operator group, I could imagine the bridge allowing for a message for restarting a workload or even pausing telemetry as in the examples.

specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
@andykellr
Copy link
Contributor Author

Related to this, should we enforce line-length in .markdownlint.yaml? My editor wraps at 120. I noticed most of the existing lines are between 80-90.

Yes, please keep the line wraps similar to the rest of the spec (I think it is at 90).

I attempted to enforce this with the following change, but it requires many sections to be rewrapped to meet the requirement. I can do this in a separate PR if we believe it should be enforced.

<!-- markdownlint-configure-file
{
  "line-length": {
    "line_length": 90,
    "stern": true
  }
}
-->

specification.md Outdated Show resolved Hide resolved
specification.md Show resolved Hide resolved

#### CustomMessageCapabilities Message

The Agent and Server should both use this message to signal that they support specific

Choose a reason for hiding this comment

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

Just a thought. Although probably unlikely, a string capability for one server may differ from the functionality on another (e.g. vendor) and a connecting agent may not know any better resulting in unexpected behaviour. Do we care about interoperability? Do the strings need to be distro/vendor specific? Do we need another field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent is that they would use a reverse FQDN to be vendor specific, e.g. our pause/resume feature might be com.observiq.pause. I used io.opentelemetry in the examples to keep vendor specific names out of the spec.

andykellr and others added 3 commits October 3, 2023 21:52
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@andykellr andykellr marked this pull request as ready for review October 4, 2023 21:07
@andykellr andykellr requested review from a team, portertech and evan-bradley October 4, 2023 21:07
@andykellr
Copy link
Contributor Author

I just updated the PR with CustomCapabilities in addition to CustomMessage as discussed above.

proto/opamp.proto Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
Copy link

@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 this looks good to me, I only have minor comments on wording.

specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
andykellr and others added 3 commits October 5, 2023 15:12
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
proto/opamp.proto Show resolved Hide resolved
proto/opamp.proto Show resolved Hide resolved
proto/opamp.proto Show resolved Hide resolved
specification.md Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
andykellr and others added 2 commits October 16, 2023 19:28
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
specification.md Outdated Show resolved Hide resolved
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
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.

Just 2 small nits and we can merge after that.

specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
andykellr and others added 3 commits October 18, 2023 13:44
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
@tigrannajaryan tigrannajaryan merged commit cb1cf6d into open-telemetry:main Oct 20, 2023
5 checks passed
@andykellr andykellr deleted the custom-messages branch October 20, 2023 15:17
tigrannajaryan pushed a commit to open-telemetry/opamp-go that referenced this pull request Mar 14, 2024
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.
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.

8 participants