Opamp client api #1835
Opamp client api #1835
Conversation
|
I may have said this in the past, but to reiterate: my preference is to make OpAMP Java a separate library. opamp-go is separate and, we don't plan to move it into Go contrib repo. It is sufficiently distinct that keeping it separate makes sense. It is fine if you want to do the main development in this repo but for consistency it would be nice if before we make a stable release it is moved to opamp-java. |
opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/OpampClient.java
Show resolved
Hide resolved
hey @tigrannajaryan, each of the components in this repository are published as separate libraries, can you explain a bit more what you mean / what your concerns are with publishing it from this repository? thanks! |
It is good that it is published as a separate library. However, it is still inconsistent as a practice to put it in the contrib repo, while the Go version uses a separate repo. |
It's been a while since we started discussing this implementation, though iirc, one of the things we discussed was that this repo isn't going to be the "final one" for the Java OpAMP Client, though please correct me if I'm wrong, @trask. It's a great place to do experiments with it, especially related to what's going to be its final API, while we can benefit from the available contrib infra that allows us to deploy it as an independent artifact without having to go through the hassle it means to configure a new Java repo to do so. My plan for this project is to have an independent, yet "internal" client implementation that allows us to do dogfooding in a "safe to break" environment. The reason why I think this is needed, at least within the OTel Java community, is that it seems like there are too many doubts around "what the APIs should look like" and "who's going to maintain it" given that the Java community seems to be short of bandwidth to commit to new projects. So to me it seems like this is a tradeoff that allows us to move forward in the meantime, because otherwise it seems like the alternative is doing nothing at all until an "ideal set of requirements" is met, and I think that'd be wrong because, given that there doesn't seem to be a defined set of rules for what an OpAMP SDK API should look like, then that means that we need to figure it out iteratively, for which we'll need feedback from the community. But for that to happen, we should at least have a "wrong implementation" available to criticise and work upon, otherwise it seems like the Java community will stay paralyzed on this front. |
|
I don't mind if the initial experimental implementation is done here. That's totally fine. Once you are certain the implementation is what you need I would prefer that it is moved to a separate opamp-java repo, before making a stable release. |
opamp-client/build.gradle.kts
Outdated
| @@ -9,3 +15,66 @@ java { | |||
| sourceCompatibility = JavaVersion.VERSION_1_8 | |||
| targetCompatibility = JavaVersion.VERSION_1_8 | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
you shouldn't need to set the source/target version since otel.java-conventions already defaults to 8
There was a problem hiding this comment.
Thanks, I wasn't aware of it. I just removed them.
| * After this call returns successfully it is guaranteed that no callbacks will be called. Once | ||
| * stopped, the client cannot be started again. | ||
| */ | ||
| void stop(); |
There was a problem hiding this comment.
did you consider implementing AutoCloseable?
There was a problem hiding this comment.
I haven't so far. The namings I used are based on the Go client ones, although we can change them to something that works better for Java if needed. I must admit that naming is hard for me because I tend to overthink it, so for example if we go with close here, then I would start thinking about changing the start one to open, so that the lifecycle names are intuitive.
Before making the changes, I wanted to check with you if there's a use case you have in mind that benefits from using AutoCloseable, apart from try-with-resources? Since the client most likely will be a long-running operation, it's not clear to me that a try-with-resources use case will be a common usage for it.
There was a problem hiding this comment.
Naming is hard for everybody, I guess we can revisit this later. While it is true that perhaps using try-with-resources with the opamp client is not going to be a common use case to me the Closeable (should have used this instead of AutoCloseable) just feels like a natural way to communicate that this object has a lifecycle and it can be stopped. For example the OpenTelemetrySdk also implements Closeable. As for renaming start I'd guess there is a chance that you eventually won't need that method at all. Since OpampClient is an interface one way you could approach it is that you have a builder that produces an instance of OpampClient that is already started.
When we are already talking about names then I think that Listener or SomethingSomethingListener could be a better name for Callbacks since classes like that are often called listeners in java. Again this is just an idea. I think it fine to merge this PR as it is now.
|
@LikeTheSalad thanks for working on this. Please keep tagging me on the OpAMP PRs, I am interested in the progress. |
Retry of #1481
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.