Add opamp command processor#2884
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for processing OpAMP ServerToAgentCommand messages by introducing a configurable command processor and dispatching command payloads from received server responses.
Changes:
- Adds
OpampClient.CommandProcessoras a new public callback interface. - Adds
setCommandProcessor(...)toOpampClientBuilderand wires it intoOpampClientImpl. - Adds tests covering command dispatch and no-op behavior when no command is present.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
opamp-client/src/main/java/io/opentelemetry/opamp/client/OpampClient.java |
Adds the public CommandProcessor interface. |
opamp-client/src/main/java/io/opentelemetry/opamp/client/OpampClientBuilder.java |
Adds builder storage and setter for the command processor and passes it into client creation. |
opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/impl/OpampClientImpl.java |
Stores the processor, provides a no-op default, and invokes it for received commands. |
opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/impl/OpampClientImplTest.java |
Adds tests for command callback invocation and non-invocation. |
| @CanIgnoreReturnValue | ||
| public OpampClientBuilder setCommandProcessor( | ||
| @Nullable OpampClient.CommandProcessor commandProcessor) { | ||
| this.commandProcessor = commandProcessor; |
There was a problem hiding this comment.
This is a reasonable assessment, however what I don't like is the assumption that ALL command processors will handle restartability. The spec is intentionally extensible, and even tho the protobufs only have a single value in the enum today (restart), we would expect it to expand in the future.
Coupling the command processor to this functionality (like in the suggestion) is more brittle than the separate enable method.
There was a problem hiding this comment.
Also, since the protos are v3, it should be possible (tolerable) to use other integer values for the CommandType. Unfortunately, our currently generated proto classes seem like they forbid/exclude this ability to override...and since the java instrumentation agent isn't exactly restartable, maybe this entire effort is for nought. 😢
There was a problem hiding this comment.
it should be possible (tolerable) to use other integer values for the CommandType
I'm not sure I'm following, but it sounds like the goal of these changes is to provide a generic way of receiving commands from the server?
If the above is true, then I'm a bit confused, because my understanding from the spec is that commands are fixed and defined in the spec, and only restart is available now.
Also, according to the spec, if an agent receives a restart command and can't handle it, the agent can ignore it. So, by adding support for commands and then not enabling the one currently available, it seems like we're adding effectively dead code (unless I missed something from the spec that allows for custom commands or the like).
Based on the above, and following up from this other comment, I think a way forward could be to move the onCommand function to the existing Callbacks interface, which will allow us to keep consistency with the Go client while at the same time avoiding the need to pass a new nullable object around and/or to decide whether we should couple this setter with some capabilities flag setting, and instead replace the setter by a specific command flag enabling option, like enableRestartCommand(), where only the flag is added to capabilities, which will allow us to keep on adding these enabler methods as new commands are added into the spec later on.
There was a problem hiding this comment.
it should be possible (tolerable) to use other integer values for the CommandType
I'm not sure I'm following, but it sounds like the goal of these changes is to provide a generic way of receiving commands from the server?
That's right. This is the direction that this is trying take. The protobuf enum only has one value, but proto3 allows any values in enums...even if our generated classes don't. This allows custom/bespoke commands that are not (yet) spec'd to be implemented as a demo/prototype.
If the above is true, then I'm a bit confused, because my understanding from the spec is that commands are fixed and defined in the spec, and only
restartis available now.
Going strictly by the spec yes. Looking at the protos, then no, proto3 allows arbitrary int values in enums.
if an agent receives a
restartcommand and can't handle it, the agent can ignore it.
Right, but I don't actually care about the one thing that is spec'd currently. I want to use it for other stuff.
I think a way forward could be to move the
onCommandfunction to the existingCallbacksinterface
I don't care for the Callbacks interface because I think it does too much already. I intentionally chose to create a new interface so that it wouldn't make that problem worse...but also to keep this new/unused functionality more isolated.
which will allow us to keep consistency with the Go client
Never been a priority for me. 🤷🏻
| if (response.remote_config != null) { | ||
| notifyOnMessage = true; | ||
| messageBuilder.setRemoteConfig(response.remote_config); | ||
| callbacks.onMessage(this, messageBuilder.build()); |
There was a problem hiding this comment.
I understand that, given the current state of this project where only remote config is supported, it makes sense to get rid of the boolean flag. However, the reason why it's there is that, as we add support for more serverToAgent fields, we're going to need to aggregate them into a single call to callbacks.onMessage and avoid calling it multiple times. There might be more elegant ways to achieve the same goal than using a flag, though, if you can think of an alternative that allows the aggregation, I'm up for it. If not, I'd like to at least add some tests that ensure that this method is called only once per serverToAgent message, even though a remote config isn't present in the payload (but another field for onMessage is).
There was a problem hiding this comment.
I only simplified because it was clunky and reads funny. I follow your future-proofing explanation though. I can put it back if you'd like.
| if (notifyOnMessage) { | ||
| callbacks.onMessage(this, messageBuilder.build()); | ||
| if (response.command != null) { | ||
| commandProcessor.onCommand(response.command); |
There was a problem hiding this comment.
According to the spec, this field is special in the sense that it represents a terminal operation, where any other field from the serverToAgent payload, aside from instance_uid and capabilities, must be ignored. That leaves out the possibility of calling callbacks.onMessage, so it's a bit strange to have it handled after checking for calls to callbacks.onMessage. I believe the ideal way to handle it is before starting to build a MessageData object, and return early in case a command is found.
There was a problem hiding this comment.
sure, we could move the command handling up and then return early I guess. I think it's silly that the spec both allows and disallows sending messages alongside commands, but whatever.
| } | ||
|
|
||
| @FunctionalInterface | ||
| interface CommandProcessor { |
There was a problem hiding this comment.
I like this idea in general, but I'd like to try to keep as much parity as possible with the Go client implementation, for which commands are handled within the same Callbacks interface in a function named onCommand.
I'm not opposed to doing things differently, though, but I think we should be consistent. If we feel like Go's style is causing us issues, then I think we should consider changing our approach, but in an internally consistent way, by refactoring existing callbacks too. However, right now I don't see why we should do things differently than Go, though.
There was a problem hiding this comment.
It's fine. I've never cared about trying to follow the Go client, but if you want to I can. That's fine.
| @CanIgnoreReturnValue | ||
| public OpampClientBuilder setCommandProcessor( | ||
| @Nullable OpampClient.CommandProcessor commandProcessor) { | ||
| this.commandProcessor = commandProcessor; |
There was a problem hiding this comment.
it should be possible (tolerable) to use other integer values for the CommandType
I'm not sure I'm following, but it sounds like the goal of these changes is to provide a generic way of receiving commands from the server?
If the above is true, then I'm a bit confused, because my understanding from the spec is that commands are fixed and defined in the spec, and only restart is available now.
Also, according to the spec, if an agent receives a restart command and can't handle it, the agent can ignore it. So, by adding support for commands and then not enabling the one currently available, it seems like we're adding effectively dead code (unless I missed something from the spec that allows for custom commands or the like).
Based on the above, and following up from this other comment, I think a way forward could be to move the onCommand function to the existing Callbacks interface, which will allow us to keep consistency with the Go client while at the same time avoiding the need to pass a new nullable object around and/or to decide whether we should couple this setter with some capabilities flag setting, and instead replace the setter by a specific command flag enabling option, like enableRestartCommand(), where only the flag is added to capabilities, which will allow us to keep on adding these enabler methods as new commands are added into the spec later on.
The OpAmp spec provides a way for the
ServerToAgentmessage to contain aServerToAgentCommand, but this client doesn't yet provide a way to handle these commands.This PR introduces a new inner interface called
CommandProcessorwhich can be configured to handle commands with the client.