Simplify OpAmp client api

[laurit]

Simplify OpAmp client api by allowing user to specify an endpoint. We'll choose whether to use http or websocket based on the endpoint scheme. This way users don't need to create the RequestService and OkHttpSender or OkHttpWebSocket themselves. This PR also moves RequestService back to internal package as most users won't be needing it. For those who need to provide a custom RequestService I have added an experimental api that lets users set a factory Function<URI, RequestService> for creating RequestService instances.

[LikeTheSalad]

This PR also moves RequestService back to internal package as most users won't be needing it.

I'm not too worried about the internal package, as this lib is not stable anyway, though I currently need to pass a custom service with a customized okhttp client instance to intercept all the requests, so having to rely on statics for this factory and within an experimental scope seems a bit unnecessarily inconvenient to me, given the current status of this lib.

What if this setter is moved into the builder and it's called from within the setEndpoint(URI) function instead? I agree there's value in having convenient functions such as setEndpoint(String/URI) so I was thinking if it could be done in a way that presents different options in the same place (the builder) and people would choose the one that better fits their needs by looking at the options from an ide autocomplete list.

[laurit]

This is the pattern we are using in the instrumentation repository to introduce experimental functionality. Vast majority of users shouldn't really need the RequestService abstraction. For your use case it doesn't really change much. You'd just change

requestService = HttpRequestService.create(OkHttpSender.create(connectivity.getUrl(), okhttpClient))
builder.setRequestService(requestService)

to

Experimental.setServiceFactory(builder, endpoint -> HttpRequestService.create(OkHttpSender.create(endpoint, okhttpClient)));

I'd say it isn't too bad usability wise. Explicitly having to go through a class named Experimental shouldn't be a problem for those who are fine with using classes from a package named internal.

[LikeTheSalad]

Got it, thanks for the example. I agree it's not too bad, I just have the following questions:

• Why bothering with an experimental/hidden API when the whole lib is essentially experimental? Hiding RequestService away might just discourage people from using it, and I guess ideally people should use the apis to see if there're no issues with them. Though I guess there's also the counter argument that, if it's hidden, then we'll know if it's actually needed whenever people ask for it to be included in the "main api", is that the reason for hiding it?
• I'm not sure about the vast majority not needing it. Without it, the only configuration for the OpAMP server connection they can provide is the URL, but what if they need other often needed features, such as payload compression, tls config, custom periodic request delay for when http is used?

[laurit]

Why bothering with an experimental/hidden API when the whole lib is essentially experimental?

because eventually we'd like it to be not experimental, got to start from somewhere

I'm not sure about the vast majority not needing it. Without it, the only configuration for the OpAMP server connection they can provide is the URL, but what if they need other often needed features, such as payload compression, tls config, custom periodic request delay for when http is used?

we could expose these in the OpampClientBuilder and change the factory to BiFunction<URI, Options, RequestService>

[LikeTheSalad]

because eventually we'd like it to be not experimental, got to start from somewhere

This is the confusing part for me, I think we both want the same, but from my point of view, hiding the API away will actually slow this process down.

we could expose these in the OpampClientBuilder and change the factory to BiFunction<URI, Options, RequestService

Sounds good. Still, I'd suggest adding the factory method right away. If anything, having to provide a preconfigured object to do the network connectivity shouldn't be weird for OTel users, as that's essentially what we do with the [Signal]Exporters already. Also, the Options object would probably have to be an interface too, as there's not a full overlap across the options we can pass when using HTTP vs WebSocket.

[breedx-splk]

It seems a little unconventional in Java to use a function for this kind of thing. I'd prefer that we just stick with using a method...which I think can even be static.

[laurit]

Currently we allow users replace this factory function, so they could customize things. Ideally eventually this will go away when we can provide users an option to customize the okhttp client without having to mess with the RequestService or HttpSender implementations.

[LikeTheSalad]

I gave a little more thought at it and I keep finding potential issues with this proposal. At first it seems quite nice for the simplicity that setEndpoint(String/URI) seems to provide over HttpRequestService.create(OkHttpSender.create(endpoint)), and it does for testing purposes, but the same simplicity can cause issues later in real world use cases whenever someone wants to customize things such as tls cert config, compression, polling times, even dynamic config in case the server connection changes on the fly, which I understand the opamp spec allows. The RequestService abstraction should enable those use cases. I thought about the following paths to move forward, but they all have cons:

