Skip to content

Simplify OpAmp client api #2408

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Simplify OpAmp client api #2408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions opamp-client/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ client [spec](https://github.com/open-telemetry/opamp-spec/blob/main/specificati
```java
// Initializing it

RequestService requestService = HttpRequestService.create(OkHttpSender.create("[OPAMP_SERVICE_URL]"));
// RequestService requestService = WebSocketRequestService.create(OkHttpWebSocket.create("[OPAMP_SERVICE_URL]")); // Use this instead to connect to the server via WebSocket.
OpampClient client =
OpampClient.builder()
.setEndpoint("[OPAMP_SERVICE_URL]")
.putIdentifyingAttribute("service.name", "My service name")
.enableRemoteConfig()
.setRequestService(requestService)
Expand Down
54 changes: 44 additions & 10 deletions opamp-client/src/main/java/io/opentelemetry/opamp/client/OpampClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,23 @@

import com.github.f4b6a3.uuid.UuidCreator;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import io.opentelemetry.opamp.client.internal.Experimental;
import io.opentelemetry.opamp.client.internal.connectivity.http.OkHttpSender;
import io.opentelemetry.opamp.client.internal.connectivity.websocket.OkHttpWebSocket;
import io.opentelemetry.opamp.client.internal.impl.OpampClientImpl;
import io.opentelemetry.opamp.client.internal.impl.OpampClientState;
import io.opentelemetry.opamp.client.internal.request.service.HttpRequestService;
import io.opentelemetry.opamp.client.internal.request.service.RequestService;
import io.opentelemetry.opamp.client.internal.request.service.WebSocketRequestService;
import io.opentelemetry.opamp.client.internal.state.State;
import io.opentelemetry.opamp.client.request.service.RequestService;
import java.net.URI;
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.function.Function;
import javax.annotation.Nullable;
import opamp.proto.AgentCapabilities;
import opamp.proto.AgentDescription;
Expand All @@ -30,27 +34,57 @@

/** Builds an {@link OpampClient} instance. */
public final class OpampClientBuilder {
private static final String DEFAULT_ENDPOINT_URL = "http://localhost:4320/v1/opamp";
private static final URI DEFAULT_ENDPOINT = URI.create(DEFAULT_ENDPOINT_URL);

private final Map<String, AnyValue> identifyingAttributes = new HashMap<>();
private final Map<String, AnyValue> nonIdentifyingAttributes = new HashMap<>();
private long capabilities = 0;
private RequestService service =
HttpRequestService.create(OkHttpSender.create("http://localhost:4320/v1/opamp"));
private Function<URI, RequestService> serviceFactory =
Copy link
Contributor

@breedx-splk breedx-splk Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@laurit laurit Nov 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endpoint -> {
String scheme = endpoint.getScheme();
if ("ws".equalsIgnoreCase(scheme) || "wss".equalsIgnoreCase(scheme)) {
return WebSocketRequestService.create(OkHttpWebSocket.create(endpoint.toString()));
} else {
return HttpRequestService.create(OkHttpSender.create(endpoint.toString()));
}
};

@Nullable private byte[] instanceUid;
@Nullable private State.EffectiveConfig effectiveConfigState;
@Nullable private URI endpoint = DEFAULT_ENDPOINT;

static {
Experimental.internalSetServiceFactory(
(builder, serviceFactory) -> builder.serviceFactory = serviceFactory);
}

OpampClientBuilder() {}

/**
* Sets an implementation of a {@link RequestService} to handle the request's sending process.
* There are 2 possible options, either {@link HttpRequestService} to use HTTP, or {@link
* WebSocketRequestService} to use WebSocket.
* Sets the OpAMP endpoint to connect to. If unset, defaults to {@value DEFAULT_ENDPOINT_URL}. The
* endpoint must start with either http://, https://, ws:// or wss://. Sets the address of the
* OpAMP Server.
*
* @param endpoint The OpAMP endpoint.
* @return this
*/
@CanIgnoreReturnValue
public OpampClientBuilder setEndpoint(String endpoint) {
return setEndpoint(URI.create(endpoint));
}

/**
* Sets the OpAMP endpoint to connect to. If unset, defaults to {@value DEFAULT_ENDPOINT_URL}. The
* endpoint must start with either http://, https://, ws:// or wss://. Sets the address of the
* OpAMP Server.
*
* @param service The request service implementation.
* @param endpoint The OpAMP endpoint.
* @return this
*/
@CanIgnoreReturnValue
public OpampClientBuilder setRequestService(RequestService service) {
this.service = service;
public OpampClientBuilder setEndpoint(URI endpoint) {
this.endpoint = endpoint;
return this;
}

Expand Down Expand Up @@ -402,7 +436,7 @@ public OpampClient build(OpampClient.Callbacks callbacks) {
new State.InstanceUid(instanceUid),
new State.Flags(0L),
effectiveConfigState);
return OpampClientImpl.create(service, state, callbacks);
return OpampClientImpl.create(serviceFactory.apply(endpoint), state, callbacks);
}

private static State.EffectiveConfig createEffectiveConfigNoop() {
Expand Down
42 changes: 42 additions & 0 deletions opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/Experimental.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.opamp.client.internal;

import io.opentelemetry.opamp.client.OpampClientBuilder;
import io.opentelemetry.opamp.client.internal.request.service.RequestService;
import java.net.URI;
import java.util.function.BiConsumer;
import java.util.function.Function;
import javax.annotation.Nullable;

/**
* This class is internal and experimental. Its APIs are unstable and can change at any time. Its
* APIs (or a version of them) may be promoted to the public stable API in the future, but no
* guarantees are made.
*/
public final class Experimental {

@Nullable
private static volatile BiConsumer<OpampClientBuilder, Function<URI, RequestService>>
setServiceFactory;

/**
* Sets factory function for creating {@link RequestService} instances form a given server URI.
*/
public static void setServiceFactory(
Copy link
Contributor

@LikeTheSalad LikeTheSalad Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@laurit laurit Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@LikeTheSalad LikeTheSalad Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@laurit laurit Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Contributor

@LikeTheSalad LikeTheSalad Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

OpampClientBuilder builder, Function<URI, RequestService> serviceFactory) {
if (setServiceFactory != null) {
setServiceFactory.accept(builder, serviceFactory);
}
}

public static void internalSetServiceFactory(
BiConsumer<OpampClientBuilder, Function<URI, RequestService>> setServiceFactory) {
Experimental.setServiceFactory = setServiceFactory;
}

private Experimental() {}
}
2 changes: 1 addition & 1 deletion opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/impl/OpampClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
import io.opentelemetry.opamp.client.internal.impl.recipe.appenders.SequenceNumberAppender;
import io.opentelemetry.opamp.client.internal.request.Field;
import io.opentelemetry.opamp.client.internal.request.Request;
import io.opentelemetry.opamp.client.internal.request.service.RequestService;
import io.opentelemetry.opamp.client.internal.response.MessageData;
import io.opentelemetry.opamp.client.internal.response.OpampServerResponseException;
import io.opentelemetry.opamp.client.internal.response.Response;
import io.opentelemetry.opamp.client.internal.state.ObservableState;
import io.opentelemetry.opamp.client.internal.state.State;
import io.opentelemetry.opamp.client.request.service.RequestService;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand Down
1 change: 0 additions & 1 deletion .../main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import io.opentelemetry.opamp.client.internal.request.delay.RetryPeriodicDelay;
import io.opentelemetry.opamp.client.internal.response.OpampServerResponseException;
import io.opentelemetry.opamp.client.internal.response.Response;
import io.opentelemetry.opamp.client.request.service.RequestService;
import java.io.IOException;
import java.time.Duration;
import java.util.Objects;
Expand Down
4 changes: 1 addition & 3 deletions ...lient/request/service/RequestService.java → ...ernal/request/service/RequestService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,10 @@
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.opamp.client.request.service;
package io.opentelemetry.opamp.client.internal.request.service;

import io.opentelemetry.opamp.client.OpampClient;
import io.opentelemetry.opamp.client.internal.request.Request;
import io.opentelemetry.opamp.client.internal.request.service.HttpRequestService;
import io.opentelemetry.opamp.client.internal.request.service.WebSocketRequestService;
import io.opentelemetry.opamp.client.internal.response.Response;
import java.util.function.Supplier;

Expand Down
1 change: 0 additions & 1 deletion .../java/io/opentelemetry/opamp/client/internal/request/service/WebSocketRequestService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import io.opentelemetry.opamp.client.internal.request.delay.RetryPeriodicDelay;
import io.opentelemetry.opamp.client.internal.response.OpampServerResponseException;
import io.opentelemetry.opamp.client.internal.response.Response;
import io.opentelemetry.opamp.client.request.service.RequestService;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.time.Duration;
Expand Down
2 changes: 1 addition & 1 deletion ...client/src/test/java/io/opentelemetry/opamp/client/internal/impl/OpampClientImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
import io.opentelemetry.opamp.client.internal.connectivity.http.OkHttpSender;
import io.opentelemetry.opamp.client.internal.request.Request;
import io.opentelemetry.opamp.client.internal.request.service.HttpRequestService;
import io.opentelemetry.opamp.client.internal.request.service.RequestService;
import io.opentelemetry.opamp.client.internal.response.MessageData;
import io.opentelemetry.opamp.client.internal.state.State;
import io.opentelemetry.opamp.client.request.service.RequestService;
import java.io.IOException;
import java.time.Duration;
import java.util.ArrayList;
Expand Down
1 change: 0 additions & 1 deletion ...t/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import io.opentelemetry.opamp.client.internal.request.Request;
import io.opentelemetry.opamp.client.internal.request.delay.PeriodicDelay;
import io.opentelemetry.opamp.client.internal.response.Response;
import io.opentelemetry.opamp.client.request.service.RequestService;
import java.io.ByteArrayInputStream;
import java.time.Duration;
import java.util.ArrayList;
Expand Down
1 change: 0 additions & 1 deletion ...a/io/opentelemetry/opamp/client/internal/request/service/WebSocketRequestServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import io.opentelemetry.opamp.client.internal.request.Request;
import io.opentelemetry.opamp.client.internal.response.OpampServerResponseException;
import io.opentelemetry.opamp.client.internal.response.Response;
import io.opentelemetry.opamp.client.request.service.RequestService;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.time.Duration;
Expand Down