Opamp client api #1481
Opamp client api #1481
Conversation
In opamp-go I use a git submodule to bring the protos instead of manually copying. |
Thanks. Nice to have consistency in the APIs in different languages. |
|
BTW, opamp-go is still unstable, so if you notice something you don't like in the API please let me know, we can look into fixing it. |
| * @param callback The Callback to which the Client will notify about any Server requests and | ||
| * responses. | ||
| */ | ||
| void start(Callback callback); |
There was a problem hiding this comment.
Do you plan to add other start-related settings later? Things like server url, agent instance id, etc that we have in opamp-go.
There was a problem hiding this comment.
I'm happy to discuss this more in detail, though for now, that's not the plan. It might change once we get to the part where more init params are needed though, I haven't fully analyzed all of the ones in the Go struct, but for now I can mention that the builder pattern currently takes care of things such as the instance id for example (following the note here on why we're using the builder pattern).
However, regarding the server url (or anything related to network connectivity) things get a little complicated in Java. Long story short, there are multiple tools to make HTTP requests and establish websocket connections and people have strong opinions about which ones to use, so it's a common practice to abstract those processes and create implementations that make use of one of those tools. So because of that, for anything related to network connectivity (HTTP or WebSocket) we have this abstraction that works as a placeholder and then each impl can decide how to get the values it needs (such as the server url). So for example, this is the HTTP impl that contains an instance of HttpSender where we use this sender impl that receives the HTTP url when getting created. Later, when building the OpAMP client, we pass the HttpSender impl into the builder, already configured with all the params it needs to work, such as the server url. So for now the idea is to pass an implementation that already knows where and how to send messages (and receive them too in the case of the websocket impl).
There was a problem hiding this comment.
I am very interested in your approach that abstracts away the connectivity responsibility. We felt a somewhat similar need in opamp-go. The reason there was that Go's HTTP client has a gazillion options and it is a slippery slope to try to add all of them to OpAMPClient API. Instead we want to find a good way to allow passing the http client to OpAMPClient. The difficulty we have is that HTTP Client and WebSocket client have very different APIs in Go, which results in different looking OpAMPClient API's depending on what OpAMP transport you use. This clashes with our desire to abstract away the transport differences at OpAMP level as much as possible.
There was a problem hiding this comment.
I see, I'm not sure if what we have might work for Go too, though I'm happy to help in case you'd like to get more details about it.
| * | ||
| * @param effectiveConfig The new effective config. | ||
| */ | ||
| void setEffectiveConfig(Opamp.EffectiveConfig effectiveConfig); |
There was a problem hiding this comment.
In opamp-go effective config is provided via a callback. I know it is not consistent with the rest, but there is a reason we went that way, see comments here https://github.com/open-telemetry/opamp-go/blob/b33ab76c401d0f5c3f179c5905409c1502176f97/client/internal/clientstate.go#L26
Happy to expand more if the comments are not clear.
There was a problem hiding this comment.
I must admit I thought it was strange at first because it wasn't consistent and I also missed that comment, so thank you for pointing it out 🙏
Now that I'm aware of this, I'm curious if there could be a way to address this issue while keeping some consistency with the other values at the same time 🤔 I'll have a look, and if I can't find a way I'll add the callback option 👍
There was a problem hiding this comment.
Excellent. I am happy to hear any thoughts you have about how we can make this better. I will gladly take it back to opamp-go.
There was a problem hiding this comment.
I believe I found a way to address this, I also like the overall impl better thanks to the changes I made for it to work so I'm glad we had this discussion.
The approach combines a Supplier and the observer pattern to create a State type which will be used for all the AgentToServer fields.
For most of the fields, the State implementation they will use is InMemoryState, which will store its value in memory, but for the effective config state, users will have to provide their own implementation when building the client.
When it's built, the client observes changes from these states to know when to add them into the next request. That way users can tell from within their own state implementation when should the client query it and send the effective config field.
I created this dummy implementation which is used in this example to show how it could be used.
The consequence of this is that the OpampClient interface won't have any setter/updater method related to the effective config because that state isn't handled by the client, so the user should keep their provided implementation somewhere handy for them to interact with it and update it/request for it to be sent in the next request. For the values that we want to store in memory though, the client keeps the setter methods as a convenient way of setting new values into the InMemoryState implementations, like this one for example. I'm not sure if we should keep these convenience methods though because probably some users will get confused about why not all fields have setters in the client, but the alternative is to make users to provide all the state implementations and keep them all somewhere handy so that they can change them at some point, so I guess the convenience methods can be very handy to make things easier in general.
I'm open to discussing more details about it if needed and also looking forward to any feedback! Cheers.
| import io.opentelemetry.opamp.client.internal.response.MessageData; | ||
| import opamp.proto.Opamp; | ||
|
|
||
| public interface OpampClient { |
There was a problem hiding this comment.
I don't see some of opamp-go Client's methods here, e.g. SetAgentDescription. Just wanted to confirm this is deliberate and not accidentally missing.
There was a problem hiding this comment.
Yes, for context, I'm currently moving the PoC implementation created here which had only the bare minimum apis/functionality for our use-case to work. So we should keep on adding more methods in the future. The SetAgentDescription one seems to be pretty straightforward so I think we could add it right away to account for the use-case of changing the description after the client has started.
An interesting note on the client initialization method used in the PoC is that it relies on the builder pattern. So, setting the agent description before starting the client is done in the client builder, as you can see here. There are 2 main reasons for this, the first one being that it's less error-prone as users wouldn't need to remember setting the agent description before starting the client, which is something I noticed is mentioned here in the Go impl, and the second reason is that using the builder pattern to solve these issues is a common practice in Java and makes it easier to adjust/extend initialization-related processes in the future. It's also the pattern used when initializing the OTel Java SDK so I think that's a bonus in terms of consistency across Java-related projects in OTel.
There was a problem hiding this comment.
I've just added SetAgentDescription.
There was a problem hiding this comment.
Makes sense. I think you are free to deviate from opamp-go and use whatever is the most idiomatic approach for Java. Builder pattern is less common in Go world. Struct of options is typically what Go uses, e.g. the standard library http server: https://pkg.go.dev/net/http#hdr-Servers
| */ | ||
| void setEffectiveConfig(Opamp.EffectiveConfig effectiveConfig); | ||
|
|
||
| interface Callback { |
There was a problem hiding this comment.
Nit: Callback or Callbacks? We use Callbacks in go.
There was a problem hiding this comment.
Good point, I'll make the changes 👍
| */ | ||
| void onMessage(OpampClient client, MessageData messageData); | ||
| } | ||
| } |
There was a problem hiding this comment.
I assume more callbacks will be added later, e.g. OnOpampConnectionSettings, OnCommand, etc.
There was a problem hiding this comment.
Correct, I think those should be added as soon as we add support for those functionalities (unless we see a reason to add them beforehand with noop functionality).
Sounds good! I just added it as a submodule and it works on my machine but I noticed that it's failing in the GH action, maybe we need to change it so that it fetches the submodules too. |
You can fetch on your machine and commit the submodule to the gitrepo. This way opamp-spec becomes a normal subdirectory. The submodule/subdirectory can be updated manually (in a PR) anytime you want to bump the version number of the spec that opamp-java implements. This way opamp-java implementation pins opamp-spec to the exact commit. See how it is done opamp-go: |
I fetched it via |
Creation of the OpampClient main API. It's inspired by the existing OpAMP Go client one with the lifecycle methods as well as some setters and a callback, which is also inspired by the callback in the same Go client.
I've also added the 2
protofiles from here to be able to generate the proto bindings in this same project as a temporary solution. I'm open to suggestions for better (yet simple) alternatives than having to copy the files over here.