• Moving the factory setter to the builder. This was my initial thought, but then I noticed that it can be confusing, as people will see the setEndpoint(String) and probably use it to set the endpoint and consider it done, and then will set a factory to configure the rest, but it turns out that within their factory they still have to ensure to use the same URL they provided in the other setter to build the instance of the RequestService which kinda defeats the purpose of having that other setter in the first place.
• Then I thought about making setEndpoint(URI) call the existing setRequestService(RequestService) setter, as a handy/quick setup for tests, but then people would have to be aware that both setters can override each other's actions, which most likely can cause annoying issues.
• Then I thought about going with the BiFunction approach mentioned earlier to provide an Options object apart from the URL, while removing the RequestService setter. One downside is that we can't provide a single Options that would cover both the HTTP and WebSocket protocols, unless we wanted to mix a bunch of optional params that would be used depending on the case, which sounds ugly to me, so I'd rather go with Options as an abstraction that only covers the common config options, and add specific ones per protocol's impl, but then it would defeat the purpose of having a handy setter that checks the url schema to know what to do, because the options would be protocol-specific, possibly even causing issues in case people provide an impl that doesn't match the url's protocol. On top of that, RequestService implementation would be limited to the Options that might be considered global across all implementations, which will make them less flexible in case a client such as okhttp for example provides some specific functionalities that other doesn't, though still people wouldn't be able of taking advantage of those because the options would be limited to the common config capabilities across all http clients. Even worse is that we would then have to expand the public API surface to keep on adding config options to cover the protocol client impl's capabilities, similarly to what they did in the go impl, which would create a long set of options that will need to be maintained, which is something they don't like having to deal with, and neither would I, tbh.

I'm also finding it difficult to see why asking users to create a RequestService instance would be problematic, as it's largely the same we do with the upstream's [Signal]Exporters needed to build an OTel instance that I mentioned earlier. I really like what's done with those [Signal]Exporter interfaces because of the flexibility they provide.

[laurit]

One of the guiding principles in the core and instrumentation repository is that there is a conscious effort to minimize the public api surface that the user can interact with. If a class is not supposed to be part of the public api it is made package private or moved to an internal package. If a class is not supposed to be extended it is made final.

but the same simplicity can cause issues later in real world use cases whenever someone wants to customize things such as tls cert config, compression, polling times, even dynamic config in case the server connection changes on the fly, which I understand the opamp spec allows. The RequestService abstraction should enable those use cases.

My understanding is that RequestService is used to abstract the communication protocol http vs websocket. The actual http client/websocket implementation is hidden behind HttpSender and WebSocket these are really where you'd configure tls or compression.

Moving the factory setter to the builder. This was my initial thought, but then I noticed that it can be confusing, as people will see the setEndpoint(String) and probably use it to set the endpoint and consider it done, and then will set a factory to configure the rest, but it turns out that within their factory they still have to ensure to use the same URL they provided in the other setter to build the instance of the RequestService which kinda defeats the purpose of having that other setter in the first place.

To me RequestService looks like purely an internal api that the users shouldn't be concerned with. Having an api like setRequestService is not good because it exposes these internal details to the user. Furthermore it requires users to directly interact with HttpRequestService, WebSocketRequestService, OkHttpSender and OkHttpWebSocket that also shouldn't be part of the public api.
The question you should ask is what do the users really need to customize? RequestService implementations have the periodic delays that could be customized, but these could easily be exposed as options that could be set on the opamp builder so users wouldn't need to be aware of the RequestService. I remember you mentioning that it should be possible to replace okhttp with a different http client being the motivation behind HttpSender interface. We expect the vast majority of users to be fine with okhttp so replacing HttpSender isn't too important either. That leaves us with configuring the underlying http client. Which means that the users don't really want an api like setRequestService but rather something that would let them supply an OkHttpClient or configure an OkHttpClient.Builder. Configuring the okhttp client doesn't need the endpoint at all so having setEndpoint wouldn't affect it.

One downside is that we can't provide a single Options that would cover both the HTTP and WebSocket protocols, unless we wanted to mix a bunch of optional params that would be used depending on the case, which sounds ugly to me, so I'd rather go with Options as an abstraction that only covers the common config options, and add specific ones per protocol's impl, but then it would defeat the purpose of having a handy setter that checks the url schema to know what to do, because the options would be protocol-specific, possibly even causing issues in case people provide an impl that doesn't match the url's protocol. On top of that, RequestService implementation would be limited to the Options that might be considered global across all implementations, which will make them less flexible in case a client such as okhttp for example provides some specific functionalities that other doesn't, though still people wouldn't be able of taking advantage of those because the options would be limited to the common config capabilities across all http clients. Even worse is that we would then have to expand the public API surface to keep on adding config options to cover the protocol client impl's capabilities, similarly to what they did in the go impl, which would create a long set of options that will need to be maintained, which is something they #1481 (comment), and neither would I, tbh.

