-
Notifications
You must be signed in to change notification settings - Fork 45
Separate the notions of Agent and Client #122
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
Separate the notions of Agent and Client #122
Conversation
…ty) and Client (as an endpoint of OpAMP).
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @PeterF778
This is very useful. I am not entirely sure the renamings work fine in all places. I wonder also what @andykellr thinks.
Also, no changes in the Packages section? Do you think it should stay as is?
specification.md
Outdated
message is a | ||
[binary serialized Protobuf](https://developers.google.com/protocol-buffers/docs/encoding) | ||
message. The Agent sends AgentToServer Protobuf messages and the Server sends | ||
message. The Client sends AgentToServer Protobuf messages and the Server sends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also rename AgentToServer to ClientToServer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the protocol is still very much Agent-oriented. The Server is almost exclusively concerned about the Agent capabilities, configuration and status, and hardly sees the Client as an independent entity - even when asking for Agent upgrade, it happens magically behind the scene, even though we know that it requires some sort of a supervisor.
Thanks @tigrannajaryan for reviewing. |
Thank you for this 👍 I recently found myself stumbling over words when explaining how OpAMP works with/within the OT Agent 😅 |
@portertech do you think the changes introduced by this PR make it clearer? |
@andykellr @portertech I wonder what you guys think about this change. As the author of the current text I am probably biased towards the usage of "Agent" everywhere, so I may not be the best person to tell if this change makes the spec more clear (I think it definitely does in some places, but I am worried that overall it may result in some confusion because of using 2 different concepts). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the issues Tigran highlighted and my only other piece of feedback was a small grammatical change.
Thank you for taking the time to go through the spec and identify all of the necessary changes. I expanded all of the unchanged sections as I review the changes and I believe all of the Agent => Client changes are appropriate and I did not find any that were overlooked.
Thank you @andykellr for the review. I agree with yours and @tigrannajaryan suggestions. I'll make the corrections next week. |
@andykellr I will keep this open for you to also review. |
Merged. Thanks @PeterF778 ! |
Separating the previous notion of Agent into Agent (as a managed entity) and Client (as an endpoint of OpAMP).
We have heard from a few people that it was confusing and unclear how the Agent and the Client are related. This makes clarifications and makes the Client and the Agent more distinct in the spec.