I think it could be solved in multiple ways. If you want users building a websocket client only have access to websocket options then this would require subclassing or parameterizing the builder. Instead of just using OpampClient.builder() you'd use OpampClient.websocketBuilder() and that could expose only the websocket options. Similarly you could probably also introduce something like OkHttpOpampClient.websocketBuilder() to expose okhttp specific options. Alternatively if we believe that being able to set a websocket option while really using http is not a big deal we could go with something like <T> void option(Option<T> option, T value) and have separate classes like WebsocketOption or OkHttpOptions that list keys specific to them. In OkHttpOptions you could even have an option that lets user provide a Consumer<OkHttpClient.Builder> that configures the client. That way users could configure the http client without us providing any http client specific options. The api wouldn't be directly connected to okhttp and alternative http clients could use a similar approach. If we are willing to give up switching the http client could also let user supply okhttp client directly to the builder. Instead of OpampClient could also introduce OkHttpOpampClient where users could supply the okhttp client. If one wishes to use a different http client besides implementing the http sender and websocket interfaces they'd also need to extend some abstract opamp client base class that would let them wire it together and allow user to pass in whatever is needed. Or you could introduce something like

interface ConnectionFactory {
  HttpSender httpSender();
  WebSocket websocket();
}
...
void setConnectionFactory(new OkHttpConnectionFactory(client));

I hope that now you see that setRequestService api isn't really desirable and that there are alternatives. The aim of this PR is not to replace setRequestService but rather provide a convenient way to set the endpoint without having to use setRequestService.

[LikeTheSalad]

My understanding is that RequestService is used to abstract the communication protocol http vs websocket. The actual http client/websocket implementation is hidden behind HttpSender and WebSocket these are really where you'd configure tls or compression.

This is a very fair point.

The question you should ask is what do the users really need to customize?

I agree this is a good starting point. I'm not sure of all the things users would need to customize, but I am certain that it'll be more than just the URL, because it's already the case for Elastic's use cases, the one I mentioned for Android as well as other for Elastic's Java agent where they need to provide a custom http periodic polling mechanism, and I think we could also check out what's needed in the Go client in terms of configs, as it's been around longer than this one. So in other words, I'm certain that right now a simple setEndpoint(String/URL) is not enough and can even be more problematic to add it because of the reasons I mentioned in my previous comment. I'm also certain that the same config options won't universally apply to both protocols.

One of the guiding principles in the core and instrumentation repository is that there is a conscious effort to minimize the public api surface that the user can interact with.

I agree with this, which is why I'm strongly against going with an Option<T> API, or even a whole client implementation that's tied to a specific tool, such as OkHttpOpampClient. My understanding is that, without a network-related abstraction, they had to create different client impls per protocol in Go, which causes a lot of code duplication and possible feature-pairing maintainance work. The ConnectionFactory approach sounds good to keep the builder fairly untouched, though not only would it mean extending the public API surface, but each implementation would also have to provide both protocols, and also the factory methods would most likely have to get some sort of Options-like params added later as needed, or the ConnectionFactory impl would need a lot more params to get built to pass them onto the factories. Expanding even further the public API surface.

Based on that, I agree that it's not necessary to expose RequestService, though replacing it with an endpoint setter won't properly cover real world use cases and it can also enforce us to further expand the public API surface in the future without need. I think a nice option to remove RequestService from the public APIs would be the one you mentioned about having separate builders, i.e. OpampClient.websocketBuilder() and OpampClient.httpBuilder(), and adding setters for WebSocket and HttpSender impls respectively, as well as other protocol-specific ones, such as the periodic polling for http. I also don't think we should add any endpoint setter to either builder, based on the issues I mentioned in my previous comment of having overlapping setters. If we agree on it, I also think we should apply the changes right away into the public API and avoid creating hidden APIs mechanisms, which I think will slow down making this lib stable, as people would choose not to use those, so we wouldn't get prompt feedback of what could be improved, and in the end we'll still have to choose a final stable API, so we'll just be delaying breaking changes for later, when there's probably going to be a more extended user base using those apis.