From 0064a0b333e34f34841b025329ad1f067c471a67 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Thu, 29 May 2025 19:00:51 +0200 Subject: [PATCH 01/38] Creating Request --- .../client/internal/request/Request.java | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java new file mode 100644 index 000000000..1998187a5 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java @@ -0,0 +1,32 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package io.opentelemetry.opamp.client.internal.request; + +import com.google.auto.value.AutoValue; +import opamp.proto.AgentToServer; + +/** Wrapper class for "AgentToServer" request body. */ +@AutoValue +public abstract class Request { + public abstract AgentToServer getAgentToServer(); + + public static Request create(AgentToServer agentToServer) { + return new AutoValue_Request(agentToServer); + } +} From 84bd658338765d073e9ddb079ccd5990ab2954ea Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 30 May 2025 13:40:54 +0200 Subject: [PATCH 02/38] Adding periodic task executor --- .../client/internal/request/Request.java | 18 -- .../request/delay/FixedPeriodicDelay.java | 19 ++ .../internal/request/delay/PeriodicDelay.java | 13 ++ .../request/delay/PeriodicTaskExecutor.java | 77 ++++++++ .../delay/PeriodicTaskExecutorTest.java | 168 ++++++++++++++++++ 5 files changed, 277 insertions(+), 18 deletions(-) create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/FixedPeriodicDelay.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicDelay.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java create mode 100644 opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java index 1998187a5..56d768a25 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java @@ -1,21 +1,3 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ package io.opentelemetry.opamp.client.internal.request; import com.google.auto.value.AutoValue; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/FixedPeriodicDelay.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/FixedPeriodicDelay.java new file mode 100644 index 000000000..f0d0c1481 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/FixedPeriodicDelay.java @@ -0,0 +1,19 @@ +package io.opentelemetry.opamp.client.internal.request.delay; + +import java.time.Duration; + +final class FixedPeriodicDelay implements PeriodicDelay { + private final Duration duration; + + public FixedPeriodicDelay(Duration duration) { + this.duration = duration; + } + + @Override + public Duration getNextDelay() { + return duration; + } + + @Override + public void reset() {} +} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicDelay.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicDelay.java new file mode 100644 index 000000000..6e5526796 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicDelay.java @@ -0,0 +1,13 @@ +package io.opentelemetry.opamp.client.internal.request.delay; + +import java.time.Duration; + +public interface PeriodicDelay { + static PeriodicDelay ofFixedDuration(Duration duration) { + return new FixedPeriodicDelay(duration); + } + + Duration getNextDelay(); + + void reset(); +} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java new file mode 100644 index 000000000..410629146 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java @@ -0,0 +1,77 @@ +package io.opentelemetry.opamp.client.internal.request.delay; + +import java.util.Objects; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + +public final class PeriodicTaskExecutor { + private final ScheduledExecutorService executorService; + private final Lock delaySetLock = new ReentrantLock(); + private final AtomicReference periodicTask = new AtomicReference<>(); + private PeriodicDelay periodicDelay; + @Nullable private ScheduledFuture scheduledFuture; + + public static PeriodicTaskExecutor create(PeriodicDelay initialPeriodicDelay) { + return new PeriodicTaskExecutor( + Executors.newSingleThreadScheduledExecutor(), initialPeriodicDelay); + } + + PeriodicTaskExecutor( + ScheduledExecutorService executorService, PeriodicDelay initialPeriodicDelay) { + this.executorService = executorService; + this.periodicDelay = initialPeriodicDelay; + } + + public void start(@Nonnull Runnable periodicTask) { + this.periodicTask.set(periodicTask); + scheduleNext(); + } + + public void executeNow() { + executorService.execute(periodicTask.get()); + } + + public void setPeriodicDelay(PeriodicDelay periodicDelay) { + delaySetLock.lock(); + try { + if (scheduledFuture != null) { + scheduledFuture.cancel(false); + } + this.periodicDelay = periodicDelay; + periodicDelay.reset(); + scheduleNext(); + } finally { + delaySetLock.unlock(); + } + } + + public void stop() { + executorService.shutdown(); + } + + private void scheduleNext() { + delaySetLock.lock(); + try { + scheduledFuture = + executorService.schedule( + new PeriodicRunner(), periodicDelay.getNextDelay().toNanos(), TimeUnit.NANOSECONDS); + } finally { + delaySetLock.unlock(); + } + } + + private class PeriodicRunner implements Runnable { + @Override + public void run() { + Objects.requireNonNull(periodicTask.get()).run(); + scheduleNext(); + } + } +} diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java new file mode 100644 index 000000000..13d7a3cc9 --- /dev/null +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java @@ -0,0 +1,168 @@ +package io.opentelemetry.opamp.client.internal.request.delay; + +import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.time.Duration; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import org.junit.jupiter.api.Test; +import org.mockito.InOrder; + +@SuppressWarnings({"rawtypes", "unchecked"}) +class PeriodicTaskExecutorTest { + private static final int TIMEOUT_SECONDS = 5; + + @Test + void verifyRunAtStart() throws InterruptedException { + CountDownLatch latch = new CountDownLatch(1); + PeriodicDelay delay = new TestPeriodicDelay(Duration.ofSeconds(0)); + + PeriodicTaskExecutor.create(delay).start(latch::countDown); + + if (!latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + fail("Timed out waiting"); + } + } + + @Test + void verifyRerunAfterFinishedRunning() throws InterruptedException { + CountDownLatch latch = new CountDownLatch(2); + PeriodicDelay delay = new TestPeriodicDelay(Duration.ofMillis(500)); + + PeriodicTaskExecutor.create(delay).start(latch::countDown); + + if (!latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + fail("Timed out waiting"); + } + } + + @Test + void verifyRunningImmediatelyWhenRequested() { + PeriodicDelay delay = new TestPeriodicDelay(Duration.ofMinutes(1)); + ScheduledExecutorService executorService = mock(); + Runnable task = mock(Runnable.class); + PeriodicTaskExecutor executor = new PeriodicTaskExecutor(executorService, delay); + executor.start(task); + + executor.executeNow(); + + verify(executorService).execute(task); + } + + @Test + void verifyKeepingOriginalScheduleAfterRunningImmediately() throws InterruptedException { + CountDownLatch latch = new CountDownLatch(2); + PeriodicDelay delay = new TestPeriodicDelay(Duration.ofSeconds(1)); + + PeriodicTaskExecutor executor = PeriodicTaskExecutor.create(delay); + executor.start(latch::countDown); + + executor.executeNow(); + + if (!latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + fail("Timed out waiting"); + } + } + + @Test + void verifyDelayValueUsed() { + Duration duration = Duration.ofSeconds(1); + PeriodicDelay delay = new TestPeriodicDelay(duration); + ScheduledExecutorService executorService = mock(); + + PeriodicTaskExecutor executor = new PeriodicTaskExecutor(executorService, delay); + + executor.start(() -> {}); + + verify(executorService) + .schedule(any(Runnable.class), eq(duration.toNanos()), eq(TimeUnit.NANOSECONDS)); + } + + @Test + void verifyDelayChange() throws InterruptedException { + CountDownLatch latch = new CountDownLatch(1); + PeriodicDelay delay = new TestPeriodicDelay(Duration.ofMinutes(1)); + + PeriodicTaskExecutor executor = PeriodicTaskExecutor.create(delay); + executor.start(latch::countDown); + + executor.setPeriodicDelay(new TestPeriodicDelay(Duration.ofSeconds(1))); + + if (!latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)) { + fail("Timed out waiting"); + } + } + + @Test + void verifyPreviousTaskCancelledWhenDelayChanges() { + ScheduledExecutorService executorService = mock(); + ScheduledFuture firstTaskFuture = mock(ScheduledFuture.class); + when(executorService.schedule(any(Runnable.class), anyLong(), any())) + .thenReturn(firstTaskFuture); + + PeriodicTaskExecutor executor = + new PeriodicTaskExecutor(executorService, new TestPeriodicDelay(Duration.ofSeconds(1))); + + executor.start(() -> {}); + + executor.setPeriodicDelay(new TestPeriodicDelay(Duration.ofSeconds(2))); + + verify(firstTaskFuture).cancel(false); + } + + @Test + void verifyNewPeriodicDelayGetsResetBeforeBeingUsed() { + ScheduledExecutorService executorService = mock(); + ScheduledFuture firstTaskFuture = mock(ScheduledFuture.class); + when(executorService.schedule(any(Runnable.class), anyLong(), any())) + .thenReturn(firstTaskFuture); + PeriodicDelay delayChange = mock(); + + PeriodicTaskExecutor executor = + new PeriodicTaskExecutor(executorService, new TestPeriodicDelay(Duration.ofSeconds(1))); + + executor.start(() -> {}); + + executor.setPeriodicDelay(delayChange); + + InOrder inOrder = inOrder(delayChange); + inOrder.verify(delayChange).reset(); + inOrder.verify(delayChange).getNextDelay(); + } + + @Test + void verifyStop() { + ScheduledExecutorService executorService = mock(); + + PeriodicTaskExecutor executor = + new PeriodicTaskExecutor(executorService, new TestPeriodicDelay(Duration.ofSeconds(1))); + executor.stop(); + + verify(executorService).shutdown(); + } + + private static class TestPeriodicDelay implements PeriodicDelay { + private final Duration delay; + + private TestPeriodicDelay(Duration delay) { + this.delay = delay; + } + + @Override + public Duration getNextDelay() { + return delay; + } + + @Override + public void reset() {} + } +} From 194a0eefb358d6008d6038b4151a0f0e258133c1 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 30 May 2025 14:33:59 +0200 Subject: [PATCH 03/38] Adding RequestService and its http implementation --- opamp-client/build.gradle.kts | 1 + .../connectivity/http/HttpErrorException.java | 36 +++ .../connectivity/http/HttpSender.java | 22 ++ .../connectivity/http/OkHttpSender.java | 124 ++++++++++ .../request/delay/AcceptsDelaySuggestion.java | 31 +++ .../request/service/HttpRequestService.java | 222 ++++++++++++++++++ .../request/service/RequestService.java | 70 ++++++ .../client/internal/response/Response.java | 13 + 8 files changed, 519 insertions(+) create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/Response.java diff --git a/opamp-client/build.gradle.kts b/opamp-client/build.gradle.kts index a1f0c698e..482a2c4f5 100644 --- a/opamp-client/build.gradle.kts +++ b/opamp-client/build.gradle.kts @@ -12,6 +12,7 @@ description = "Client implementation of the OpAMP spec." otelJava.moduleName.set("io.opentelemetry.contrib.opamp.client") dependencies { + implementation("com.squareup.okhttp3:okhttp") annotationProcessor("com.google.auto.value:auto-value") compileOnly("com.google.auto.value:auto-value-annotations") } diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java new file mode 100644 index 000000000..8cbb8da6e --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java @@ -0,0 +1,36 @@ +package io.opentelemetry.opamp.client.internal.connectivity.http; + +import java.util.Objects; + +@SuppressWarnings("serial") +public class HttpErrorException extends Exception { + public final int errorCode; + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof HttpErrorException)) { + return false; + } + HttpErrorException that = (HttpErrorException) o; + return errorCode == that.errorCode && Objects.equals(getMessage(), that.getMessage()); + } + + @Override + public int hashCode() { + return Objects.hash(errorCode, getMessage()); + } + + /** + * Constructs an HTTP error related exception. + * + * @param errorCode The HTTP error code. + * @param message The HTTP error message associated with the code. + */ + public HttpErrorException(int errorCode, String message) { + super(message); + this.errorCode = errorCode; + } +} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java new file mode 100644 index 000000000..01033d975 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java @@ -0,0 +1,22 @@ +package io.opentelemetry.opamp.client.internal.connectivity.http; + +import java.io.Closeable; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.concurrent.CompletableFuture; +import java.util.function.Consumer; + +public interface HttpSender { + + CompletableFuture send(Consumer writer, int contentLength); + + interface Response extends Closeable { + int statusCode(); + + String statusMessage(); + + InputStream bodyInputStream(); + + String getHeader(String name); + } +} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java new file mode 100644 index 000000000..fc0119dca --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java @@ -0,0 +1,124 @@ +package io.opentelemetry.opamp.client.internal.connectivity.http; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.concurrent.CompletableFuture; +import java.util.function.Consumer; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.RequestBody; +import okio.BufferedSink; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +public class OkHttpSender implements HttpSender { + private final OkHttpClient client; + private final String url; + + public static OkHttpSender create(String url) { + return create(url, new OkHttpClient()); + } + + public static OkHttpSender create(String url, OkHttpClient client) { + return new OkHttpSender(url, client); + } + + private OkHttpSender(String url, OkHttpClient client) { + this.url = url; + this.client = client; + } + + @Override + public CompletableFuture send(Consumer writer, int contentLength) { + CompletableFuture future = new CompletableFuture<>(); + okhttp3.Request.Builder builder = new okhttp3.Request.Builder().url(url); + String contentType = "application/x-protobuf"; + builder.addHeader("Content-Type", contentType); + + RequestBody body = new RawRequestBody(writer, contentLength, MediaType.parse(contentType)); + builder.post(body); + + try { + okhttp3.Response response = client.newCall(builder.build()).execute(); + if (response.isSuccessful()) { + if (response.body() != null) { + future.complete(new OkHttpResponse(response)); + } + } else { + future.completeExceptionally(new HttpErrorException(response.code(), response.message())); + } + } catch (IOException e) { + future.completeExceptionally(e); + } + + future.completeExceptionally(new IllegalStateException()); + + return future; + } + + private static class OkHttpResponse implements Response { + private final okhttp3.Response response; + + private OkHttpResponse(okhttp3.Response response) { + this.response = response; + } + + @Override + public int statusCode() { + return response.code(); + } + + @Override + public String statusMessage() { + return response.message(); + } + + @Override + public InputStream bodyInputStream() { + if (response.body() != null) { + return response.body().byteStream(); + } + throw new NullPointerException(); + } + + @Override + public String getHeader(String name) { + return response.headers().get(name); + } + + @Override + public void close() { + response.close(); + } + } + + private static class RawRequestBody extends RequestBody { + private final Consumer writer; + private final int contentLength; + private final MediaType contentType; + + private RawRequestBody( + Consumer writer, int contentLength, MediaType contentType) { + this.writer = writer; + this.contentLength = contentLength; + this.contentType = contentType; + } + + @Nullable + @Override + public MediaType contentType() { + return contentType; + } + + @Override + public long contentLength() { + return contentLength; + } + + @Override + public void writeTo(@NotNull BufferedSink bufferedSink) { + writer.accept(bufferedSink.outputStream()); + } + } +} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java new file mode 100644 index 000000000..43fd71715 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java @@ -0,0 +1,31 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package io.opentelemetry.opamp.client.internal.request.delay; + +import java.time.Duration; + +/** + * A {@link PeriodicDelay} implementation that wants to accept delay time suggestions, as explained + * here, + * must implement this interface. + */ +public interface AcceptsDelaySuggestion { + void suggestDelay(Duration delay); +} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java new file mode 100644 index 000000000..38a1982d8 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -0,0 +1,222 @@ +package io.opentelemetry.opamp.client.internal.request.service; + +import io.opentelemetry.opamp.client.internal.connectivity.http.HttpErrorException; +import io.opentelemetry.opamp.client.internal.connectivity.http.HttpSender; +import io.opentelemetry.opamp.client.internal.request.Request; +import io.opentelemetry.opamp.client.internal.request.delay.AcceptsDelaySuggestion; +import io.opentelemetry.opamp.client.internal.request.delay.PeriodicDelay; +import io.opentelemetry.opamp.client.internal.request.delay.PeriodicTaskExecutor; +import io.opentelemetry.opamp.client.internal.response.Response; +import java.io.IOException; +import java.io.OutputStream; +import java.time.Duration; +import java.util.Objects; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; +import java.util.function.Supplier; +import javax.annotation.Nullable; +import opamp.proto.AgentToServer; +import opamp.proto.ServerErrorResponse; +import opamp.proto.ServerErrorResponseType; +import opamp.proto.ServerToAgent; + +public final class HttpRequestService implements RequestService, Runnable { + private final HttpSender requestSender; + private final PeriodicTaskExecutor executor; + private final PeriodicDelay periodicRequestDelay; + private final PeriodicDelay periodicRetryDelay; + private final Object runningLock = new Object(); + private final AtomicBoolean retryModeEnabled = new AtomicBoolean(false); + @Nullable private Callback callback; + @Nullable private Supplier requestSupplier; + private boolean isRunning = false; + private boolean isStopped = false; + public static final PeriodicDelay DEFAULT_DELAY_BETWEEN_REQUESTS = + PeriodicDelay.ofFixedDuration(Duration.ofSeconds(30)); + + /** + * Creates an {@link HttpRequestService}. + * + * @param requestSender The HTTP sender implementation. + */ + public static HttpRequestService create(HttpSender requestSender) { + return create(requestSender, DEFAULT_DELAY_BETWEEN_REQUESTS, DEFAULT_DELAY_BETWEEN_REQUESTS); + } + + /** + * Creates an {@link HttpRequestService}. + * + * @param requestSender The HTTP sender implementation. + * @param periodicRequestDelay The time to wait between requests in general. + * @param periodicRetryDelay The time to wait between retries. + */ + public static HttpRequestService create( + HttpSender requestSender, + PeriodicDelay periodicRequestDelay, + PeriodicDelay periodicRetryDelay) { + return new HttpRequestService( + requestSender, + PeriodicTaskExecutor.create(periodicRequestDelay), + periodicRequestDelay, + periodicRetryDelay); + } + + HttpRequestService( + HttpSender requestSender, + PeriodicTaskExecutor executor, + PeriodicDelay periodicRequestDelay, + PeriodicDelay periodicRetryDelay) { + this.requestSender = requestSender; + this.executor = executor; + this.periodicRequestDelay = periodicRequestDelay; + this.periodicRetryDelay = periodicRetryDelay; + } + + @Override + public void start(Callback callback, Supplier requestSupplier) { + synchronized (runningLock) { + if (isStopped) { + throw new IllegalStateException("RequestDispatcher has been stopped"); + } + if (isRunning) { + throw new IllegalStateException("RequestDispatcher is already running"); + } + this.callback = callback; + this.requestSupplier = requestSupplier; + executor.start(this); + isRunning = true; + } + } + + @Override + public void stop() { + synchronized (runningLock) { + if (!isRunning || isStopped) { + return; + } + isStopped = true; + executor.executeNow(); + executor.stop(); + } + } + + private void enableRetryMode(@Nullable Duration suggestedDelay) { + if (retryModeEnabled.compareAndSet(false, true)) { + if (suggestedDelay != null && periodicRetryDelay instanceof AcceptsDelaySuggestion) { + ((AcceptsDelaySuggestion) periodicRetryDelay).suggestDelay(suggestedDelay); + } + executor.setPeriodicDelay(periodicRetryDelay); + } + } + + private void disableRetryMode() { + if (retryModeEnabled.compareAndSet(true, false)) { + executor.setPeriodicDelay(periodicRequestDelay); + } + } + + @Override + public void sendRequest() { + if (!retryModeEnabled.get()) { + executor.executeNow(); + } + } + + @Override + public void run() { + doSendRequest(); + } + + private void doSendRequest() { + try { + AgentToServer agentToServer = + Objects.requireNonNull(requestSupplier).get().getAgentToServer(); + + byte[] data = agentToServer.encodeByteString().toByteArray(); + try (HttpSender.Response response = + requestSender.send(new ByteArrayWriter(data), data.length).get()) { + if (isSuccessful(response)) { + handleSuccessResponse( + Response.create(ServerToAgent.ADAPTER.decode(response.bodyInputStream()))); + } else { + handleHttpError(response); + } + } catch (IOException e) { + getCallback().onRequestFailed(e); + } + + } catch (InterruptedException e) { + getCallback().onRequestFailed(e); + } catch (ExecutionException e) { + if (e.getCause() != null) { + getCallback().onRequestFailed(e.getCause()); + } else { + getCallback().onRequestFailed(e); + } + } + } + + private void handleHttpError(HttpSender.Response response) { + int errorCode = response.statusCode(); + getCallback().onRequestFailed(new HttpErrorException(errorCode, response.statusMessage())); + + if (errorCode == 503 || errorCode == 429) { + String retryAfterHeader = response.getHeader("Retry-After"); + Duration retryAfter = null; + if (retryAfterHeader != null) { + // retryAfter = TODO parse header to duration + } + enableRetryMode(retryAfter); + } + } + + private static boolean isSuccessful(HttpSender.Response response) { + return response.statusCode() >= 200 && response.statusCode() < 300; + } + + private void handleSuccessResponse(Response response) { + if (retryModeEnabled.get()) { + disableRetryMode(); + } + ServerToAgent serverToAgent = response.getServerToAgent(); + + if (serverToAgent.error_response != null) { + handleErrorResponse(serverToAgent.error_response); + } + + getCallback().onRequestSuccess(response); + } + + private void handleErrorResponse(ServerErrorResponse errorResponse) { + if (errorResponse.type.equals(ServerErrorResponseType.ServerErrorResponseType_Unavailable)) { + Duration retryAfter = null; + if (errorResponse.retry_info != null) { + retryAfter = Duration.ofNanos(errorResponse.retry_info.retry_after_nanoseconds); + } + enableRetryMode(retryAfter); + } + } + + private Callback getCallback() { + return Objects.requireNonNull(callback); + } + + private static class ByteArrayWriter implements Consumer { + private final byte[] data; + + private ByteArrayWriter(byte[] data) { + this.data = data; + } + + @SuppressWarnings("ThrowSpecificExceptions") + @Override + public void accept(OutputStream outputStream) { + try { + outputStream.write(data); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + } +} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java new file mode 100644 index 000000000..f9f5530f2 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java @@ -0,0 +1,70 @@ +package io.opentelemetry.opamp.client.internal.request.service; + +import io.opentelemetry.opamp.client.internal.OpampClient; +import io.opentelemetry.opamp.client.internal.request.Request; +import io.opentelemetry.opamp.client.internal.response.Response; +import java.util.function.Supplier; + +/** + * Handles the network connectivity in general, its implementation can choose what protocol to use + * (HTTP or WebSocket) and should provide the necessary configurations options depending on the + * case. There are 2 implementations ready to use, {@link HttpRequestService}, for using HTTP, and + * {@link WebSocketRequestService} for using WebSocket. The {@link OpampClient} must not be aware of + * the specific implementation it uses as it can expect the same behavior from either. + */ +public interface RequestService { + + /** + * Starts the service. The actions done in this method depend on the implementation. For HTTP + * there's nothing to do here, whereas for WebSocket this is where the connectivity is started. + * + * @param callback This is the only way that the service can communicate back to the {@link + * OpampClient} implementation. + * @param requestSupplier This supplier must be queried every time a new request is about to be + * sent. + */ + void start(Callback callback, Supplier requestSupplier); + + /** Triggers a new request send. */ + void sendRequest(); + + /** + * Clears the service for good. No further calls to {@link #sendRequest()} can be made after this + * method is called. + */ + void stop(); + + /** Allows the service to talk back to the {@link OpampClient} implementation. */ + interface Callback { + /** + * For WebSocket implementations, this is called when the connection is established. For HTTP + * implementations, this is called on every HTTP request that ends successfully. + */ + void onConnectionSuccess(); + + /** + * For WebSocket implementations, this is called when the connection cannot be made or is lost. + * For HTTP implementations, this is called on every HTTP request that fails. + * + * @param throwable The detailed error. + */ + void onConnectionFailed(Throwable throwable); + + /** + * For WebSocket implementations, this is called every time there's a new message from the + * server. For HTTP implementations, this is called when a successful HTTP request is finished + * with a valid server to agent response body. + * + * @param response The server to agent message. + */ + void onRequestSuccess(Response response); + + /** + * For both HTTP and WebSocket implementations, this is called when an attempt at sending a + * message fails. + * + * @param throwable The detailed error. + */ + void onRequestFailed(Throwable throwable); + } +} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/Response.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/Response.java new file mode 100644 index 000000000..84171dd48 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/Response.java @@ -0,0 +1,13 @@ +package io.opentelemetry.opamp.client.internal.response; + +import com.google.auto.value.AutoValue; +import opamp.proto.ServerToAgent; + +@AutoValue +public abstract class Response { + public abstract ServerToAgent getServerToAgent(); + + public static Response create(ServerToAgent serverToAgent) { + return new AutoValue_Response(serverToAgent); + } +} From 815b7c5154c19dc8e86c01d7e8504d324ae7b411 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 30 May 2025 15:02:51 +0200 Subject: [PATCH 04/38] Adding tests --- opamp-client/build.gradle.kts | 1 + .../service/HttpRequestServiceTest.java | 319 ++++++++++++++++++ 2 files changed, 320 insertions(+) create mode 100644 opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java diff --git a/opamp-client/build.gradle.kts b/opamp-client/build.gradle.kts index 482a2c4f5..6045e6add 100644 --- a/opamp-client/build.gradle.kts +++ b/opamp-client/build.gradle.kts @@ -15,6 +15,7 @@ dependencies { implementation("com.squareup.okhttp3:okhttp") annotationProcessor("com.google.auto.value:auto-value") compileOnly("com.google.auto.value:auto-value-annotations") + testImplementation("org.mockito:mockito-inline") } val opampReleaseInfo = tasks.register("opampLastReleaseInfo") { diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java new file mode 100644 index 000000000..c9aed2d87 --- /dev/null +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -0,0 +1,319 @@ +package io.opentelemetry.opamp.client.internal.request.service; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import io.opentelemetry.opamp.client.internal.connectivity.http.HttpErrorException; +import io.opentelemetry.opamp.client.internal.connectivity.http.HttpSender; +import io.opentelemetry.opamp.client.internal.request.Request; +import io.opentelemetry.opamp.client.internal.request.delay.AcceptsDelaySuggestion; +import io.opentelemetry.opamp.client.internal.request.delay.PeriodicDelay; +import io.opentelemetry.opamp.client.internal.request.delay.PeriodicTaskExecutor; +import io.opentelemetry.opamp.client.internal.response.Response; +import java.io.ByteArrayInputStream; +import java.time.Duration; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import java.util.function.Supplier; +import opamp.proto.AgentToServer; +import opamp.proto.RetryInfo; +import opamp.proto.ServerErrorResponse; +import opamp.proto.ServerErrorResponseType; +import opamp.proto.ServerToAgent; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InOrder; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@SuppressWarnings("unchecked") +@ExtendWith(MockitoExtension.class) +class HttpRequestServiceTest { + @Mock private HttpSender requestSender; + @Mock private PeriodicDelay periodicRequestDelay; + @Mock private TestPeriodicRetryDelay periodicRetryDelay; + @Mock private PeriodicTaskExecutor executor; + @Mock private RequestService.Callback callback; + @Mock private Supplier requestSupplier; + private int requestSize = -1; + private HttpRequestService httpRequestService; + + @BeforeEach + void setUp() { + httpRequestService = + new HttpRequestService(requestSender, executor, periodicRequestDelay, periodicRetryDelay); + } + + @Test + void verifyStart() { + httpRequestService.start(callback, requestSupplier); + + InOrder inOrder = inOrder(periodicRequestDelay, executor); + inOrder.verify(executor).start(httpRequestService); + + // Try starting it again: + try { + httpRequestService.start(callback, requestSupplier); + fail(); + } catch (IllegalStateException e) { + assertThat(e).hasMessage("RequestDispatcher is already running"); + } + } + + @Test + void verifyStop() { + httpRequestService.start(callback, requestSupplier); + httpRequestService.stop(); + + verify(executor).stop(); + + // Try stopping it again: + clearInvocations(executor); + httpRequestService.stop(); + verifyNoInteractions(executor); + } + + @Test + void verifyStop_whenNotStarted() { + httpRequestService.stop(); + + verifyNoInteractions(executor, requestSender, periodicRequestDelay); + } + + @Test + void whenTryingToStartAfterStopHasBeenCalled_throwException() { + httpRequestService.start(callback, requestSupplier); + httpRequestService.stop(); + try { + httpRequestService.start(callback, requestSupplier); + } catch (IllegalStateException e) { + assertThat(e).hasMessage("RequestDispatcher has been stopped"); + } + } + + @Test + void verifySendingRequest_happyPath() { + HttpSender.Response httpResponse = mock(); + ServerToAgent serverToAgent = new ServerToAgent.Builder().build(); + attachServerToAgentMessage(serverToAgent.encodeByteString().toByteArray(), httpResponse); + prepareRequest(); + enqueueResponse(httpResponse); + + httpRequestService.run(); + + verify(requestSender).send(any(), eq(requestSize)); + verify(callback).onRequestSuccess(Response.create(serverToAgent)); + } + + @Test + void verifySendingRequest_whenTheresAParsingError() { + HttpSender.Response httpResponse = mock(); + attachServerToAgentMessage(new byte[] {1, 2, 3}, httpResponse); + prepareRequest(); + enqueueResponse(httpResponse); + + httpRequestService.run(); + + verify(requestSender).send(any(), eq(requestSize)); + verify(callback).onRequestFailed(any()); + } + + @Test + void verifySendingRequest_whenThereIsAnExecutionError() + throws ExecutionException, InterruptedException { + prepareRequest(); + CompletableFuture future = mock(); + when(requestSender.send(any(), anyInt())).thenReturn(future); + Exception myException = mock(); + doThrow(new ExecutionException(myException)).when(future).get(); + + httpRequestService.run(); + + verify(requestSender).send(any(), eq(requestSize)); + verify(callback).onRequestFailed(myException); + } + + @Test + void verifySendingRequest_whenThereIsAnInterruptedException() + throws ExecutionException, InterruptedException { + prepareRequest(); + CompletableFuture future = mock(); + when(requestSender.send(any(), anyInt())).thenReturn(future); + InterruptedException myException = mock(); + doThrow(myException).when(future).get(); + + httpRequestService.run(); + + verify(requestSender).send(any(), eq(requestSize)); + verify(callback).onRequestFailed(myException); + } + + @Test + void verifySendingRequest_whenThereIsAGenericHttpError() { + HttpSender.Response response = mock(); + when(response.statusCode()).thenReturn(500); + when(response.statusMessage()).thenReturn("Error message"); + prepareRequest(); + enqueueResponse(response); + + httpRequestService.run(); + + verify(callback).onRequestFailed(new HttpErrorException(500, "Error message")); + verifyNoInteractions(executor); + } + + @Test + void verifySendingRequest_whenThereIsATooManyRequestsError() { + HttpSender.Response response = mock(); + when(response.statusCode()).thenReturn(429); + when(response.statusMessage()).thenReturn("Error message"); + prepareRequest(); + enqueueResponse(response); + + httpRequestService.run(); + + verify(callback).onRequestFailed(new HttpErrorException(429, "Error message")); + verify(executor).setPeriodicDelay(periodicRetryDelay); + } + + @Test + void verifySendingRequest_whenServerProvidesRetryInfo_usingTheProvidedInfo() { + HttpSender.Response response = mock(); + long nanosecondsToWaitForRetry = 1000; + ServerErrorResponse errorResponse = + new ServerErrorResponse.Builder() + .type(ServerErrorResponseType.ServerErrorResponseType_Unavailable) + .retry_info( + new RetryInfo.Builder().retry_after_nanoseconds(nanosecondsToWaitForRetry).build()) + .build(); + ServerToAgent serverToAgent = new ServerToAgent.Builder().error_response(errorResponse).build(); + attachServerToAgentMessage(serverToAgent.encodeByteString().toByteArray(), response); + prepareRequest(); + enqueueResponse(response); + + httpRequestService.run(); + + verify(callback).onRequestSuccess(Response.create(serverToAgent)); + verify(periodicRetryDelay).suggestDelay(Duration.ofNanos(nanosecondsToWaitForRetry)); + verify(executor).setPeriodicDelay(periodicRetryDelay); + } + + @Test + void verifySendingRequest_whenServerIsUnavailable() { + HttpSender.Response response = mock(); + ServerErrorResponse errorResponse = + new ServerErrorResponse.Builder() + .type(ServerErrorResponseType.ServerErrorResponseType_Unavailable) + .build(); + ServerToAgent serverToAgent = new ServerToAgent.Builder().error_response(errorResponse).build(); + attachServerToAgentMessage(serverToAgent.encodeByteString().toByteArray(), response); + prepareRequest(); + enqueueResponse(response); + + httpRequestService.run(); + + verify(callback).onRequestSuccess(Response.create(serverToAgent)); + verify(periodicRetryDelay, never()).suggestDelay(any()); + verify(executor).setPeriodicDelay(periodicRetryDelay); + } + + @Test + void verifySendingRequest_whenThereIsAServiceUnavailableError() { + HttpSender.Response response = mock(); + when(response.statusCode()).thenReturn(503); + when(response.statusMessage()).thenReturn("Error message"); + prepareRequest(); + enqueueResponse(response); + + httpRequestService.run(); + + verify(callback).onRequestFailed(new HttpErrorException(503, "Error message")); + verify(executor).setPeriodicDelay(periodicRetryDelay); + } + + @Test + void verifySendingRequest_duringRegularMode() { + httpRequestService.sendRequest(); + + verify(executor).executeNow(); + } + + @Test + void verifySendingRequest_duringRetryMode() { + enableRetryMode(); + + httpRequestService.sendRequest(); + + verify(executor, never()).executeNow(); + } + + @Test + void verifySuccessfulSendingRequest_duringRetryMode() { + enableRetryMode(); + HttpSender.Response response = mock(); + attachServerToAgentMessage( + new ServerToAgent.Builder().build().encodeByteString().toByteArray(), response); + enqueueResponse(response); + + httpRequestService.run(); + + verify(executor).setPeriodicDelay(periodicRequestDelay); + } + + private void enableRetryMode() { + HttpSender.Response response = mock(); + when(response.statusCode()).thenReturn(503); + when(response.statusMessage()).thenReturn("Error message"); + prepareRequest(); + enqueueResponse(response); + + httpRequestService.run(); + } + + private void prepareRequest() { + httpRequestService.start(callback, requestSupplier); + clearInvocations(executor); + AgentToServer agentToServer = new AgentToServer.Builder().sequence_num(10).build(); + requestSize = agentToServer.encodeByteString().size(); + Request request = Request.create(agentToServer); + when(requestSupplier.get()).thenReturn(request); + } + + private void enqueueResponse(HttpSender.Response httpResponse) { + when(requestSender.send(any(), anyInt())) + .thenReturn(CompletableFuture.completedFuture(httpResponse)); + } + + private static void attachServerToAgentMessage( + byte[] serverToAgent, HttpSender.Response httpResponse) { + ByteArrayInputStream byteArrayInputStream = new ByteArrayInputStream(serverToAgent); + when(httpResponse.statusCode()).thenReturn(200); + when(httpResponse.bodyInputStream()).thenReturn(byteArrayInputStream); + } + + private static class TestPeriodicRetryDelay implements PeriodicDelay, AcceptsDelaySuggestion { + + @Override + public void suggestDelay(Duration delay) {} + + @Override + public Duration getNextDelay() { + return null; + } + + @Override + public void reset() {} + } +} From bb084c3f0610cd6cbcdc2b19ec9ded76a85e1856 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 30 May 2025 16:49:57 +0200 Subject: [PATCH 05/38] Updating HttpRequestService --- .../connectivity/http/OkHttpSender.java | 2 +- .../request/service/HttpRequestService.java | 23 +++++-------------- 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java index fc0119dca..e8f3b7c99 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java @@ -12,7 +12,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -public class OkHttpSender implements HttpSender { +public final class OkHttpSender implements HttpSender { private final OkHttpClient client; private final String url; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 38a1982d8..07aeaf493 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -26,12 +26,10 @@ public final class HttpRequestService implements RequestService, Runnable { private final PeriodicTaskExecutor executor; private final PeriodicDelay periodicRequestDelay; private final PeriodicDelay periodicRetryDelay; - private final Object runningLock = new Object(); private final AtomicBoolean retryModeEnabled = new AtomicBoolean(false); + private final AtomicBoolean isRunning = new AtomicBoolean(false); @Nullable private Callback callback; @Nullable private Supplier requestSupplier; - private boolean isRunning = false; - private boolean isStopped = false; public static final PeriodicDelay DEFAULT_DELAY_BETWEEN_REQUESTS = PeriodicDelay.ofFixedDuration(Duration.ofSeconds(30)); @@ -75,27 +73,18 @@ public static HttpRequestService create( @Override public void start(Callback callback, Supplier requestSupplier) { - synchronized (runningLock) { - if (isStopped) { - throw new IllegalStateException("RequestDispatcher has been stopped"); - } - if (isRunning) { - throw new IllegalStateException("RequestDispatcher is already running"); - } + if (isRunning.compareAndSet(false, true)) { this.callback = callback; this.requestSupplier = requestSupplier; executor.start(this); - isRunning = true; + } else { + throw new IllegalStateException("RequestDispatcher is already running"); } } @Override public void stop() { - synchronized (runningLock) { - if (!isRunning || isStopped) { - return; - } - isStopped = true; + if (isRunning.compareAndSet(true, false)) { executor.executeNow(); executor.stop(); } @@ -165,7 +154,7 @@ private void handleHttpError(HttpSender.Response response) { String retryAfterHeader = response.getHeader("Retry-After"); Duration retryAfter = null; if (retryAfterHeader != null) { - // retryAfter = TODO parse header to duration + retryAfter = Duration.ofSeconds(Long.parseLong(retryAfterHeader)); } enableRetryMode(retryAfter); } From 61fb2ee3f8d1b4c36346e2f8aaa45d186f14598c Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 30 May 2025 16:50:09 +0200 Subject: [PATCH 06/38] Creating RetryAfterParser --- .../connectivity/http/RetryAfterParser.java | 44 +++++++++++++++++++ .../client/internal/tools/SystemTime.java | 16 +++++++ .../http/RetryAfterParserTest.java | 24 ++++++++++ 3 files changed, 84 insertions(+) create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/tools/SystemTime.java create mode 100644 opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java new file mode 100644 index 000000000..c6aab3257 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java @@ -0,0 +1,44 @@ +package io.opentelemetry.opamp.client.internal.connectivity.http; + +import io.opentelemetry.opamp.client.internal.tools.SystemTime; +import java.text.ParseException; +import java.text.SimpleDateFormat; +import java.time.Duration; +import java.util.Locale; +import java.util.regex.Pattern; + +public final class RetryAfterParser { + private final SystemTime systemTime; + public static final Pattern SECONDS_PATTERN = Pattern.compile("\\d+"); + public static final Pattern DATE_PATTERN = + Pattern.compile( + "^([A-Za-z]{3}, [0-3][0-9] [A-Za-z]{3} [0-9]{4} [0-2][0-9]:[0-5][0-9]:[0-5][0-9] GMT)$"); + private static final SimpleDateFormat dateFormat = + new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss z", Locale.US); + + public static RetryAfterParser getInstance() { + return new RetryAfterParser(SystemTime.getInstance()); + } + + RetryAfterParser(SystemTime systemTime) { + this.systemTime = systemTime; + } + + public Duration parse(String value) { + if (SECONDS_PATTERN.matcher(value).matches()) { + return Duration.ofSeconds(Long.parseLong(value)); + } else if (DATE_PATTERN.matcher(value).matches()) { + return Duration.ofMillis(toMilliseconds(value) - systemTime.getCurrentTimeMillis()); + } + throw new IllegalArgumentException("Invalid Retry-After value: " + value); + } + + @SuppressWarnings({"JavaUtilDate", "ThrowSpecificExceptions"}) + private static long toMilliseconds(String value) { + try { + return dateFormat.parse(value).getTime(); + } catch (ParseException e) { + throw new RuntimeException(e); + } + } +} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/tools/SystemTime.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/tools/SystemTime.java new file mode 100644 index 000000000..4d97967ba --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/tools/SystemTime.java @@ -0,0 +1,16 @@ +package io.opentelemetry.opamp.client.internal.tools; + +/** Utility to be able to mock the current system time for testing purposes. */ +public final class SystemTime { + private static final SystemTime INSTANCE = new SystemTime(); + + public static SystemTime getInstance() { + return INSTANCE; + } + + private SystemTime() {} + + public long getCurrentTimeMillis() { + return System.currentTimeMillis(); + } +} diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java new file mode 100644 index 000000000..13b8f6728 --- /dev/null +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java @@ -0,0 +1,24 @@ +package io.opentelemetry.opamp.client.internal.connectivity.http; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import io.opentelemetry.opamp.client.internal.tools.SystemTime; +import java.time.Duration; +import org.junit.jupiter.api.Test; + +class RetryAfterParserTest { + + @Test + void verifyParsing() { + SystemTime systemTime = mock(); + long currentTimeMillis = 1577836800000L; + when(systemTime.getCurrentTimeMillis()).thenReturn(currentTimeMillis); + + RetryAfterParser parser = new RetryAfterParser(systemTime); + + assertThat(parser.parse("123")).isEqualTo(Duration.ofSeconds(123)); + assertThat(parser.parse("Wed, 01 Jan 2020 01:00:00 GMT")).isEqualTo(Duration.ofHours(1)); + } +} From acce15e17b7c1700af432c4b0c20cc0ac427e0b5 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 30 May 2025 17:12:11 +0200 Subject: [PATCH 07/38] Adding suggested delay --- .../request/service/HttpRequestService.java | 11 +++-- .../service/HttpRequestServiceTest.java | 42 ++++++++++++++++++- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 07aeaf493..7a1a2cec3 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -2,6 +2,7 @@ import io.opentelemetry.opamp.client.internal.connectivity.http.HttpErrorException; import io.opentelemetry.opamp.client.internal.connectivity.http.HttpSender; +import io.opentelemetry.opamp.client.internal.connectivity.http.RetryAfterParser; import io.opentelemetry.opamp.client.internal.request.Request; import io.opentelemetry.opamp.client.internal.request.delay.AcceptsDelaySuggestion; import io.opentelemetry.opamp.client.internal.request.delay.PeriodicDelay; @@ -28,6 +29,7 @@ public final class HttpRequestService implements RequestService, Runnable { private final PeriodicDelay periodicRetryDelay; private final AtomicBoolean retryModeEnabled = new AtomicBoolean(false); private final AtomicBoolean isRunning = new AtomicBoolean(false); + private final RetryAfterParser retryAfterParser; @Nullable private Callback callback; @Nullable private Supplier requestSupplier; public static final PeriodicDelay DEFAULT_DELAY_BETWEEN_REQUESTS = @@ -57,18 +59,21 @@ public static HttpRequestService create( requestSender, PeriodicTaskExecutor.create(periodicRequestDelay), periodicRequestDelay, - periodicRetryDelay); + periodicRetryDelay, + RetryAfterParser.getInstance()); } HttpRequestService( HttpSender requestSender, PeriodicTaskExecutor executor, PeriodicDelay periodicRequestDelay, - PeriodicDelay periodicRetryDelay) { + PeriodicDelay periodicRetryDelay, + RetryAfterParser retryAfterParser) { this.requestSender = requestSender; this.executor = executor; this.periodicRequestDelay = periodicRequestDelay; this.periodicRetryDelay = periodicRetryDelay; + this.retryAfterParser = retryAfterParser; } @Override @@ -154,7 +159,7 @@ private void handleHttpError(HttpSender.Response response) { String retryAfterHeader = response.getHeader("Retry-After"); Duration retryAfter = null; if (retryAfterHeader != null) { - retryAfter = Duration.ofSeconds(Long.parseLong(retryAfterHeader)); + retryAfter = retryAfterParser.parse(retryAfterHeader); } enableRetryMode(retryAfter); } diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index c9aed2d87..46f1663d6 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -16,6 +16,7 @@ import io.opentelemetry.opamp.client.internal.connectivity.http.HttpErrorException; import io.opentelemetry.opamp.client.internal.connectivity.http.HttpSender; +import io.opentelemetry.opamp.client.internal.connectivity.http.RetryAfterParser; import io.opentelemetry.opamp.client.internal.request.Request; import io.opentelemetry.opamp.client.internal.request.delay.AcceptsDelaySuggestion; import io.opentelemetry.opamp.client.internal.request.delay.PeriodicDelay; @@ -53,7 +54,12 @@ class HttpRequestServiceTest { @BeforeEach void setUp() { httpRequestService = - new HttpRequestService(requestSender, executor, periodicRequestDelay, periodicRetryDelay); + new HttpRequestService( + requestSender, + executor, + periodicRequestDelay, + periodicRetryDelay, + RetryAfterParser.getInstance()); } @Test @@ -186,6 +192,23 @@ void verifySendingRequest_whenThereIsATooManyRequestsError() { verify(callback).onRequestFailed(new HttpErrorException(429, "Error message")); verify(executor).setPeriodicDelay(periodicRetryDelay); + verify(periodicRetryDelay, never()).suggestDelay(any()); + } + + @Test + void verifySendingRequest_whenThereIsATooManyRequestsError_withSuggestedDelay() { + HttpSender.Response response = mock(); + when(response.statusCode()).thenReturn(429); + when(response.statusMessage()).thenReturn("Error message"); + when(response.getHeader("Retry-After")).thenReturn("5"); + prepareRequest(); + enqueueResponse(response); + + httpRequestService.run(); + + verify(callback).onRequestFailed(new HttpErrorException(429, "Error message")); + verify(executor).setPeriodicDelay(periodicRetryDelay); + verify(periodicRetryDelay).suggestDelay(Duration.ofSeconds(5)); } @Test @@ -241,6 +264,23 @@ void verifySendingRequest_whenThereIsAServiceUnavailableError() { verify(callback).onRequestFailed(new HttpErrorException(503, "Error message")); verify(executor).setPeriodicDelay(periodicRetryDelay); + verify(periodicRetryDelay, never()).suggestDelay(any()); + } + + @Test + void verifySendingRequest_whenThereIsAServiceUnavailableError_withSuggestedDelay() { + HttpSender.Response response = mock(); + when(response.getHeader("Retry-After")).thenReturn("2"); + when(response.statusCode()).thenReturn(503); + when(response.statusMessage()).thenReturn("Error message"); + prepareRequest(); + enqueueResponse(response); + + httpRequestService.run(); + + verify(callback).onRequestFailed(new HttpErrorException(503, "Error message")); + verify(executor).setPeriodicDelay(periodicRetryDelay); + verify(periodicRetryDelay).suggestDelay(Duration.ofSeconds(2)); } @Test From 3dc6f1205f0a5b23bc987d70a2c2e7537e59ce69 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 30 May 2025 17:15:28 +0200 Subject: [PATCH 08/38] Clean up --- .../request/delay/AcceptsDelaySuggestion.java | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java index 43fd71715..7d8f6da97 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java @@ -1,21 +1,3 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ package io.opentelemetry.opamp.client.internal.request.delay; import java.time.Duration; From 5657876e3003daab83b56d77553bb5802b83a425 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 30 May 2025 17:15:49 +0200 Subject: [PATCH 09/38] Spotless --- .../internal/connectivity/http/HttpErrorException.java | 5 +++++ .../opamp/client/internal/connectivity/http/HttpSender.java | 5 +++++ .../client/internal/connectivity/http/OkHttpSender.java | 5 +++++ .../client/internal/connectivity/http/RetryAfterParser.java | 5 +++++ .../opentelemetry/opamp/client/internal/request/Request.java | 5 +++++ .../internal/request/delay/AcceptsDelaySuggestion.java | 5 +++++ .../client/internal/request/delay/FixedPeriodicDelay.java | 5 +++++ .../opamp/client/internal/request/delay/PeriodicDelay.java | 5 +++++ .../client/internal/request/delay/PeriodicTaskExecutor.java | 5 +++++ .../client/internal/request/service/HttpRequestService.java | 5 +++++ .../client/internal/request/service/RequestService.java | 5 +++++ .../opamp/client/internal/response/Response.java | 5 +++++ .../opamp/client/internal/tools/SystemTime.java | 5 +++++ .../internal/connectivity/http/RetryAfterParserTest.java | 5 +++++ .../internal/request/delay/PeriodicTaskExecutorTest.java | 5 +++++ .../internal/request/service/HttpRequestServiceTest.java | 5 +++++ 16 files changed, 80 insertions(+) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java index 8cbb8da6e..054d6c829 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.connectivity.http; import java.util.Objects; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java index 01033d975..643fb49be 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.connectivity.http; import java.io.Closeable; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java index e8f3b7c99..8aa56d143 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.connectivity.http; import java.io.IOException; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java index c6aab3257..46488e8ee 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.connectivity.http; import io.opentelemetry.opamp.client.internal.tools.SystemTime; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java index 56d768a25..d6a3c7aed 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Request.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.request; import com.google.auto.value.AutoValue; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java index 7d8f6da97..f53b00d45 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/AcceptsDelaySuggestion.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.request.delay; import java.time.Duration; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/FixedPeriodicDelay.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/FixedPeriodicDelay.java index f0d0c1481..2b54180e5 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/FixedPeriodicDelay.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/FixedPeriodicDelay.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.request.delay; import java.time.Duration; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicDelay.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicDelay.java index 6e5526796..67aa93491 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicDelay.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicDelay.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.request.delay; import java.time.Duration; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java index 410629146..183724e8b 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.request.delay; import java.util.Objects; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 7a1a2cec3..3c6cf2064 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.request.service; import io.opentelemetry.opamp.client.internal.connectivity.http.HttpErrorException; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java index f9f5530f2..e751196d4 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.request.service; import io.opentelemetry.opamp.client.internal.OpampClient; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/Response.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/Response.java index 84171dd48..c9b6bc19e 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/Response.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/Response.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.response; import com.google.auto.value.AutoValue; diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/tools/SystemTime.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/tools/SystemTime.java index 4d97967ba..1d7f9e61e 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/tools/SystemTime.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/tools/SystemTime.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.tools; /** Utility to be able to mock the current system time for testing purposes. */ diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java index 13b8f6728..13e7920af 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.connectivity.http; import static org.assertj.core.api.Assertions.assertThat; diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java index 13d7a3cc9..207e3af63 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.request.delay; import static org.junit.jupiter.api.Assertions.fail; diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index 46f1663d6..ca1c703e6 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.request.service; import static org.assertj.core.api.Assertions.assertThat; From d94ff62a99a629d624b5cbd091a444c0a82b0b83 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 11:01:56 +0200 Subject: [PATCH 10/38] Applying PR review suggestions --- .../internal/connectivity/http/HttpErrorException.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java index 054d6c829..2b0aac57c 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java @@ -7,9 +7,10 @@ import java.util.Objects; -@SuppressWarnings("serial") public class HttpErrorException extends Exception { - public final int errorCode; + private final int errorCode; + + private static final long serialVersionUID = 1L; @Override public boolean equals(Object o) { From 37bf5a98a0396ab5f73af6208aa549a3f0e56214 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 11:07:06 +0200 Subject: [PATCH 11/38] Applying PR review suggestions --- .../connectivity/http/HttpErrorException.java | 19 ++----------------- .../service/HttpRequestServiceTest.java | 18 +++++++++++++----- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java index 2b0aac57c..c1104118c 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpErrorException.java @@ -5,28 +5,13 @@ package io.opentelemetry.opamp.client.internal.connectivity.http; -import java.util.Objects; - public class HttpErrorException extends Exception { private final int errorCode; private static final long serialVersionUID = 1L; - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (!(o instanceof HttpErrorException)) { - return false; - } - HttpErrorException that = (HttpErrorException) o; - return errorCode == that.errorCode && Objects.equals(getMessage(), that.getMessage()); - } - - @Override - public int hashCode() { - return Objects.hash(errorCode, getMessage()); + public int getErrorCode() { + return errorCode; } /** diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index ca1c703e6..8667d2e45 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -40,6 +40,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -181,7 +182,7 @@ void verifySendingRequest_whenThereIsAGenericHttpError() { httpRequestService.run(); - verify(callback).onRequestFailed(new HttpErrorException(500, "Error message")); + verifyRequestFailedCallback(500); verifyNoInteractions(executor); } @@ -195,7 +196,7 @@ void verifySendingRequest_whenThereIsATooManyRequestsError() { httpRequestService.run(); - verify(callback).onRequestFailed(new HttpErrorException(429, "Error message")); + verifyRequestFailedCallback(429); verify(executor).setPeriodicDelay(periodicRetryDelay); verify(periodicRetryDelay, never()).suggestDelay(any()); } @@ -211,7 +212,7 @@ void verifySendingRequest_whenThereIsATooManyRequestsError_withSuggestedDelay() httpRequestService.run(); - verify(callback).onRequestFailed(new HttpErrorException(429, "Error message")); + verifyRequestFailedCallback(429); verify(executor).setPeriodicDelay(periodicRetryDelay); verify(periodicRetryDelay).suggestDelay(Duration.ofSeconds(5)); } @@ -267,7 +268,7 @@ void verifySendingRequest_whenThereIsAServiceUnavailableError() { httpRequestService.run(); - verify(callback).onRequestFailed(new HttpErrorException(503, "Error message")); + verifyRequestFailedCallback(503); verify(executor).setPeriodicDelay(periodicRetryDelay); verify(periodicRetryDelay, never()).suggestDelay(any()); } @@ -283,11 +284,18 @@ void verifySendingRequest_whenThereIsAServiceUnavailableError_withSuggestedDelay httpRequestService.run(); - verify(callback).onRequestFailed(new HttpErrorException(503, "Error message")); + verifyRequestFailedCallback(503); verify(executor).setPeriodicDelay(periodicRetryDelay); verify(periodicRetryDelay).suggestDelay(Duration.ofSeconds(2)); } + private void verifyRequestFailedCallback(int errorCode) { + ArgumentCaptor captor = ArgumentCaptor.forClass(HttpErrorException.class); + verify(callback).onRequestFailed(captor.capture()); + assertThat(captor.getValue().getErrorCode()).isEqualTo(errorCode); + assertThat(captor.getValue().getMessage()).isEqualTo("Error message"); + } + @Test void verifySendingRequest_duringRegularMode() { httpRequestService.sendRequest(); From 91f876a9a164a6e06c102f262221a6f41559fc9b Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 11:09:20 +0200 Subject: [PATCH 12/38] Applying PR review suggestions --- .../client/internal/connectivity/http/OkHttpSender.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java index 8aa56d143..c63de704d 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java @@ -29,6 +29,9 @@ public static OkHttpSender create(String url, OkHttpClient client) { return new OkHttpSender(url, client); } + private static final String CONTENT_TYPE = "application/x-protobuf"; + private static final MediaType MEDIA_TYPE = MediaType.parse(CONTENT_TYPE); + private OkHttpSender(String url, OkHttpClient client) { this.url = url; this.client = client; @@ -38,10 +41,9 @@ private OkHttpSender(String url, OkHttpClient client) { public CompletableFuture send(Consumer writer, int contentLength) { CompletableFuture future = new CompletableFuture<>(); okhttp3.Request.Builder builder = new okhttp3.Request.Builder().url(url); - String contentType = "application/x-protobuf"; - builder.addHeader("Content-Type", contentType); + builder.addHeader("Content-Type", CONTENT_TYPE); - RequestBody body = new RawRequestBody(writer, contentLength, MediaType.parse(contentType)); + RequestBody body = new RawRequestBody(writer, contentLength, MEDIA_TYPE); builder.post(body); try { From 84d17d8af91bb29d6203f8b82e862f1ae287c83f Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 11:14:04 +0200 Subject: [PATCH 13/38] Applying PR review suggestions --- .../connectivity/http/OkHttpSender.java | 38 ++++++++++++------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java index c63de704d..9a09393a9 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java @@ -10,9 +10,12 @@ import java.io.OutputStream; import java.util.concurrent.CompletableFuture; import java.util.function.Consumer; +import okhttp3.Call; +import okhttp3.Callback; import okhttp3.MediaType; import okhttp3.OkHttpClient; import okhttp3.RequestBody; +import okhttp3.Response; import okio.BufferedSink; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -46,20 +49,27 @@ public CompletableFuture send(Consumer writer, int conte RequestBody body = new RawRequestBody(writer, contentLength, MEDIA_TYPE); builder.post(body); - try { - okhttp3.Response response = client.newCall(builder.build()).execute(); - if (response.isSuccessful()) { - if (response.body() != null) { - future.complete(new OkHttpResponse(response)); - } - } else { - future.completeExceptionally(new HttpErrorException(response.code(), response.message())); - } - } catch (IOException e) { - future.completeExceptionally(e); - } - - future.completeExceptionally(new IllegalStateException()); + client + .newCall(builder.build()) + .enqueue( + new Callback() { + @Override + public void onResponse(@NotNull Call call, @NotNull okhttp3.Response response) { + if (response.isSuccessful()) { + if (response.body() != null) { + future.complete(new OkHttpResponse(response)); + } + } else { + future.completeExceptionally( + new HttpErrorException(response.code(), response.message())); + } + } + + @Override + public void onFailure(@NotNull Call call, @NotNull IOException e) { + future.completeExceptionally(e); + } + }); return future; } From 8625f8f4652419c527789cb230e37c0c62987365 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 11:19:14 +0200 Subject: [PATCH 14/38] Applying PR review suggestions --- .../client/internal/connectivity/http/OkHttpSender.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java index 9a09393a9..eba8bb01c 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java @@ -15,7 +15,6 @@ import okhttp3.MediaType; import okhttp3.OkHttpClient; import okhttp3.RequestBody; -import okhttp3.Response; import okio.BufferedSink; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -78,6 +77,9 @@ private static class OkHttpResponse implements Response { private final okhttp3.Response response; private OkHttpResponse(okhttp3.Response response) { + if (response.body() == null) { + throw new IllegalStateException(); + } this.response = response; } @@ -93,10 +95,7 @@ public String statusMessage() { @Override public InputStream bodyInputStream() { - if (response.body() != null) { - return response.body().byteStream(); - } - throw new NullPointerException(); + return response.body().byteStream(); } @Override From 70e7d95a92cade1219ba8a130e98c494c52416dd Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 12:05:02 +0200 Subject: [PATCH 15/38] Avoid using SimpleDateFormat --- .../connectivity/http/RetryAfterParser.java | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java index 46488e8ee..3a985fdbf 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java @@ -6,9 +6,9 @@ package io.opentelemetry.opamp.client.internal.connectivity.http; import io.opentelemetry.opamp.client.internal.tools.SystemTime; -import java.text.ParseException; -import java.text.SimpleDateFormat; import java.time.Duration; +import java.time.ZonedDateTime; +import java.time.format.DateTimeFormatter; import java.util.Locale; import java.util.regex.Pattern; @@ -18,8 +18,8 @@ public final class RetryAfterParser { public static final Pattern DATE_PATTERN = Pattern.compile( "^([A-Za-z]{3}, [0-3][0-9] [A-Za-z]{3} [0-9]{4} [0-2][0-9]:[0-5][0-9]:[0-5][0-9] GMT)$"); - private static final SimpleDateFormat dateFormat = - new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss z", Locale.US); + private static final DateTimeFormatter DATE_FORMAT = + DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss z", Locale.US); public static RetryAfterParser getInstance() { return new RetryAfterParser(SystemTime.getInstance()); @@ -38,12 +38,7 @@ public Duration parse(String value) { throw new IllegalArgumentException("Invalid Retry-After value: " + value); } - @SuppressWarnings({"JavaUtilDate", "ThrowSpecificExceptions"}) private static long toMilliseconds(String value) { - try { - return dateFormat.parse(value).getTime(); - } catch (ParseException e) { - throw new RuntimeException(e); - } + return ZonedDateTime.parse(value, DATE_FORMAT).toInstant().toEpochMilli(); } } From b190ad13c1442f5cd4d00e9f134cf0097ce677da Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 12:23:23 +0200 Subject: [PATCH 16/38] Validating when retry-after time is older than current one --- .../connectivity/http/RetryAfterParser.java | 13 +++++++++---- .../request/service/HttpRequestService.java | 6 +++++- .../connectivity/http/RetryAfterParserTest.java | 11 ++++++++--- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java index 3a985fdbf..0026d297e 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java @@ -10,6 +10,7 @@ import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; import java.util.Locale; +import java.util.Optional; import java.util.regex.Pattern; public final class RetryAfterParser { @@ -29,13 +30,17 @@ public static RetryAfterParser getInstance() { this.systemTime = systemTime; } - public Duration parse(String value) { + public Optional tryParse(String value) { + Duration duration = null; if (SECONDS_PATTERN.matcher(value).matches()) { - return Duration.ofSeconds(Long.parseLong(value)); + duration = Duration.ofSeconds(Long.parseLong(value)); } else if (DATE_PATTERN.matcher(value).matches()) { - return Duration.ofMillis(toMilliseconds(value) - systemTime.getCurrentTimeMillis()); + long difference = toMilliseconds(value) - systemTime.getCurrentTimeMillis(); + if (difference > 0) { + duration = Duration.ofMillis(difference); + } } - throw new IllegalArgumentException("Invalid Retry-After value: " + value); + return Optional.ofNullable(duration); } private static long toMilliseconds(String value) { diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 3c6cf2064..99359f06d 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -17,6 +17,7 @@ import java.io.OutputStream; import java.time.Duration; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; @@ -164,7 +165,10 @@ private void handleHttpError(HttpSender.Response response) { String retryAfterHeader = response.getHeader("Retry-After"); Duration retryAfter = null; if (retryAfterHeader != null) { - retryAfter = retryAfterParser.parse(retryAfterHeader); + Optional duration = retryAfterParser.tryParse(retryAfterHeader); + if (duration.isPresent()) { + retryAfter = duration.get(); + } } enableRetryMode(retryAfter); } diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java index 13e7920af..ece1ff0a7 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParserTest.java @@ -18,12 +18,17 @@ class RetryAfterParserTest { @Test void verifyParsing() { SystemTime systemTime = mock(); - long currentTimeMillis = 1577836800000L; + long currentTimeMillis = 1577836800000L; // Wed, 01 Jan 2020 00:00:00 GMT when(systemTime.getCurrentTimeMillis()).thenReturn(currentTimeMillis); RetryAfterParser parser = new RetryAfterParser(systemTime); - assertThat(parser.parse("123")).isEqualTo(Duration.ofSeconds(123)); - assertThat(parser.parse("Wed, 01 Jan 2020 01:00:00 GMT")).isEqualTo(Duration.ofHours(1)); + assertThat(parser.tryParse("123")).get().isEqualTo(Duration.ofSeconds(123)); + assertThat(parser.tryParse("Wed, 01 Jan 2020 01:00:00 GMT")) + .get() + .isEqualTo(Duration.ofHours(1)); + + // Check when provided time is older than the current one + assertThat(parser.tryParse("Tue, 31 Dec 2019 23:00:00 GMT")).isNotPresent(); } } From 7afe527af77905ab3699dfc023051addfbc38675 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 12:27:58 +0200 Subject: [PATCH 17/38] Updating javadoc based on PR review --- .../client/internal/request/service/RequestService.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java index e751196d4..e192e8435 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java @@ -20,8 +20,9 @@ public interface RequestService { /** - * Starts the service. The actions done in this method depend on the implementation. For HTTP - * there's nothing to do here, whereas for WebSocket this is where the connectivity is started. + * Starts the service. The actions done in this method depend on the implementation. For HTTP this + * is where the periodic poll task should get started, whereas for WebSocket this is where the + * connectivity is started. * * @param callback This is the only way that the service can communicate back to the {@link * OpampClient} implementation. From 51c9e0897f9cd11a4a9f18f8f2fe4a9f07c33cf4 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 12:35:51 +0200 Subject: [PATCH 18/38] Using daemon threads for opamp client, as mentioned in pr comments --- .../request/delay/PeriodicTaskExecutor.java | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java index 183724e8b..6a6ee030b 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java @@ -9,6 +9,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.Lock; @@ -25,7 +26,8 @@ public final class PeriodicTaskExecutor { public static PeriodicTaskExecutor create(PeriodicDelay initialPeriodicDelay) { return new PeriodicTaskExecutor( - Executors.newSingleThreadScheduledExecutor(), initialPeriodicDelay); + Executors.newSingleThreadScheduledExecutor(new DaemonThreadFactory()), + initialPeriodicDelay); } PeriodicTaskExecutor( @@ -79,4 +81,19 @@ public void run() { scheduleNext(); } } + + private static class DaemonThreadFactory implements ThreadFactory { + private final ThreadFactory delegate = Executors.defaultThreadFactory(); + + @Override + public Thread newThread(@Nonnull Runnable r) { + Thread t = delegate.newThread(r); + try { + t.setDaemon(true); + } catch (SecurityException e) { + // Well, we tried. + } + return t; + } + } } From 9a4e4836e11958a1931b231ffc84a864bddf02ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar?= <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 14:38:59 +0200 Subject: [PATCH 19/38] Update opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java Co-authored-by: Lauri Tulmin --- .../opamp/client/internal/connectivity/http/OkHttpSender.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java index eba8bb01c..3596d5098 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java @@ -78,7 +78,7 @@ private static class OkHttpResponse implements Response { private OkHttpResponse(okhttp3.Response response) { if (response.body() == null) { - throw new IllegalStateException(); + throw new IllegalStateException("null response body not expected"); } this.response = response; } From b299d2c23589fe4ec1470c8636856873da7bc136 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 14:45:42 +0200 Subject: [PATCH 20/38] Calling completable future when there's no response body --- .../client/internal/connectivity/http/OkHttpSender.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java index 3596d5098..c6c740a94 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java @@ -54,10 +54,8 @@ public CompletableFuture send(Consumer writer, int conte new Callback() { @Override public void onResponse(@NotNull Call call, @NotNull okhttp3.Response response) { - if (response.isSuccessful()) { - if (response.body() != null) { - future.complete(new OkHttpResponse(response)); - } + if (response.isSuccessful() && response.body() != null) { + future.complete(new OkHttpResponse(response)); } else { future.completeExceptionally( new HttpErrorException(response.code(), response.message())); From 71e6710005afa18f8702950bebbaffa19c02f74d Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 15:02:03 +0200 Subject: [PATCH 21/38] Updating retry-after pattern for seconds --- .../client/internal/connectivity/http/RetryAfterParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java index 0026d297e..2c053baf8 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java @@ -15,7 +15,7 @@ public final class RetryAfterParser { private final SystemTime systemTime; - public static final Pattern SECONDS_PATTERN = Pattern.compile("\\d+"); + public static final Pattern SECONDS_PATTERN = Pattern.compile("^\\d+$"); public static final Pattern DATE_PATTERN = Pattern.compile( "^([A-Za-z]{3}, [0-3][0-9] [A-Za-z]{3} [0-9]{4} [0-2][0-9]:[0-5][0-9]:[0-5][0-9] GMT)$"); From 451812e2061ffa5814aac5509568f15844c86f31 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 17:15:43 +0200 Subject: [PATCH 22/38] Simplifying retry-after parser patterns --- .../client/internal/connectivity/http/RetryAfterParser.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java index 2c053baf8..6cc2d1e3b 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/RetryAfterParser.java @@ -15,10 +15,10 @@ public final class RetryAfterParser { private final SystemTime systemTime; - public static final Pattern SECONDS_PATTERN = Pattern.compile("^\\d+$"); - public static final Pattern DATE_PATTERN = + private static final Pattern SECONDS_PATTERN = Pattern.compile("\\d+"); + private static final Pattern DATE_PATTERN = Pattern.compile( - "^([A-Za-z]{3}, [0-3][0-9] [A-Za-z]{3} [0-9]{4} [0-2][0-9]:[0-5][0-9]:[0-5][0-9] GMT)$"); + "[A-Za-z]{3}, [0-3][0-9] [A-Za-z]{3} [0-9]{4} [0-2][0-9]:[0-5][0-9]:[0-5][0-9] GMT"); private static final DateTimeFormatter DATE_FORMAT = DateTimeFormatter.ofPattern("EEE, dd MMM yyyy HH:mm:ss z", Locale.US); From 3c3830b9540a5501eaea2e212872f74f324282b9 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 4 Jun 2025 17:32:39 +0200 Subject: [PATCH 23/38] Created BodyWriter to use in HttpSender --- .../internal/connectivity/http/HttpSender.java | 8 ++++++-- .../internal/connectivity/http/OkHttpSender.java | 13 +++++-------- .../request/service/HttpRequestService.java | 12 +++--------- 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java index 643fb49be..704d172ac 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/HttpSender.java @@ -6,14 +6,18 @@ package io.opentelemetry.opamp.client.internal.connectivity.http; import java.io.Closeable; +import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.util.concurrent.CompletableFuture; -import java.util.function.Consumer; public interface HttpSender { - CompletableFuture send(Consumer writer, int contentLength); + CompletableFuture send(BodyWriter writer, int contentLength); + + interface BodyWriter { + void writeTo(OutputStream outputStream) throws IOException; + } interface Response extends Closeable { int statusCode(); diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java index c6c740a94..3e8f9a174 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/connectivity/http/OkHttpSender.java @@ -7,9 +7,7 @@ import java.io.IOException; import java.io.InputStream; -import java.io.OutputStream; import java.util.concurrent.CompletableFuture; -import java.util.function.Consumer; import okhttp3.Call; import okhttp3.Callback; import okhttp3.MediaType; @@ -40,7 +38,7 @@ private OkHttpSender(String url, OkHttpClient client) { } @Override - public CompletableFuture send(Consumer writer, int contentLength) { + public CompletableFuture send(BodyWriter writer, int contentLength) { CompletableFuture future = new CompletableFuture<>(); okhttp3.Request.Builder builder = new okhttp3.Request.Builder().url(url); builder.addHeader("Content-Type", CONTENT_TYPE); @@ -108,12 +106,11 @@ public void close() { } private static class RawRequestBody extends RequestBody { - private final Consumer writer; + private final BodyWriter writer; private final int contentLength; private final MediaType contentType; - private RawRequestBody( - Consumer writer, int contentLength, MediaType contentType) { + private RawRequestBody(BodyWriter writer, int contentLength, MediaType contentType) { this.writer = writer; this.contentLength = contentLength; this.contentType = contentType; @@ -131,8 +128,8 @@ public long contentLength() { } @Override - public void writeTo(@NotNull BufferedSink bufferedSink) { - writer.accept(bufferedSink.outputStream()); + public void writeTo(@NotNull BufferedSink bufferedSink) throws IOException { + writer.writeTo(bufferedSink.outputStream()); } } } diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 99359f06d..1d809e844 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -20,7 +20,6 @@ import java.util.Optional; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.Consumer; import java.util.function.Supplier; import javax.annotation.Nullable; import opamp.proto.AgentToServer; @@ -205,21 +204,16 @@ private Callback getCallback() { return Objects.requireNonNull(callback); } - private static class ByteArrayWriter implements Consumer { + private static class ByteArrayWriter implements HttpSender.BodyWriter { private final byte[] data; private ByteArrayWriter(byte[] data) { this.data = data; } - @SuppressWarnings("ThrowSpecificExceptions") @Override - public void accept(OutputStream outputStream) { - try { - outputStream.write(data); - } catch (IOException e) { - throw new RuntimeException(e); - } + public void writeTo(OutputStream outputStream) throws IOException { + outputStream.write(data); } } } From 260cbc46a78c24f2f2dcb9a499245ef8f69a8ce3 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Thu, 5 Jun 2025 15:29:31 +0200 Subject: [PATCH 24/38] Removing nested try with resources as suggested in PR reviews --- .../request/service/HttpRequestService.java | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 1d809e844..aa4d35761 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -128,24 +128,18 @@ public void run() { } private void doSendRequest() { - try { - AgentToServer agentToServer = - Objects.requireNonNull(requestSupplier).get().getAgentToServer(); - - byte[] data = agentToServer.encodeByteString().toByteArray(); - try (HttpSender.Response response = - requestSender.send(new ByteArrayWriter(data), data.length).get()) { - if (isSuccessful(response)) { - handleSuccessResponse( - Response.create(ServerToAgent.ADAPTER.decode(response.bodyInputStream()))); - } else { - handleHttpError(response); - } - } catch (IOException e) { - getCallback().onRequestFailed(e); + AgentToServer agentToServer = Objects.requireNonNull(requestSupplier).get().getAgentToServer(); + + byte[] data = agentToServer.encodeByteString().toByteArray(); + try (HttpSender.Response response = + requestSender.send(new ByteArrayWriter(data), data.length).get()) { + if (isSuccessful(response)) { + handleSuccessResponse( + Response.create(ServerToAgent.ADAPTER.decode(response.bodyInputStream()))); + } else { + handleHttpError(response); } - - } catch (InterruptedException e) { + } catch (IOException | InterruptedException e) { getCallback().onRequestFailed(e); } catch (ExecutionException e) { if (e.getCause() != null) { From 98d1b7f09749d6ae9775ffce6ea019153b01d6fd Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Thu, 5 Jun 2025 15:33:58 +0200 Subject: [PATCH 25/38] Clean up --- .../internal/request/service/HttpRequestService.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index aa4d35761..26ad7feb8 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -18,7 +18,10 @@ import java.time.Duration; import java.util.Objects; import java.util.Optional; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -131,15 +134,16 @@ private void doSendRequest() { AgentToServer agentToServer = Objects.requireNonNull(requestSupplier).get().getAgentToServer(); byte[] data = agentToServer.encodeByteString().toByteArray(); - try (HttpSender.Response response = - requestSender.send(new ByteArrayWriter(data), data.length).get()) { + CompletableFuture future = + requestSender.send(new ByteArrayWriter(data), data.length); + try (HttpSender.Response response = future.get(30, TimeUnit.SECONDS)) { if (isSuccessful(response)) { handleSuccessResponse( Response.create(ServerToAgent.ADAPTER.decode(response.bodyInputStream()))); } else { handleHttpError(response); } - } catch (IOException | InterruptedException e) { + } catch (IOException | InterruptedException | TimeoutException e) { getCallback().onRequestFailed(e); } catch (ExecutionException e) { if (e.getCause() != null) { From 08c3bc296e219c8438a4443bb53625995d5d0278 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Thu, 5 Jun 2025 15:35:31 +0200 Subject: [PATCH 26/38] Clean up --- .../request/service/HttpRequestService.java | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 26ad7feb8..f7fcf5cab 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -14,7 +14,6 @@ import io.opentelemetry.opamp.client.internal.request.delay.PeriodicTaskExecutor; import io.opentelemetry.opamp.client.internal.response.Response; import java.io.IOException; -import java.io.OutputStream; import java.time.Duration; import java.util.Objects; import java.util.Optional; @@ -135,7 +134,7 @@ private void doSendRequest() { byte[] data = agentToServer.encodeByteString().toByteArray(); CompletableFuture future = - requestSender.send(new ByteArrayWriter(data), data.length); + requestSender.send(outputStream -> outputStream.write(data), data.length); try (HttpSender.Response response = future.get(30, TimeUnit.SECONDS)) { if (isSuccessful(response)) { handleSuccessResponse( @@ -201,17 +200,4 @@ private void handleErrorResponse(ServerErrorResponse errorResponse) { private Callback getCallback() { return Objects.requireNonNull(callback); } - - private static class ByteArrayWriter implements HttpSender.BodyWriter { - private final byte[] data; - - private ByteArrayWriter(byte[] data) { - this.data = data; - } - - @Override - public void writeTo(OutputStream outputStream) throws IOException { - outputStream.write(data); - } - } } From 6d3baed5b1df9e32d2508f7dc707a0501ee740cd Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Thu, 5 Jun 2025 15:36:59 +0200 Subject: [PATCH 27/38] Updating tests --- .../request/service/HttpRequestServiceTest.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index 8667d2e45..f79fe7c1e 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -31,6 +31,8 @@ import java.time.Duration; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.function.Supplier; import opamp.proto.AgentToServer; import opamp.proto.RetryInfo; @@ -144,12 +146,12 @@ void verifySendingRequest_whenTheresAParsingError() { @Test void verifySendingRequest_whenThereIsAnExecutionError() - throws ExecutionException, InterruptedException { + throws ExecutionException, InterruptedException, TimeoutException { prepareRequest(); CompletableFuture future = mock(); when(requestSender.send(any(), anyInt())).thenReturn(future); Exception myException = mock(); - doThrow(new ExecutionException(myException)).when(future).get(); + doThrow(new ExecutionException(myException)).when(future).get(30, TimeUnit.SECONDS); httpRequestService.run(); @@ -159,12 +161,12 @@ void verifySendingRequest_whenThereIsAnExecutionError() @Test void verifySendingRequest_whenThereIsAnInterruptedException() - throws ExecutionException, InterruptedException { + throws ExecutionException, InterruptedException, TimeoutException { prepareRequest(); CompletableFuture future = mock(); when(requestSender.send(any(), anyInt())).thenReturn(future); InterruptedException myException = mock(); - doThrow(myException).when(future).get(); + doThrow(myException).when(future).get(30, TimeUnit.SECONDS); httpRequestService.run(); From 478e8c4e3812ecc576e512954e4a1116304ff9ca Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Thu, 5 Jun 2025 15:41:11 +0200 Subject: [PATCH 28/38] Updating tests --- .../client/internal/request/service/HttpRequestService.java | 5 +++++ .../internal/request/service/HttpRequestServiceTest.java | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index f7fcf5cab..bff639c06 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -36,6 +36,7 @@ public final class HttpRequestService implements RequestService, Runnable { private final PeriodicDelay periodicRetryDelay; private final AtomicBoolean retryModeEnabled = new AtomicBoolean(false); private final AtomicBoolean isRunning = new AtomicBoolean(false); + private final AtomicBoolean hasStopped = new AtomicBoolean(false); private final RetryAfterParser retryAfterParser; @Nullable private Callback callback; @Nullable private Supplier requestSupplier; @@ -85,6 +86,9 @@ public static HttpRequestService create( @Override public void start(Callback callback, Supplier requestSupplier) { + if (hasStopped.get()) { + throw new IllegalStateException("HttpRequestService cannot start after it has been stopped."); + } if (isRunning.compareAndSet(false, true)) { this.callback = callback; this.requestSupplier = requestSupplier; @@ -97,6 +101,7 @@ public void start(Callback callback, Supplier requestSupplier) { @Override public void stop() { if (isRunning.compareAndSet(true, false)) { + hasStopped.set(true); executor.executeNow(); executor.stop(); } diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index f79fe7c1e..b083002fc 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -112,8 +112,9 @@ void whenTryingToStartAfterStopHasBeenCalled_throwException() { httpRequestService.stop(); try { httpRequestService.start(callback, requestSupplier); + fail(); } catch (IllegalStateException e) { - assertThat(e).hasMessage("RequestDispatcher has been stopped"); + assertThat(e).hasMessage("HttpRequestService cannot start after it has been stopped."); } } From 77ae328674a0874c84517584dfdfa7120f059397 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Thu, 5 Jun 2025 15:48:11 +0200 Subject: [PATCH 29/38] Calling callback.onConnectionFailed --- .../internal/request/service/HttpRequestService.java | 7 ++++--- .../client/internal/request/service/RequestService.java | 2 +- .../internal/request/service/HttpRequestServiceTest.java | 6 +++--- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index bff639c06..3527bddcf 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -141,6 +141,7 @@ private void doSendRequest() { CompletableFuture future = requestSender.send(outputStream -> outputStream.write(data), data.length); try (HttpSender.Response response = future.get(30, TimeUnit.SECONDS)) { + getCallback().onConnectionSuccess(); if (isSuccessful(response)) { handleSuccessResponse( Response.create(ServerToAgent.ADAPTER.decode(response.bodyInputStream()))); @@ -148,12 +149,12 @@ private void doSendRequest() { handleHttpError(response); } } catch (IOException | InterruptedException | TimeoutException e) { - getCallback().onRequestFailed(e); + getCallback().onConnectionFailed(e); } catch (ExecutionException e) { if (e.getCause() != null) { - getCallback().onRequestFailed(e.getCause()); + getCallback().onConnectionFailed(e.getCause()); } else { - getCallback().onRequestFailed(e); + getCallback().onConnectionFailed(e); } } } diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java index e192e8435..ee47e4249 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/RequestService.java @@ -50,7 +50,7 @@ interface Callback { /** * For WebSocket implementations, this is called when the connection cannot be made or is lost. - * For HTTP implementations, this is called on every HTTP request that fails. + * For HTTP implementations, this is called on every HTTP request that cannot get a response. * * @param throwable The detailed error. */ diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index b083002fc..b0eb675f9 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -142,7 +142,7 @@ void verifySendingRequest_whenTheresAParsingError() { httpRequestService.run(); verify(requestSender).send(any(), eq(requestSize)); - verify(callback).onRequestFailed(any()); + verify(callback).onConnectionFailed(any()); } @Test @@ -157,7 +157,7 @@ void verifySendingRequest_whenThereIsAnExecutionError() httpRequestService.run(); verify(requestSender).send(any(), eq(requestSize)); - verify(callback).onRequestFailed(myException); + verify(callback).onConnectionFailed(myException); } @Test @@ -172,7 +172,7 @@ void verifySendingRequest_whenThereIsAnInterruptedException() httpRequestService.run(); verify(requestSender).send(any(), eq(requestSize)); - verify(callback).onRequestFailed(myException); + verify(callback).onConnectionFailed(myException); } @Test From 17ac9d64428d6a05e13e3704171094b934e1a726 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 6 Jun 2025 11:31:42 +0200 Subject: [PATCH 30/38] Removing executor for HttpRequestServiceTest --- .../request/service/HttpRequestService.java | 2 +- .../service/HttpRequestServiceTest.java | 157 +++++++++++------- 2 files changed, 100 insertions(+), 59 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 3527bddcf..5cb5ff0cf 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -94,7 +94,7 @@ public void start(Callback callback, Supplier requestSupplier) { this.requestSupplier = requestSupplier; executor.start(this); } else { - throw new IllegalStateException("RequestDispatcher is already running"); + throw new IllegalStateException("HttpRequestService is already running"); } } diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index b0eb675f9..6ec18536b 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -10,13 +10,10 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import io.opentelemetry.opamp.client.internal.connectivity.http.HttpErrorException; @@ -28,82 +25,64 @@ import io.opentelemetry.opamp.client.internal.request.delay.PeriodicTaskExecutor; import io.opentelemetry.opamp.client.internal.response.Response; import java.io.ByteArrayInputStream; +import java.io.Closeable; import java.time.Duration; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.Queue; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; import opamp.proto.AgentToServer; import opamp.proto.RetryInfo; import opamp.proto.ServerErrorResponse; import opamp.proto.ServerErrorResponseType; import opamp.proto.ServerToAgent; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.ArgumentCaptor; -import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @SuppressWarnings("unchecked") @ExtendWith(MockitoExtension.class) class HttpRequestServiceTest { - @Mock private HttpSender requestSender; - @Mock private PeriodicDelay periodicRequestDelay; - @Mock private TestPeriodicRetryDelay periodicRetryDelay; - @Mock private PeriodicTaskExecutor executor; @Mock private RequestService.Callback callback; @Mock private Supplier requestSupplier; + private TestHttpSender requestSender; + private final PeriodicDelay periodicRequestDelay = + PeriodicDelay.ofFixedDuration(Duration.ofSeconds(1)); + private TestPeriodicRetryDelay periodicRetryDelay; private int requestSize = -1; private HttpRequestService httpRequestService; @BeforeEach void setUp() { + requestSender = new TestHttpSender(); + periodicRetryDelay = new TestPeriodicRetryDelay(Duration.ofSeconds(2)); httpRequestService = new HttpRequestService( requestSender, - executor, + PeriodicTaskExecutor.create(periodicRequestDelay), periodicRequestDelay, periodicRetryDelay, RetryAfterParser.getInstance()); - } - - @Test - void verifyStart() { httpRequestService.start(callback, requestSupplier); - - InOrder inOrder = inOrder(periodicRequestDelay, executor); - inOrder.verify(executor).start(httpRequestService); - - // Try starting it again: - try { - httpRequestService.start(callback, requestSupplier); - fail(); - } catch (IllegalStateException e) { - assertThat(e).hasMessage("RequestDispatcher is already running"); - } } - @Test - void verifyStop() { - httpRequestService.start(callback, requestSupplier); - httpRequestService.stop(); - - verify(executor).stop(); - - // Try stopping it again: - clearInvocations(executor); - httpRequestService.stop(); - verifyNoInteractions(executor); - } - - @Test - void verifyStop_whenNotStarted() { + @AfterEach + void tearDown() { + requestSender.close(); httpRequestService.stop(); - - verifyNoInteractions(executor, requestSender, periodicRequestDelay); } @Test @@ -126,9 +105,12 @@ void verifySendingRequest_happyPath() { prepareRequest(); enqueueResponse(httpResponse); - httpRequestService.run(); + httpRequestService.sendRequest(); - verify(requestSender).send(any(), eq(requestSize)); + requestSender.awaitForRequest(Duration.ofMillis(500)); + assertThat(requestSender.requests).hasSize(1); + assertThat(requestSender.requests.get(0).contentLength).isEqualTo(requestSize); + verify(callback).onConnectionSuccess(); verify(callback).onRequestSuccess(Response.create(serverToAgent)); } @@ -186,7 +168,6 @@ void verifySendingRequest_whenThereIsAGenericHttpError() { httpRequestService.run(); verifyRequestFailedCallback(500); - verifyNoInteractions(executor); } @Test @@ -200,7 +181,7 @@ void verifySendingRequest_whenThereIsATooManyRequestsError() { httpRequestService.run(); verifyRequestFailedCallback(429); - verify(executor).setPeriodicDelay(periodicRetryDelay); + // verify(executor).setPeriodicDelay(periodicRetryDelay); todo verify(periodicRetryDelay, never()).suggestDelay(any()); } @@ -216,7 +197,7 @@ void verifySendingRequest_whenThereIsATooManyRequestsError_withSuggestedDelay() httpRequestService.run(); verifyRequestFailedCallback(429); - verify(executor).setPeriodicDelay(periodicRetryDelay); + // verify(executor).setPeriodicDelay(periodicRetryDelay); todo verify(periodicRetryDelay).suggestDelay(Duration.ofSeconds(5)); } @@ -239,7 +220,7 @@ void verifySendingRequest_whenServerProvidesRetryInfo_usingTheProvidedInfo() { verify(callback).onRequestSuccess(Response.create(serverToAgent)); verify(periodicRetryDelay).suggestDelay(Duration.ofNanos(nanosecondsToWaitForRetry)); - verify(executor).setPeriodicDelay(periodicRetryDelay); + // verify(executor).setPeriodicDelay(periodicRetryDelay); todo } @Test @@ -258,7 +239,7 @@ void verifySendingRequest_whenServerIsUnavailable() { verify(callback).onRequestSuccess(Response.create(serverToAgent)); verify(periodicRetryDelay, never()).suggestDelay(any()); - verify(executor).setPeriodicDelay(periodicRetryDelay); + // verify(executor).setPeriodicDelay(periodicRetryDelay); todo } @Test @@ -272,7 +253,7 @@ void verifySendingRequest_whenThereIsAServiceUnavailableError() { httpRequestService.run(); verifyRequestFailedCallback(503); - verify(executor).setPeriodicDelay(periodicRetryDelay); + // verify(executor).setPeriodicDelay(periodicRetryDelay); todo verify(periodicRetryDelay, never()).suggestDelay(any()); } @@ -288,7 +269,7 @@ void verifySendingRequest_whenThereIsAServiceUnavailableError_withSuggestedDelay httpRequestService.run(); verifyRequestFailedCallback(503); - verify(executor).setPeriodicDelay(periodicRetryDelay); + // verify(executor).setPeriodicDelay(periodicRetryDelay); todo verify(periodicRetryDelay).suggestDelay(Duration.ofSeconds(2)); } @@ -303,7 +284,7 @@ private void verifyRequestFailedCallback(int errorCode) { void verifySendingRequest_duringRegularMode() { httpRequestService.sendRequest(); - verify(executor).executeNow(); + // verify(executor).executeNow(); todo } @Test @@ -312,7 +293,7 @@ void verifySendingRequest_duringRetryMode() { httpRequestService.sendRequest(); - verify(executor, never()).executeNow(); + // verify(executor, never()).executeNow(); todo } @Test @@ -325,7 +306,7 @@ void verifySuccessfulSendingRequest_duringRetryMode() { httpRequestService.run(); - verify(executor).setPeriodicDelay(periodicRequestDelay); + // verify(executor).setPeriodicDelay(periodicRequestDelay); todo } private void enableRetryMode() { @@ -339,8 +320,6 @@ private void enableRetryMode() { } private void prepareRequest() { - httpRequestService.start(callback, requestSupplier); - clearInvocations(executor); AgentToServer agentToServer = new AgentToServer.Builder().sequence_num(10).build(); requestSize = agentToServer.encodeByteString().size(); Request request = Request.create(agentToServer); @@ -348,8 +327,7 @@ private void prepareRequest() { } private void enqueueResponse(HttpSender.Response httpResponse) { - when(requestSender.send(any(), anyInt())) - .thenReturn(CompletableFuture.completedFuture(httpResponse)); + requestSender.enqueueResponse(httpResponse); } private static void attachServerToAgentMessage( @@ -360,16 +338,79 @@ private static void attachServerToAgentMessage( } private static class TestPeriodicRetryDelay implements PeriodicDelay, AcceptsDelaySuggestion { + private final Duration delay; + + private TestPeriodicRetryDelay(Duration delay) { + this.delay = delay; + } @Override public void suggestDelay(Duration delay) {} @Override public Duration getNextDelay() { - return null; + return delay; } @Override public void reset() {} } + + private static class TestHttpSender implements HttpSender, Closeable { + private final List requests = Collections.synchronizedList(new ArrayList<>()); + private final Queue responses = new ConcurrentLinkedQueue<>(); + private final AtomicInteger unexpectedRequests = new AtomicInteger(0); + private volatile CountDownLatch latch; + + @Override + public CompletableFuture send(BodyWriter writer, int contentLength) { + requests.add(new RequestParams(contentLength)); + HttpSender.Response response = null; + try { + response = responses.remove(); + if (latch != null) { + latch.countDown(); + } + } catch (NoSuchElementException e) { + unexpectedRequests.incrementAndGet(); + } + return CompletableFuture.completedFuture(response); + } + + public void enqueueResponse(HttpSender.Response response) { + responses.add(response); + } + + public void awaitForRequest(Duration timeout) { + if (latch != null) { + throw new IllegalStateException(); + } + latch = new CountDownLatch(1); + try { + if (!latch.await(timeout.toMillis(), TimeUnit.MILLISECONDS)) { + fail("No request received before timeout " + timeout); + } + } catch (InterruptedException e) { + throw new RuntimeException(e); + } finally { + latch = null; + } + } + + @Override + public void close() { + int count = unexpectedRequests.get(); + if (count > 0) { + fail("Unexpected requests count: " + count); + } + } + + private static class RequestParams { + public final int contentLength; + + private RequestParams(int contentLength) { + this.contentLength = contentLength; + } + } + } } From c272f970c4b5679794086a55e59aea4b8ef22b59 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 6 Jun 2025 16:22:19 +0200 Subject: [PATCH 31/38] Updating tests --- .../request/delay/PeriodicTaskExecutor.java | 3 +- .../request/service/HttpRequestService.java | 3 +- .../service/HttpRequestServiceTest.java | 402 ++++++++++-------- 3 files changed, 223 insertions(+), 185 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java index 6a6ee030b..fbb225068 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java @@ -30,7 +30,7 @@ public static PeriodicTaskExecutor create(PeriodicDelay initialPeriodicDelay) { initialPeriodicDelay); } - PeriodicTaskExecutor( + public PeriodicTaskExecutor( ScheduledExecutorService executorService, PeriodicDelay initialPeriodicDelay) { this.executorService = executorService; this.periodicDelay = initialPeriodicDelay; @@ -52,7 +52,6 @@ public void setPeriodicDelay(PeriodicDelay periodicDelay) { scheduledFuture.cancel(false); } this.periodicDelay = periodicDelay; - periodicDelay.reset(); scheduleNext(); } finally { delaySetLock.unlock(); diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 5cb5ff0cf..dfe6ae30c 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -102,13 +102,13 @@ public void start(Callback callback, Supplier requestSupplier) { public void stop() { if (isRunning.compareAndSet(true, false)) { hasStopped.set(true); - executor.executeNow(); executor.stop(); } } private void enableRetryMode(@Nullable Duration suggestedDelay) { if (retryModeEnabled.compareAndSet(false, true)) { + periodicRetryDelay.reset(); if (suggestedDelay != null && periodicRetryDelay instanceof AcceptsDelaySuggestion) { ((AcceptsDelaySuggestion) periodicRetryDelay).suggestDelay(suggestedDelay); } @@ -118,6 +118,7 @@ private void enableRetryMode(@Nullable Duration suggestedDelay) { private void disableRetryMode() { if (retryModeEnabled.compareAndSet(true, false)) { + periodicRequestDelay.reset(); executor.setPeriodicDelay(periodicRequestDelay); } } diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index 6ec18536b..88dac0d7b 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -8,12 +8,16 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyInt; -import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import io.opentelemetry.opamp.client.internal.connectivity.http.HttpErrorException; @@ -25,20 +29,19 @@ import io.opentelemetry.opamp.client.internal.request.delay.PeriodicTaskExecutor; import io.opentelemetry.opamp.client.internal.response.Response; import java.io.ByteArrayInputStream; -import java.io.Closeable; import java.time.Duration; import java.util.ArrayList; import java.util.Collections; +import java.util.LinkedList; import java.util.List; import java.util.NoSuchElementException; import java.util.Queue; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.ConcurrentLinkedQueue; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; import opamp.proto.AgentToServer; import opamp.proto.RetryInfo; @@ -56,154 +59,122 @@ @SuppressWarnings("unchecked") @ExtendWith(MockitoExtension.class) class HttpRequestServiceTest { + + private static final Duration REGULAR_DELAY = Duration.ofSeconds(1); + private static final Duration RETRY_DELAY = Duration.ofSeconds(5); + @Mock private RequestService.Callback callback; @Mock private Supplier requestSupplier; + private final List scheduledTasks = new ArrayList<>(); + private ScheduledExecutorService executorService; private TestHttpSender requestSender; - private final PeriodicDelay periodicRequestDelay = - PeriodicDelay.ofFixedDuration(Duration.ofSeconds(1)); - private TestPeriodicRetryDelay periodicRetryDelay; + private PeriodicDelay periodicRequestDelay; + private PeriodicDelayWithSuggestion periodicRetryDelay; private int requestSize = -1; private HttpRequestService httpRequestService; @BeforeEach void setUp() { requestSender = new TestHttpSender(); - periodicRetryDelay = new TestPeriodicRetryDelay(Duration.ofSeconds(2)); + periodicRequestDelay = createPeriodicDelay(REGULAR_DELAY); + periodicRetryDelay = createPeriodicDelayWithSuggestionSupport(RETRY_DELAY); + executorService = createTestScheduleExecutorService(); httpRequestService = new HttpRequestService( requestSender, - PeriodicTaskExecutor.create(periodicRequestDelay), + new PeriodicTaskExecutor(executorService, periodicRequestDelay), periodicRequestDelay, periodicRetryDelay, RetryAfterParser.getInstance()); httpRequestService.start(callback, requestSupplier); + prepareRequest(); } @AfterEach void tearDown() { - requestSender.close(); - httpRequestService.stop(); - } - - @Test - void whenTryingToStartAfterStopHasBeenCalled_throwException() { - httpRequestService.start(callback, requestSupplier); httpRequestService.stop(); - try { - httpRequestService.start(callback, requestSupplier); - fail(); - } catch (IllegalStateException e) { - assertThat(e).hasMessage("HttpRequestService cannot start after it has been stopped."); - } + scheduledTasks.clear(); + verify(executorService).shutdown(); + verifyNoMoreInteractions(executorService); } @Test void verifySendingRequest_happyPath() { - HttpSender.Response httpResponse = mock(); ServerToAgent serverToAgent = new ServerToAgent.Builder().build(); - attachServerToAgentMessage(serverToAgent.encodeByteString().toByteArray(), httpResponse); - prepareRequest(); - enqueueResponse(httpResponse); + HttpSender.Response httpResponse = createSuccessfulResponse(serverToAgent); + requestSender.enqueueResponse(httpResponse); httpRequestService.sendRequest(); - requestSender.awaitForRequest(Duration.ofMillis(500)); - assertThat(requestSender.requests).hasSize(1); - assertThat(requestSender.requests.get(0).contentLength).isEqualTo(requestSize); + verifySingleRequestSent(); + verifyRequestSuccessCallback(serverToAgent); verify(callback).onConnectionSuccess(); - verify(callback).onRequestSuccess(Response.create(serverToAgent)); } @Test void verifySendingRequest_whenTheresAParsingError() { - HttpSender.Response httpResponse = mock(); - attachServerToAgentMessage(new byte[] {1, 2, 3}, httpResponse); - prepareRequest(); - enqueueResponse(httpResponse); + HttpSender.Response httpResponse = createSuccessfulResponse(new byte[] {1, 2, 3}); + requestSender.enqueueResponse(httpResponse); - httpRequestService.run(); + httpRequestService.sendRequest(); - verify(requestSender).send(any(), eq(requestSize)); + verifySingleRequestSent(); verify(callback).onConnectionFailed(any()); } @Test void verifySendingRequest_whenThereIsAnExecutionError() throws ExecutionException, InterruptedException, TimeoutException { - prepareRequest(); CompletableFuture future = mock(); - when(requestSender.send(any(), anyInt())).thenReturn(future); + requestSender.enqueueResponseFuture(future); Exception myException = mock(); doThrow(new ExecutionException(myException)).when(future).get(30, TimeUnit.SECONDS); - httpRequestService.run(); + httpRequestService.sendRequest(); - verify(requestSender).send(any(), eq(requestSize)); + verifySingleRequestSent(); verify(callback).onConnectionFailed(myException); } @Test void verifySendingRequest_whenThereIsAnInterruptedException() throws ExecutionException, InterruptedException, TimeoutException { - prepareRequest(); CompletableFuture future = mock(); - when(requestSender.send(any(), anyInt())).thenReturn(future); + requestSender.enqueueResponseFuture(future); InterruptedException myException = mock(); doThrow(myException).when(future).get(30, TimeUnit.SECONDS); - httpRequestService.run(); + httpRequestService.sendRequest(); - verify(requestSender).send(any(), eq(requestSize)); + verifySingleRequestSent(); verify(callback).onConnectionFailed(myException); } @Test void verifySendingRequest_whenThereIsAGenericHttpError() { - HttpSender.Response response = mock(); - when(response.statusCode()).thenReturn(500); - when(response.statusMessage()).thenReturn("Error message"); - prepareRequest(); - enqueueResponse(response); + requestSender.enqueueResponse(createFailedResponse(500)); - httpRequestService.run(); + httpRequestService.sendRequest(); + verifySingleRequestSent(); verifyRequestFailedCallback(500); } @Test void verifySendingRequest_whenThereIsATooManyRequestsError() { - HttpSender.Response response = mock(); - when(response.statusCode()).thenReturn(429); - when(response.statusMessage()).thenReturn("Error message"); - prepareRequest(); - enqueueResponse(response); - - httpRequestService.run(); - - verifyRequestFailedCallback(429); - // verify(executor).setPeriodicDelay(periodicRetryDelay); todo - verify(periodicRetryDelay, never()).suggestDelay(any()); + verifyRetryDelayOnError(createFailedResponse(429), RETRY_DELAY); } @Test void verifySendingRequest_whenThereIsATooManyRequestsError_withSuggestedDelay() { - HttpSender.Response response = mock(); - when(response.statusCode()).thenReturn(429); - when(response.statusMessage()).thenReturn("Error message"); + HttpSender.Response response = createFailedResponse(429); when(response.getHeader("Retry-After")).thenReturn("5"); - prepareRequest(); - enqueueResponse(response); - - httpRequestService.run(); - verifyRequestFailedCallback(429); - // verify(executor).setPeriodicDelay(periodicRetryDelay); todo - verify(periodicRetryDelay).suggestDelay(Duration.ofSeconds(5)); + verifyRetryDelayOnError(response, Duration.ofSeconds(5)); } @Test - void verifySendingRequest_whenServerProvidesRetryInfo_usingTheProvidedInfo() { - HttpSender.Response response = mock(); + void verifySendingRequest_whenServerProvidesRetryInfo() { long nanosecondsToWaitForRetry = 1000; ServerErrorResponse errorResponse = new ServerErrorResponse.Builder() @@ -212,111 +183,103 @@ void verifySendingRequest_whenServerProvidesRetryInfo_usingTheProvidedInfo() { new RetryInfo.Builder().retry_after_nanoseconds(nanosecondsToWaitForRetry).build()) .build(); ServerToAgent serverToAgent = new ServerToAgent.Builder().error_response(errorResponse).build(); - attachServerToAgentMessage(serverToAgent.encodeByteString().toByteArray(), response); - prepareRequest(); - enqueueResponse(response); - - httpRequestService.run(); + HttpSender.Response response = createSuccessfulResponse(serverToAgent); - verify(callback).onRequestSuccess(Response.create(serverToAgent)); - verify(periodicRetryDelay).suggestDelay(Duration.ofNanos(nanosecondsToWaitForRetry)); - // verify(executor).setPeriodicDelay(periodicRetryDelay); todo + verifyRetryDelayOnError(response, Duration.ofNanos(nanosecondsToWaitForRetry)); + verify(callback).onRequestFailed(any()); + verify(callback, never()).onRequestSuccess(any()); } @Test void verifySendingRequest_whenServerIsUnavailable() { - HttpSender.Response response = mock(); ServerErrorResponse errorResponse = new ServerErrorResponse.Builder() .type(ServerErrorResponseType.ServerErrorResponseType_Unavailable) .build(); ServerToAgent serverToAgent = new ServerToAgent.Builder().error_response(errorResponse).build(); - attachServerToAgentMessage(serverToAgent.encodeByteString().toByteArray(), response); - prepareRequest(); - enqueueResponse(response); - - httpRequestService.run(); + HttpSender.Response response = createSuccessfulResponse(serverToAgent); - verify(callback).onRequestSuccess(Response.create(serverToAgent)); - verify(periodicRetryDelay, never()).suggestDelay(any()); - // verify(executor).setPeriodicDelay(periodicRetryDelay); todo + verifyRetryDelayOnError(response, RETRY_DELAY); + verify(callback).onRequestFailed(any()); + verify(callback, never()).onRequestSuccess(any()); } @Test void verifySendingRequest_whenThereIsAServiceUnavailableError() { - HttpSender.Response response = mock(); - when(response.statusCode()).thenReturn(503); - when(response.statusMessage()).thenReturn("Error message"); - prepareRequest(); - enqueueResponse(response); - - httpRequestService.run(); - - verifyRequestFailedCallback(503); - // verify(executor).setPeriodicDelay(periodicRetryDelay); todo - verify(periodicRetryDelay, never()).suggestDelay(any()); + verifyRetryDelayOnError(createFailedResponse(503), RETRY_DELAY); } @Test void verifySendingRequest_whenThereIsAServiceUnavailableError_withSuggestedDelay() { - HttpSender.Response response = mock(); + HttpSender.Response response = createFailedResponse(503); when(response.getHeader("Retry-After")).thenReturn("2"); - when(response.statusCode()).thenReturn(503); - when(response.statusMessage()).thenReturn("Error message"); - prepareRequest(); - enqueueResponse(response); - - httpRequestService.run(); - verifyRequestFailedCallback(503); - // verify(executor).setPeriodicDelay(periodicRetryDelay); todo - verify(periodicRetryDelay).suggestDelay(Duration.ofSeconds(2)); - } - - private void verifyRequestFailedCallback(int errorCode) { - ArgumentCaptor captor = ArgumentCaptor.forClass(HttpErrorException.class); - verify(callback).onRequestFailed(captor.capture()); - assertThat(captor.getValue().getErrorCode()).isEqualTo(errorCode); - assertThat(captor.getValue().getMessage()).isEqualTo("Error message"); + verifyRetryDelayOnError(response, Duration.ofSeconds(2)); } @Test void verifySendingRequest_duringRegularMode() { + requestSender.enqueueResponse(createSuccessfulResponse(new ServerToAgent.Builder().build())); + httpRequestService.sendRequest(); - // verify(executor).executeNow(); todo + verifySingleRequestSent(); } @Test void verifySendingRequest_duringRetryMode() { enableRetryMode(); + requestSender.requests.clear(); httpRequestService.sendRequest(); - // verify(executor, never()).executeNow(); todo + assertThat(requestSender.requests).isEmpty(); } - @Test - void verifySuccessfulSendingRequest_duringRetryMode() { - enableRetryMode(); - HttpSender.Response response = mock(); - attachServerToAgentMessage( - new ServerToAgent.Builder().build().encodeByteString().toByteArray(), response); - enqueueResponse(response); + private void verifyRetryDelayOnError( + HttpSender.Response errorResponse, Duration expectedRetryDelay) { + requestSender.enqueueResponse(errorResponse); + ScheduledTask previousTask = getCurrentScheduledTask(); + scheduledTasks.clear(); - httpRequestService.run(); + httpRequestService.sendRequest(); - // verify(executor).setPeriodicDelay(periodicRequestDelay); todo + verifySingleRequestSent(); + verify(previousTask.future).cancel(false); + verify(periodicRetryDelay).reset(); + ScheduledTask retryTask = getCurrentScheduledTask(); + assertThat(retryTask.delay).isEqualTo(expectedRetryDelay); + + // Retry with another error + clearInvocations(callback); + scheduledTasks.clear(); + requestSender.enqueueResponse(createFailedResponse(500)); + retryTask.runnable.run(); + + verify(retryTask.future, never()).cancel(anyBoolean()); + verifySingleRequestSent(); + ScheduledTask retryTask2 = getCurrentScheduledTask(); + assertThat(retryTask2.delay).isEqualTo(expectedRetryDelay); + + // Retry with a success + clearInvocations(callback); + scheduledTasks.clear(); + ServerToAgent serverToAgent = new ServerToAgent.Builder().build(); + requestSender.enqueueResponse(createSuccessfulResponse(serverToAgent)); + retryTask2.runnable.run(); + + verify(retryTask2.future).cancel(false); + verify(periodicRequestDelay).reset(); + verifySingleRequestSent(); + verifyRequestSuccessCallback(serverToAgent); + assertThat(getCurrentScheduledTask().delay).isEqualTo(REGULAR_DELAY); } private void enableRetryMode() { - HttpSender.Response response = mock(); - when(response.statusCode()).thenReturn(503); - when(response.statusMessage()).thenReturn("Error message"); - prepareRequest(); - enqueueResponse(response); + HttpSender.Response response = createFailedResponse(503); + requestSender.enqueueResponse(response); - httpRequestService.run(); + httpRequestService.sendRequest(); } private void prepareRequest() { @@ -326,83 +289,140 @@ private void prepareRequest() { when(requestSupplier.get()).thenReturn(request); } - private void enqueueResponse(HttpSender.Response httpResponse) { - requestSender.enqueueResponse(httpResponse); + private ScheduledTask getCurrentScheduledTask() { + assertThat(scheduledTasks).hasSize(1); + return scheduledTasks.get(0); + } + + private void verifySingleRequestSent() { + List requests = requestSender.getRequests(1); + assertThat(requests.get(0).contentLength).isEqualTo(requestSize); + } + + private void verifyRequestSuccessCallback(ServerToAgent serverToAgent) { + verify(callback).onRequestSuccess(Response.create(serverToAgent)); + } + + private void verifyRequestFailedCallback(int errorCode) { + ArgumentCaptor captor = ArgumentCaptor.forClass(HttpErrorException.class); + verify(callback).onRequestFailed(captor.capture()); + assertThat(captor.getValue().getErrorCode()).isEqualTo(errorCode); + assertThat(captor.getValue().getMessage()).isEqualTo("Error message"); + } + + private ScheduledExecutorService createTestScheduleExecutorService() { + ScheduledExecutorService service = mock(); + + doAnswer( + invocation -> { + Runnable runnable = invocation.getArgument(0); + runnable.run(); + return null; + }) + .when(service) + .execute(any()); + + when(service.schedule(any(Runnable.class), anyLong(), any(TimeUnit.class))) + .thenAnswer( + invocation -> { + ScheduledFuture future = mock(); + scheduledTasks.add( + ScheduledTask.create( + future, invocation.getArgument(0), invocation.getArgument(1))); + return future; + }); + + return service; + } + + private static HttpSender.Response createSuccessfulResponse(ServerToAgent serverToAgent) { + return createSuccessfulResponse(serverToAgent.encodeByteString().toByteArray()); } - private static void attachServerToAgentMessage( - byte[] serverToAgent, HttpSender.Response httpResponse) { + private static HttpSender.Response createSuccessfulResponse(byte[] serverToAgent) { + HttpSender.Response response = mock(); ByteArrayInputStream byteArrayInputStream = new ByteArrayInputStream(serverToAgent); - when(httpResponse.statusCode()).thenReturn(200); - when(httpResponse.bodyInputStream()).thenReturn(byteArrayInputStream); + when(response.statusCode()).thenReturn(200); + when(response.bodyInputStream()).thenReturn(byteArrayInputStream); + return response; } - private static class TestPeriodicRetryDelay implements PeriodicDelay, AcceptsDelaySuggestion { - private final Duration delay; + private static HttpSender.Response createFailedResponse(int statusCode) { + HttpSender.Response response = mock(); + when(response.statusCode()).thenReturn(statusCode); + when(response.statusMessage()).thenReturn("Error message"); + return response; + } - private TestPeriodicRetryDelay(Duration delay) { - this.delay = delay; + private static PeriodicDelay createPeriodicDelay(Duration delay) { + PeriodicDelay mock = mock(); + when(mock.getNextDelay()).thenReturn(delay); + return mock; + } + + private static PeriodicDelayWithSuggestion createPeriodicDelayWithSuggestionSupport( + Duration delay) { + return spy(new PeriodicDelayWithSuggestion(delay)); + } + + private static class PeriodicDelayWithSuggestion + implements PeriodicDelay, AcceptsDelaySuggestion { + private final Duration initialDelay; + private Duration currentDelay; + + private PeriodicDelayWithSuggestion(Duration initialDelay) { + this.initialDelay = initialDelay; + currentDelay = initialDelay; } @Override - public void suggestDelay(Duration delay) {} + public void suggestDelay(Duration delay) { + currentDelay = delay; + } @Override public Duration getNextDelay() { - return delay; + return currentDelay; } @Override - public void reset() {} + public void reset() { + currentDelay = initialDelay; + } } - private static class TestHttpSender implements HttpSender, Closeable { - private final List requests = Collections.synchronizedList(new ArrayList<>()); - private final Queue responses = new ConcurrentLinkedQueue<>(); - private final AtomicInteger unexpectedRequests = new AtomicInteger(0); - private volatile CountDownLatch latch; + private static class TestHttpSender implements HttpSender { + private final List requests = new ArrayList<>(); + + @SuppressWarnings("JdkObsolete") + private final Queue> responses = new LinkedList<>(); @Override public CompletableFuture send(BodyWriter writer, int contentLength) { requests.add(new RequestParams(contentLength)); - HttpSender.Response response = null; + CompletableFuture response = null; try { response = responses.remove(); - if (latch != null) { - latch.countDown(); - } } catch (NoSuchElementException e) { - unexpectedRequests.incrementAndGet(); + fail("Unwanted triggered request"); } - return CompletableFuture.completedFuture(response); + return response; } public void enqueueResponse(HttpSender.Response response) { - responses.add(response); + enqueueResponseFuture(CompletableFuture.completedFuture(response)); } - public void awaitForRequest(Duration timeout) { - if (latch != null) { - throw new IllegalStateException(); - } - latch = new CountDownLatch(1); - try { - if (!latch.await(timeout.toMillis(), TimeUnit.MILLISECONDS)) { - fail("No request received before timeout " + timeout); - } - } catch (InterruptedException e) { - throw new RuntimeException(e); - } finally { - latch = null; - } + public void enqueueResponseFuture(CompletableFuture future) { + responses.add(future); } - @Override - public void close() { - int count = unexpectedRequests.get(); - if (count > 0) { - fail("Unexpected requests count: " + count); - } + public List getRequests(int size) { + assertThat(requests).hasSize(size); + List immutableRequests = + Collections.unmodifiableList(new ArrayList<>(requests)); + requests.clear(); + return immutableRequests; } private static class RequestParams { @@ -413,4 +433,22 @@ private RequestParams(int contentLength) { } } } + + @SuppressWarnings("UnusedVariable") + private static class ScheduledTask { + private final ScheduledFuture future; + private final Runnable runnable; + private final Duration delay; + + private static ScheduledTask create( + ScheduledFuture future, Runnable runnable, long delayNanos) { + return new ScheduledTask(future, runnable, Duration.ofNanos(delayNanos)); + } + + private ScheduledTask(ScheduledFuture future, Runnable runnable, Duration delay) { + this.future = future; + this.runnable = runnable; + this.delay = delay; + } + } } From b709a8107acca95dfb06f6bb368a618cbb8ae91b Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 6 Jun 2025 16:38:55 +0200 Subject: [PATCH 32/38] Adding initial task validation --- .../service/HttpRequestServiceTest.java | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index 88dac0d7b..24e31b790 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -64,7 +64,6 @@ class HttpRequestServiceTest { private static final Duration RETRY_DELAY = Duration.ofSeconds(5); @Mock private RequestService.Callback callback; - @Mock private Supplier requestSupplier; private final List scheduledTasks = new ArrayList<>(); private ScheduledExecutorService executorService; private TestHttpSender requestSender; @@ -86,8 +85,7 @@ void setUp() { periodicRequestDelay, periodicRetryDelay, RetryAfterParser.getInstance()); - httpRequestService.start(callback, requestSupplier); - prepareRequest(); + httpRequestService.start(callback, createRequestSupplier()); } @AfterEach @@ -98,6 +96,26 @@ void tearDown() { verifyNoMoreInteractions(executorService); } + @Test + void verifyStart_scheduledFirstTask() { + assertThat(scheduledTasks).hasSize(1); + ScheduledTask firstTask = scheduledTasks.get(0); + assertThat(firstTask.delay).isEqualTo(REGULAR_DELAY); + + // Verify initial task creates next one + scheduledTasks.clear(); + requestSender.enqueueResponse(createSuccessfulResponse(new ServerToAgent.Builder().build())); + firstTask.runnable.run(); + + assertThat(scheduledTasks).hasSize(1); + + // Check on-demand requests don't create subsequent tasks + requestSender.enqueueResponse(createSuccessfulResponse(new ServerToAgent.Builder().build())); + httpRequestService.sendRequest(); + + assertThat(scheduledTasks).hasSize(1); + } + @Test void verifySendingRequest_happyPath() { ServerToAgent serverToAgent = new ServerToAgent.Builder().build(); @@ -282,11 +300,11 @@ private void enableRetryMode() { httpRequestService.sendRequest(); } - private void prepareRequest() { + private Supplier createRequestSupplier() { AgentToServer agentToServer = new AgentToServer.Builder().sequence_num(10).build(); requestSize = agentToServer.encodeByteString().size(); Request request = Request.create(agentToServer); - when(requestSupplier.get()).thenReturn(request); + return () -> request; } private ScheduledTask getCurrentScheduledTask() { @@ -434,7 +452,6 @@ private RequestParams(int contentLength) { } } - @SuppressWarnings("UnusedVariable") private static class ScheduledTask { private final ScheduledFuture future; private final Runnable runnable; From 7b214199feb14e2b1919ca779131ba82048c13e7 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 6 Jun 2025 18:37:18 +0200 Subject: [PATCH 33/38] Simplifying HttpRequestService as suggested in the PR reviews --- .../request/delay/PeriodicTaskExecutor.java | 98 ---------- .../request/service/HttpRequestService.java | 122 ++++++++---- .../response/OpampServerResponseError.java | 14 ++ .../delay/PeriodicTaskExecutorTest.java | 173 ------------------ .../service/HttpRequestServiceTest.java | 35 +--- 5 files changed, 104 insertions(+), 338 deletions(-) delete mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java create mode 100644 opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/OpampServerResponseError.java delete mode 100644 opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java deleted file mode 100644 index fbb225068..000000000 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutor.java +++ /dev/null @@ -1,98 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.opamp.client.internal.request.delay; - -import java.util.Objects; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.ScheduledFuture; -import java.util.concurrent.ThreadFactory; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReference; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; -import javax.annotation.Nonnull; -import javax.annotation.Nullable; - -public final class PeriodicTaskExecutor { - private final ScheduledExecutorService executorService; - private final Lock delaySetLock = new ReentrantLock(); - private final AtomicReference periodicTask = new AtomicReference<>(); - private PeriodicDelay periodicDelay; - @Nullable private ScheduledFuture scheduledFuture; - - public static PeriodicTaskExecutor create(PeriodicDelay initialPeriodicDelay) { - return new PeriodicTaskExecutor( - Executors.newSingleThreadScheduledExecutor(new DaemonThreadFactory()), - initialPeriodicDelay); - } - - public PeriodicTaskExecutor( - ScheduledExecutorService executorService, PeriodicDelay initialPeriodicDelay) { - this.executorService = executorService; - this.periodicDelay = initialPeriodicDelay; - } - - public void start(@Nonnull Runnable periodicTask) { - this.periodicTask.set(periodicTask); - scheduleNext(); - } - - public void executeNow() { - executorService.execute(periodicTask.get()); - } - - public void setPeriodicDelay(PeriodicDelay periodicDelay) { - delaySetLock.lock(); - try { - if (scheduledFuture != null) { - scheduledFuture.cancel(false); - } - this.periodicDelay = periodicDelay; - scheduleNext(); - } finally { - delaySetLock.unlock(); - } - } - - public void stop() { - executorService.shutdown(); - } - - private void scheduleNext() { - delaySetLock.lock(); - try { - scheduledFuture = - executorService.schedule( - new PeriodicRunner(), periodicDelay.getNextDelay().toNanos(), TimeUnit.NANOSECONDS); - } finally { - delaySetLock.unlock(); - } - } - - private class PeriodicRunner implements Runnable { - @Override - public void run() { - Objects.requireNonNull(periodicTask.get()).run(); - scheduleNext(); - } - } - - private static class DaemonThreadFactory implements ThreadFactory { - private final ThreadFactory delegate = Executors.defaultThreadFactory(); - - @Override - public Thread newThread(@Nonnull Runnable r) { - Thread t = delegate.newThread(r); - try { - t.setDaemon(true); - } catch (SecurityException e) { - // Well, we tried. - } - return t; - } - } -} diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index dfe6ae30c..6082572a7 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -11,7 +11,7 @@ import io.opentelemetry.opamp.client.internal.request.Request; import io.opentelemetry.opamp.client.internal.request.delay.AcceptsDelaySuggestion; import io.opentelemetry.opamp.client.internal.request.delay.PeriodicDelay; -import io.opentelemetry.opamp.client.internal.request.delay.PeriodicTaskExecutor; +import io.opentelemetry.opamp.client.internal.response.OpampServerResponseError; import io.opentelemetry.opamp.client.internal.response.Response; import java.io.IOException; import java.time.Duration; @@ -19,24 +19,31 @@ import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Supplier; +import javax.annotation.Nonnull; import javax.annotation.Nullable; import opamp.proto.AgentToServer; import opamp.proto.ServerErrorResponse; import opamp.proto.ServerErrorResponseType; import opamp.proto.ServerToAgent; -public final class HttpRequestService implements RequestService, Runnable { +public final class HttpRequestService implements RequestService { private final HttpSender requestSender; - private final PeriodicTaskExecutor executor; + private final ScheduledExecutorService executorService; private final PeriodicDelay periodicRequestDelay; private final PeriodicDelay periodicRetryDelay; - private final AtomicBoolean retryModeEnabled = new AtomicBoolean(false); private final AtomicBoolean isRunning = new AtomicBoolean(false); private final AtomicBoolean hasStopped = new AtomicBoolean(false); + private final AtomicReference currentDelay; + private final AtomicReference> currentTask = new AtomicReference<>(); private final RetryAfterParser retryAfterParser; @Nullable private Callback callback; @Nullable private Supplier requestSupplier; @@ -65,7 +72,7 @@ public static HttpRequestService create( PeriodicDelay periodicRetryDelay) { return new HttpRequestService( requestSender, - PeriodicTaskExecutor.create(periodicRequestDelay), + Executors.newSingleThreadScheduledExecutor(new DaemonThreadFactory()), periodicRequestDelay, periodicRetryDelay, RetryAfterParser.getInstance()); @@ -73,15 +80,16 @@ public static HttpRequestService create( HttpRequestService( HttpSender requestSender, - PeriodicTaskExecutor executor, + ScheduledExecutorService executorService, PeriodicDelay periodicRequestDelay, PeriodicDelay periodicRetryDelay, RetryAfterParser retryAfterParser) { this.requestSender = requestSender; - this.executor = executor; + this.executorService = executorService; this.periodicRequestDelay = periodicRequestDelay; this.periodicRetryDelay = periodicRetryDelay; this.retryAfterParser = retryAfterParser; + currentDelay = new AtomicReference<>(periodicRequestDelay); } @Override @@ -92,47 +100,45 @@ public void start(Callback callback, Supplier requestSupplier) { if (isRunning.compareAndSet(false, true)) { this.callback = callback; this.requestSupplier = requestSupplier; - executor.start(this); + currentTask.set( + executorService.schedule( + this::periodicSend, getNextDelay().toNanos(), TimeUnit.NANOSECONDS)); } else { throw new IllegalStateException("HttpRequestService is already running"); } } - @Override - public void stop() { - if (isRunning.compareAndSet(true, false)) { - hasStopped.set(true); - executor.stop(); - } + private void periodicSend() { + doSendRequest(); + // schedule the next execution + currentTask.set( + executorService.schedule( + this::periodicSend, getNextDelay().toNanos(), TimeUnit.NANOSECONDS)); } - private void enableRetryMode(@Nullable Duration suggestedDelay) { - if (retryModeEnabled.compareAndSet(false, true)) { - periodicRetryDelay.reset(); - if (suggestedDelay != null && periodicRetryDelay instanceof AcceptsDelaySuggestion) { - ((AcceptsDelaySuggestion) periodicRetryDelay).suggestDelay(suggestedDelay); - } - executor.setPeriodicDelay(periodicRetryDelay); - } + private void sendOnce() { + executorService.execute(this::doSendRequest); } - private void disableRetryMode() { - if (retryModeEnabled.compareAndSet(true, false)) { - periodicRequestDelay.reset(); - executor.setPeriodicDelay(periodicRequestDelay); - } + private Duration getNextDelay() { + return Objects.requireNonNull(currentDelay.get()).getNextDelay(); } @Override - public void sendRequest() { - if (!retryModeEnabled.get()) { - executor.executeNow(); + public void stop() { + if (isRunning.compareAndSet(true, false)) { + hasStopped.set(true); + executorService.shutdown(); } } @Override - public void run() { - doSendRequest(); + public void sendRequest() { + if (!isRunning.get()) { + throw new IllegalStateException("HttpRequestService is not running"); + } + + sendOnce(); } private void doSendRequest() { @@ -173,7 +179,7 @@ private void handleHttpError(HttpSender.Response response) { retryAfter = duration.get(); } } - enableRetryMode(retryAfter); + useRetryDelay(retryAfter); } } @@ -182,16 +188,14 @@ private static boolean isSuccessful(HttpSender.Response response) { } private void handleSuccessResponse(Response response) { - if (retryModeEnabled.get()) { - disableRetryMode(); - } + useRegularDelay(); ServerToAgent serverToAgent = response.getServerToAgent(); if (serverToAgent.error_response != null) { handleErrorResponse(serverToAgent.error_response); + } else { + getCallback().onRequestSuccess(response); } - - getCallback().onRequestSuccess(response); } private void handleErrorResponse(ServerErrorResponse errorResponse) { @@ -200,11 +204,51 @@ private void handleErrorResponse(ServerErrorResponse errorResponse) { if (errorResponse.retry_info != null) { retryAfter = Duration.ofNanos(errorResponse.retry_info.retry_after_nanoseconds); } - enableRetryMode(retryAfter); + useRetryDelay(retryAfter); + } + getCallback().onRequestFailed(new OpampServerResponseError(errorResponse.error_message)); + } + + private void useRegularDelay() { + if (currentDelay.compareAndSet(periodicRetryDelay, periodicRequestDelay)) { + cancelCurrentTask(); + periodicRequestDelay.reset(); + } + } + + private void useRetryDelay(@Nullable Duration retryAfter) { + if (currentDelay.compareAndSet(periodicRequestDelay, periodicRetryDelay)) { + cancelCurrentTask(); + periodicRetryDelay.reset(); + if (retryAfter != null && periodicRetryDelay instanceof AcceptsDelaySuggestion) { + ((AcceptsDelaySuggestion) periodicRetryDelay).suggestDelay(retryAfter); + } + } + } + + private void cancelCurrentTask() { + ScheduledFuture future = currentTask.get(); + if (future != null) { + future.cancel(false); } } private Callback getCallback() { return Objects.requireNonNull(callback); } + + private static class DaemonThreadFactory implements ThreadFactory { + private final ThreadFactory delegate = Executors.defaultThreadFactory(); + + @Override + public Thread newThread(@Nonnull Runnable r) { + Thread t = delegate.newThread(r); + try { + t.setDaemon(true); + } catch (SecurityException e) { + // Well, we tried. + } + return t; + } + } } diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/OpampServerResponseError.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/OpampServerResponseError.java new file mode 100644 index 000000000..d88d194a7 --- /dev/null +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/OpampServerResponseError.java @@ -0,0 +1,14 @@ +package io.opentelemetry.opamp.client.internal.response; + +public class OpampServerResponseError extends Exception { + private static final long serialVersionUID = 1L; + + /** + * Constructs an OpAMP error related exception. + * + * @param message The OpAMP error message. + */ + public OpampServerResponseError(String message) { + super(message); + } +} diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java deleted file mode 100644 index 207e3af63..000000000 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/delay/PeriodicTaskExecutorTest.java +++ /dev/null @@ -1,173 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.opamp.client.internal.request.delay; - -import static org.junit.jupiter.api.Assertions.fail; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.inOrder; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - -import java.time.Duration; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.ScheduledFuture; -import java.util.concurrent.TimeUnit; -import org.junit.jupiter.api.Test; -import org.mockito.InOrder; - -@SuppressWarnings({"rawtypes", "unchecked"}) -class PeriodicTaskExecutorTest { - private static final int TIMEOUT_SECONDS = 5; - - @Test - void verifyRunAtStart() throws InterruptedException { - CountDownLatch latch = new CountDownLatch(1); - PeriodicDelay delay = new TestPeriodicDelay(Duration.ofSeconds(0)); - - PeriodicTaskExecutor.create(delay).start(latch::countDown); - - if (!latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)) { - fail("Timed out waiting"); - } - } - - @Test - void verifyRerunAfterFinishedRunning() throws InterruptedException { - CountDownLatch latch = new CountDownLatch(2); - PeriodicDelay delay = new TestPeriodicDelay(Duration.ofMillis(500)); - - PeriodicTaskExecutor.create(delay).start(latch::countDown); - - if (!latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)) { - fail("Timed out waiting"); - } - } - - @Test - void verifyRunningImmediatelyWhenRequested() { - PeriodicDelay delay = new TestPeriodicDelay(Duration.ofMinutes(1)); - ScheduledExecutorService executorService = mock(); - Runnable task = mock(Runnable.class); - PeriodicTaskExecutor executor = new PeriodicTaskExecutor(executorService, delay); - executor.start(task); - - executor.executeNow(); - - verify(executorService).execute(task); - } - - @Test - void verifyKeepingOriginalScheduleAfterRunningImmediately() throws InterruptedException { - CountDownLatch latch = new CountDownLatch(2); - PeriodicDelay delay = new TestPeriodicDelay(Duration.ofSeconds(1)); - - PeriodicTaskExecutor executor = PeriodicTaskExecutor.create(delay); - executor.start(latch::countDown); - - executor.executeNow(); - - if (!latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)) { - fail("Timed out waiting"); - } - } - - @Test - void verifyDelayValueUsed() { - Duration duration = Duration.ofSeconds(1); - PeriodicDelay delay = new TestPeriodicDelay(duration); - ScheduledExecutorService executorService = mock(); - - PeriodicTaskExecutor executor = new PeriodicTaskExecutor(executorService, delay); - - executor.start(() -> {}); - - verify(executorService) - .schedule(any(Runnable.class), eq(duration.toNanos()), eq(TimeUnit.NANOSECONDS)); - } - - @Test - void verifyDelayChange() throws InterruptedException { - CountDownLatch latch = new CountDownLatch(1); - PeriodicDelay delay = new TestPeriodicDelay(Duration.ofMinutes(1)); - - PeriodicTaskExecutor executor = PeriodicTaskExecutor.create(delay); - executor.start(latch::countDown); - - executor.setPeriodicDelay(new TestPeriodicDelay(Duration.ofSeconds(1))); - - if (!latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS)) { - fail("Timed out waiting"); - } - } - - @Test - void verifyPreviousTaskCancelledWhenDelayChanges() { - ScheduledExecutorService executorService = mock(); - ScheduledFuture firstTaskFuture = mock(ScheduledFuture.class); - when(executorService.schedule(any(Runnable.class), anyLong(), any())) - .thenReturn(firstTaskFuture); - - PeriodicTaskExecutor executor = - new PeriodicTaskExecutor(executorService, new TestPeriodicDelay(Duration.ofSeconds(1))); - - executor.start(() -> {}); - - executor.setPeriodicDelay(new TestPeriodicDelay(Duration.ofSeconds(2))); - - verify(firstTaskFuture).cancel(false); - } - - @Test - void verifyNewPeriodicDelayGetsResetBeforeBeingUsed() { - ScheduledExecutorService executorService = mock(); - ScheduledFuture firstTaskFuture = mock(ScheduledFuture.class); - when(executorService.schedule(any(Runnable.class), anyLong(), any())) - .thenReturn(firstTaskFuture); - PeriodicDelay delayChange = mock(); - - PeriodicTaskExecutor executor = - new PeriodicTaskExecutor(executorService, new TestPeriodicDelay(Duration.ofSeconds(1))); - - executor.start(() -> {}); - - executor.setPeriodicDelay(delayChange); - - InOrder inOrder = inOrder(delayChange); - inOrder.verify(delayChange).reset(); - inOrder.verify(delayChange).getNextDelay(); - } - - @Test - void verifyStop() { - ScheduledExecutorService executorService = mock(); - - PeriodicTaskExecutor executor = - new PeriodicTaskExecutor(executorService, new TestPeriodicDelay(Duration.ofSeconds(1))); - executor.stop(); - - verify(executorService).shutdown(); - } - - private static class TestPeriodicDelay implements PeriodicDelay { - private final Duration delay; - - private TestPeriodicDelay(Duration delay) { - this.delay = delay; - } - - @Override - public Duration getNextDelay() { - return delay; - } - - @Override - public void reset() {} - } -} diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index 24e31b790..e0a4bddd3 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -11,13 +11,12 @@ import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.clearInvocations; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import io.opentelemetry.opamp.client.internal.connectivity.http.HttpErrorException; @@ -26,7 +25,6 @@ import io.opentelemetry.opamp.client.internal.request.Request; import io.opentelemetry.opamp.client.internal.request.delay.AcceptsDelaySuggestion; import io.opentelemetry.opamp.client.internal.request.delay.PeriodicDelay; -import io.opentelemetry.opamp.client.internal.request.delay.PeriodicTaskExecutor; import io.opentelemetry.opamp.client.internal.response.Response; import java.io.ByteArrayInputStream; import java.time.Duration; @@ -81,7 +79,7 @@ void setUp() { httpRequestService = new HttpRequestService( requestSender, - new PeriodicTaskExecutor(executorService, periodicRequestDelay), + executorService, periodicRequestDelay, periodicRetryDelay, RetryAfterParser.getInstance()); @@ -93,7 +91,6 @@ void tearDown() { httpRequestService.stop(); scheduledTasks.clear(); verify(executorService).shutdown(); - verifyNoMoreInteractions(executorService); } @Test @@ -204,8 +201,6 @@ void verifySendingRequest_whenServerProvidesRetryInfo() { HttpSender.Response response = createSuccessfulResponse(serverToAgent); verifyRetryDelayOnError(response, Duration.ofNanos(nanosecondsToWaitForRetry)); - verify(callback).onRequestFailed(any()); - verify(callback, never()).onRequestSuccess(any()); } @Test @@ -218,8 +213,6 @@ void verifySendingRequest_whenServerIsUnavailable() { HttpSender.Response response = createSuccessfulResponse(serverToAgent); verifyRetryDelayOnError(response, RETRY_DELAY); - verify(callback).onRequestFailed(any()); - verify(callback, never()).onRequestSuccess(any()); } @Test @@ -244,27 +237,18 @@ void verifySendingRequest_duringRegularMode() { verifySingleRequestSent(); } - @Test - void verifySendingRequest_duringRetryMode() { - enableRetryMode(); - requestSender.requests.clear(); - - httpRequestService.sendRequest(); - - assertThat(requestSender.requests).isEmpty(); - } - private void verifyRetryDelayOnError( HttpSender.Response errorResponse, Duration expectedRetryDelay) { requestSender.enqueueResponse(errorResponse); ScheduledTask previousTask = getCurrentScheduledTask(); scheduledTasks.clear(); - httpRequestService.sendRequest(); + previousTask.runnable.run(); verifySingleRequestSent(); verify(previousTask.future).cancel(false); verify(periodicRetryDelay).reset(); + verify(callback).onRequestFailed(any()); ScheduledTask retryTask = getCurrentScheduledTask(); assertThat(retryTask.delay).isEqualTo(expectedRetryDelay); @@ -276,6 +260,7 @@ private void verifyRetryDelayOnError( verify(retryTask.future, never()).cancel(anyBoolean()); verifySingleRequestSent(); + verify(callback).onRequestFailed(any()); ScheduledTask retryTask2 = getCurrentScheduledTask(); assertThat(retryTask2.delay).isEqualTo(expectedRetryDelay); @@ -293,13 +278,6 @@ private void verifyRetryDelayOnError( assertThat(getCurrentScheduledTask().delay).isEqualTo(REGULAR_DELAY); } - private void enableRetryMode() { - HttpSender.Response response = createFailedResponse(503); - requestSender.enqueueResponse(response); - - httpRequestService.sendRequest(); - } - private Supplier createRequestSupplier() { AgentToServer agentToServer = new AgentToServer.Builder().sequence_num(10).build(); requestSize = agentToServer.encodeByteString().size(); @@ -331,7 +309,8 @@ private void verifyRequestFailedCallback(int errorCode) { private ScheduledExecutorService createTestScheduleExecutorService() { ScheduledExecutorService service = mock(); - doAnswer( + lenient() + .doAnswer( invocation -> { Runnable runnable = invocation.getArgument(0); runnable.run(); From f49fa67bc110a4425cb0d655982ec2d5fd5989d3 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 6 Jun 2025 18:40:23 +0200 Subject: [PATCH 34/38] Spotless --- .../client/internal/response/OpampServerResponseError.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/OpampServerResponseError.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/OpampServerResponseError.java index d88d194a7..08ad6a4d2 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/OpampServerResponseError.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/response/OpampServerResponseError.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.opamp.client.internal.response; public class OpampServerResponseError extends Exception { From 73cb9959b493ca28a5eaf0c912671e500463b144 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 11 Jun 2025 09:15:21 +0200 Subject: [PATCH 35/38] Avoiding verifying task cancel call in tests --- .../service/HttpRequestServiceTest.java | 88 +++++++++++++------ 1 file changed, 59 insertions(+), 29 deletions(-) diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index e0a4bddd3..07b3de37b 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -8,13 +8,11 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -35,17 +33,18 @@ import java.util.NoSuchElementException; import java.util.Queue; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Delayed; import java.util.concurrent.ExecutionException; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; -import java.util.function.Supplier; import opamp.proto.AgentToServer; import opamp.proto.RetryInfo; import opamp.proto.ServerErrorResponse; import opamp.proto.ServerErrorResponseType; import opamp.proto.ServerToAgent; +import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -62,7 +61,8 @@ class HttpRequestServiceTest { private static final Duration RETRY_DELAY = Duration.ofSeconds(5); @Mock private RequestService.Callback callback; - private final List scheduledTasks = new ArrayList<>(); + private final List scheduledTasks = + Collections.synchronizedList(new ArrayList<>()); private ScheduledExecutorService executorService; private TestHttpSender requestSender; private PeriodicDelay periodicRequestDelay; @@ -83,7 +83,7 @@ void setUp() { periodicRequestDelay, periodicRetryDelay, RetryAfterParser.getInstance()); - httpRequestService.start(callback, createRequestSupplier()); + httpRequestService.start(callback, this::createRequestSupplier); } @AfterEach @@ -102,7 +102,7 @@ void verifyStart_scheduledFirstTask() { // Verify initial task creates next one scheduledTasks.clear(); requestSender.enqueueResponse(createSuccessfulResponse(new ServerToAgent.Builder().build())); - firstTask.runnable.run(); + firstTask.run(); assertThat(scheduledTasks).hasSize(1); @@ -241,12 +241,10 @@ private void verifyRetryDelayOnError( HttpSender.Response errorResponse, Duration expectedRetryDelay) { requestSender.enqueueResponse(errorResponse); ScheduledTask previousTask = getCurrentScheduledTask(); - scheduledTasks.clear(); - previousTask.runnable.run(); + previousTask.run(); verifySingleRequestSent(); - verify(previousTask.future).cancel(false); verify(periodicRetryDelay).reset(); verify(callback).onRequestFailed(any()); ScheduledTask retryTask = getCurrentScheduledTask(); @@ -256,9 +254,8 @@ private void verifyRetryDelayOnError( clearInvocations(callback); scheduledTasks.clear(); requestSender.enqueueResponse(createFailedResponse(500)); - retryTask.runnable.run(); + retryTask.run(); - verify(retryTask.future, never()).cancel(anyBoolean()); verifySingleRequestSent(); verify(callback).onRequestFailed(any()); ScheduledTask retryTask2 = getCurrentScheduledTask(); @@ -269,20 +266,18 @@ private void verifyRetryDelayOnError( scheduledTasks.clear(); ServerToAgent serverToAgent = new ServerToAgent.Builder().build(); requestSender.enqueueResponse(createSuccessfulResponse(serverToAgent)); - retryTask2.runnable.run(); + retryTask2.run(); - verify(retryTask2.future).cancel(false); verify(periodicRequestDelay).reset(); verifySingleRequestSent(); verifyRequestSuccessCallback(serverToAgent); assertThat(getCurrentScheduledTask().delay).isEqualTo(REGULAR_DELAY); } - private Supplier createRequestSupplier() { + private Request createRequestSupplier() { AgentToServer agentToServer = new AgentToServer.Builder().sequence_num(10).build(); requestSize = agentToServer.encodeByteString().size(); - Request request = Request.create(agentToServer); - return () -> request; + return Request.create(agentToServer); } private ScheduledTask getCurrentScheduledTask() { @@ -322,11 +317,12 @@ private ScheduledExecutorService createTestScheduleExecutorService() { when(service.schedule(any(Runnable.class), anyLong(), any(TimeUnit.class))) .thenAnswer( invocation -> { - ScheduledFuture future = mock(); - scheduledTasks.add( - ScheduledTask.create( - future, invocation.getArgument(0), invocation.getArgument(1))); - return future; + ScheduledTask task = + new ScheduledTask(invocation.getArgument(0), invocation.getArgument(1)); + + scheduledTasks.add(task); + + return task; }); return service; @@ -431,20 +427,54 @@ private RequestParams(int contentLength) { } } - private static class ScheduledTask { - private final ScheduledFuture future; + private class ScheduledTask implements ScheduledFuture { private final Runnable runnable; private final Duration delay; - private static ScheduledTask create( - ScheduledFuture future, Runnable runnable, long delayNanos) { - return new ScheduledTask(future, runnable, Duration.ofNanos(delayNanos)); + public void run() { + get(); } - private ScheduledTask(ScheduledFuture future, Runnable runnable, Duration delay) { - this.future = future; + private ScheduledTask(Runnable runnable, long timeNanos) { this.runnable = runnable; - this.delay = delay; + this.delay = Duration.ofNanos(timeNanos); + } + + @Override + public long getDelay(@NotNull TimeUnit unit) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean cancel(boolean mayInterruptIfRunning) { + return scheduledTasks.remove(this); + } + + @Override + public boolean isCancelled() { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isDone() { + throw new UnsupportedOperationException(); + } + + @Override + public Object get() { + scheduledTasks.remove(this); + runnable.run(); + return null; + } + + @Override + public Object get(long timeout, @NotNull TimeUnit unit) { + throw new UnsupportedOperationException(); + } + + @Override + public int compareTo(@NotNull Delayed o) { + throw new UnsupportedOperationException(); } } } From 3bb8d7bd828e8e10ad1de02169c607320f8f7311 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 11 Jun 2025 11:49:55 +0200 Subject: [PATCH 36/38] Ensuring only one task is executed at a time and it always schedules the next one --- .../request/service/HttpRequestService.java | 72 +++++++++++-------- .../service/HttpRequestServiceTest.java | 23 ++++-- 2 files changed, 62 insertions(+), 33 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index 6082572a7..f0f459c69 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -27,6 +27,8 @@ import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Supplier; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -43,7 +45,8 @@ public final class HttpRequestService implements RequestService { private final AtomicBoolean isRunning = new AtomicBoolean(false); private final AtomicBoolean hasStopped = new AtomicBoolean(false); private final AtomicReference currentDelay; - private final AtomicReference> currentTask = new AtomicReference<>(); + private final AtomicReference> scheduledTask = new AtomicReference<>(); + private final Lock sendLock = new ReentrantLock(); private final RetryAfterParser retryAfterParser; @Nullable private Callback callback; @Nullable private Supplier requestSupplier; @@ -100,26 +103,12 @@ public void start(Callback callback, Supplier requestSupplier) { if (isRunning.compareAndSet(false, true)) { this.callback = callback; this.requestSupplier = requestSupplier; - currentTask.set( - executorService.schedule( - this::periodicSend, getNextDelay().toNanos(), TimeUnit.NANOSECONDS)); + scheduleNextExecution(); } else { throw new IllegalStateException("HttpRequestService is already running"); } } - private void periodicSend() { - doSendRequest(); - // schedule the next execution - currentTask.set( - executorService.schedule( - this::periodicSend, getNextDelay().toNanos(), TimeUnit.NANOSECONDS)); - } - - private void sendOnce() { - executorService.execute(this::doSendRequest); - } - private Duration getNextDelay() { return Objects.requireNonNull(currentDelay.get()).getNextDelay(); } @@ -138,7 +127,43 @@ public void sendRequest() { throw new IllegalStateException("HttpRequestService is not running"); } - sendOnce(); + executorService.execute(this::requestOnDemand); + } + + private void requestOnDemand() { + if (sendLock.tryLock()) { + try { + ScheduledFuture scheduledFuture = scheduledTask.get(); + if (scheduledFuture != null) { + // Cancel future task + scheduledFuture.cancel(false); + } + sendAndScheduleNext(); + } finally { + sendLock.unlock(); + } + } + } + + private void periodicRequest() { + if (sendLock.tryLock()) { + try { + sendAndScheduleNext(); + } finally { + sendLock.unlock(); + } + } + } + + private void sendAndScheduleNext() { + doSendRequest(); + scheduleNextExecution(); + } + + private void scheduleNextExecution() { + scheduledTask.set( + executorService.schedule( + this::periodicRequest, getNextDelay().toNanos(), TimeUnit.NANOSECONDS)); } private void doSendRequest() { @@ -150,7 +175,7 @@ private void doSendRequest() { try (HttpSender.Response response = future.get(30, TimeUnit.SECONDS)) { getCallback().onConnectionSuccess(); if (isSuccessful(response)) { - handleSuccessResponse( + handleHttpSuccess( Response.create(ServerToAgent.ADAPTER.decode(response.bodyInputStream()))); } else { handleHttpError(response); @@ -187,7 +212,7 @@ private static boolean isSuccessful(HttpSender.Response response) { return response.statusCode() >= 200 && response.statusCode() < 300; } - private void handleSuccessResponse(Response response) { + private void handleHttpSuccess(Response response) { useRegularDelay(); ServerToAgent serverToAgent = response.getServerToAgent(); @@ -211,14 +236,12 @@ private void handleErrorResponse(ServerErrorResponse errorResponse) { private void useRegularDelay() { if (currentDelay.compareAndSet(periodicRetryDelay, periodicRequestDelay)) { - cancelCurrentTask(); periodicRequestDelay.reset(); } } private void useRetryDelay(@Nullable Duration retryAfter) { if (currentDelay.compareAndSet(periodicRequestDelay, periodicRetryDelay)) { - cancelCurrentTask(); periodicRetryDelay.reset(); if (retryAfter != null && periodicRetryDelay instanceof AcceptsDelaySuggestion) { ((AcceptsDelaySuggestion) periodicRetryDelay).suggestDelay(retryAfter); @@ -226,13 +249,6 @@ private void useRetryDelay(@Nullable Duration retryAfter) { } } - private void cancelCurrentTask() { - ScheduledFuture future = currentTask.get(); - if (future != null) { - future.cancel(false); - } - } - private Callback getCallback() { return Objects.requireNonNull(callback); } diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index 07b3de37b..0679691b3 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -126,6 +126,19 @@ void verifySendingRequest_happyPath() { verify(callback).onConnectionSuccess(); } + @Test + void verifyWhenSendingOnDemandRequest_andDelayChanges() { + // Initial state + assertThat(assertAndGetSingleCurrentTask().delay).isEqualTo(REGULAR_DELAY); + + // Trigger delay strategy change + requestSender.enqueueResponse(createFailedResponse(503)); + httpRequestService.sendRequest(); + + // Expected state + assertThat(assertAndGetSingleCurrentTask().delay).isEqualTo(RETRY_DELAY); + } + @Test void verifySendingRequest_whenTheresAParsingError() { HttpSender.Response httpResponse = createSuccessfulResponse(new byte[] {1, 2, 3}); @@ -240,14 +253,14 @@ void verifySendingRequest_duringRegularMode() { private void verifyRetryDelayOnError( HttpSender.Response errorResponse, Duration expectedRetryDelay) { requestSender.enqueueResponse(errorResponse); - ScheduledTask previousTask = getCurrentScheduledTask(); + ScheduledTask previousTask = assertAndGetSingleCurrentTask(); previousTask.run(); verifySingleRequestSent(); verify(periodicRetryDelay).reset(); verify(callback).onRequestFailed(any()); - ScheduledTask retryTask = getCurrentScheduledTask(); + ScheduledTask retryTask = assertAndGetSingleCurrentTask(); assertThat(retryTask.delay).isEqualTo(expectedRetryDelay); // Retry with another error @@ -258,7 +271,7 @@ private void verifyRetryDelayOnError( verifySingleRequestSent(); verify(callback).onRequestFailed(any()); - ScheduledTask retryTask2 = getCurrentScheduledTask(); + ScheduledTask retryTask2 = assertAndGetSingleCurrentTask(); assertThat(retryTask2.delay).isEqualTo(expectedRetryDelay); // Retry with a success @@ -271,7 +284,7 @@ private void verifyRetryDelayOnError( verify(periodicRequestDelay).reset(); verifySingleRequestSent(); verifyRequestSuccessCallback(serverToAgent); - assertThat(getCurrentScheduledTask().delay).isEqualTo(REGULAR_DELAY); + assertThat(assertAndGetSingleCurrentTask().delay).isEqualTo(REGULAR_DELAY); } private Request createRequestSupplier() { @@ -280,7 +293,7 @@ private Request createRequestSupplier() { return Request.create(agentToServer); } - private ScheduledTask getCurrentScheduledTask() { + private ScheduledTask assertAndGetSingleCurrentTask() { assertThat(scheduledTasks).hasSize(1); return scheduledTasks.get(0); } From af0fc2081d156f245215fd15bb75d646727c5daa Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 11 Jun 2025 12:30:49 +0200 Subject: [PATCH 37/38] Clean up --- .../internal/request/service/HttpRequestServiceTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java index 0679691b3..63439e8c6 100644 --- a/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java +++ b/opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestServiceTest.java @@ -61,8 +61,7 @@ class HttpRequestServiceTest { private static final Duration RETRY_DELAY = Duration.ofSeconds(5); @Mock private RequestService.Callback callback; - private final List scheduledTasks = - Collections.synchronizedList(new ArrayList<>()); + private final List scheduledTasks = new ArrayList<>(); private ScheduledExecutorService executorService; private TestHttpSender requestSender; private PeriodicDelay periodicRequestDelay; From 0a62b94e0fb06e69cc0829476ebb5a7432cdcea6 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Mon, 16 Jun 2025 07:02:09 +0200 Subject: [PATCH 38/38] Applying suggested changes in review --- .../request/service/HttpRequestService.java | 113 +++++++++--------- 1 file changed, 58 insertions(+), 55 deletions(-) diff --git a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java index f0f459c69..32d5a60f1 100644 --- a/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java +++ b/opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/service/HttpRequestService.java @@ -27,8 +27,6 @@ import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; import java.util.function.Supplier; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -39,14 +37,13 @@ public final class HttpRequestService implements RequestService { private final HttpSender requestSender; + // must be a single threaded executor, the code in this class relies on requests being processed + // serially private final ScheduledExecutorService executorService; - private final PeriodicDelay periodicRequestDelay; - private final PeriodicDelay periodicRetryDelay; private final AtomicBoolean isRunning = new AtomicBoolean(false); private final AtomicBoolean hasStopped = new AtomicBoolean(false); - private final AtomicReference currentDelay; + private final ConnectionStatus connectionStatus; private final AtomicReference> scheduledTask = new AtomicReference<>(); - private final Lock sendLock = new ReentrantLock(); private final RetryAfterParser retryAfterParser; @Nullable private Callback callback; @Nullable private Supplier requestSupplier; @@ -89,10 +86,8 @@ public static HttpRequestService create( RetryAfterParser retryAfterParser) { this.requestSender = requestSender; this.executorService = executorService; - this.periodicRequestDelay = periodicRequestDelay; - this.periodicRetryDelay = periodicRetryDelay; this.retryAfterParser = retryAfterParser; - currentDelay = new AtomicReference<>(periodicRequestDelay); + this.connectionStatus = new ConnectionStatus(periodicRequestDelay, periodicRetryDelay); } @Override @@ -109,10 +104,6 @@ public void start(Callback callback, Supplier requestSupplier) { } } - private Duration getNextDelay() { - return Objects.requireNonNull(currentDelay.get()).getNextDelay(); - } - @Override public void stop() { if (isRunning.compareAndSet(true, false)) { @@ -127,32 +118,16 @@ public void sendRequest() { throw new IllegalStateException("HttpRequestService is not running"); } - executorService.execute(this::requestOnDemand); - } - - private void requestOnDemand() { - if (sendLock.tryLock()) { - try { - ScheduledFuture scheduledFuture = scheduledTask.get(); - if (scheduledFuture != null) { - // Cancel future task - scheduledFuture.cancel(false); - } - sendAndScheduleNext(); - } finally { - sendLock.unlock(); - } - } - } - - private void periodicRequest() { - if (sendLock.tryLock()) { - try { - sendAndScheduleNext(); - } finally { - sendLock.unlock(); - } - } + executorService.execute( + () -> { + // cancel the already scheduled task, a new one is created after current request is + // processed + ScheduledFuture scheduledFuture = scheduledTask.get(); + if (scheduledFuture != null) { + scheduledFuture.cancel(false); + } + sendAndScheduleNext(); + }); } private void sendAndScheduleNext() { @@ -163,7 +138,9 @@ private void sendAndScheduleNext() { private void scheduleNextExecution() { scheduledTask.set( executorService.schedule( - this::periodicRequest, getNextDelay().toNanos(), TimeUnit.NANOSECONDS)); + this::sendAndScheduleNext, + connectionStatus.getNextDelay().toNanos(), + TimeUnit.NANOSECONDS)); } private void doSendRequest() { @@ -204,7 +181,7 @@ private void handleHttpError(HttpSender.Response response) { retryAfter = duration.get(); } } - useRetryDelay(retryAfter); + connectionStatus.retryAfter(retryAfter); } } @@ -213,7 +190,7 @@ private static boolean isSuccessful(HttpSender.Response response) { } private void handleHttpSuccess(Response response) { - useRegularDelay(); + connectionStatus.success(); ServerToAgent serverToAgent = response.getServerToAgent(); if (serverToAgent.error_response != null) { @@ -229,28 +206,54 @@ private void handleErrorResponse(ServerErrorResponse errorResponse) { if (errorResponse.retry_info != null) { retryAfter = Duration.ofNanos(errorResponse.retry_info.retry_after_nanoseconds); } - useRetryDelay(retryAfter); + connectionStatus.retryAfter(retryAfter); } getCallback().onRequestFailed(new OpampServerResponseError(errorResponse.error_message)); } - private void useRegularDelay() { - if (currentDelay.compareAndSet(periodicRetryDelay, periodicRequestDelay)) { - periodicRequestDelay.reset(); - } + private Callback getCallback() { + return Objects.requireNonNull(callback); } - private void useRetryDelay(@Nullable Duration retryAfter) { - if (currentDelay.compareAndSet(periodicRequestDelay, periodicRetryDelay)) { - periodicRetryDelay.reset(); - if (retryAfter != null && periodicRetryDelay instanceof AcceptsDelaySuggestion) { - ((AcceptsDelaySuggestion) periodicRetryDelay).suggestDelay(retryAfter); + // this class is only used from a single threaded ScheduledExecutorService, hence no + // synchronization is needed + private static class ConnectionStatus { + private final PeriodicDelay periodicRequestDelay; + private final PeriodicDelay periodicRetryDelay; + + private boolean retrying; + private PeriodicDelay currentDelay; + + ConnectionStatus(PeriodicDelay periodicRequestDelay, PeriodicDelay periodicRetryDelay) { + this.periodicRequestDelay = periodicRequestDelay; + this.periodicRetryDelay = periodicRetryDelay; + currentDelay = periodicRequestDelay; + } + + void success() { + // after successful request transition from retry to regular delay + if (retrying) { + retrying = false; + periodicRequestDelay.reset(); + currentDelay = periodicRequestDelay; } } - } - private Callback getCallback() { - return Objects.requireNonNull(callback); + void retryAfter(@Nullable Duration retryAfter) { + // after failed request transition from regular to retry delay + if (!retrying) { + retrying = true; + periodicRetryDelay.reset(); + currentDelay = periodicRetryDelay; + if (retryAfter != null && periodicRetryDelay instanceof AcceptsDelaySuggestion) { + ((AcceptsDelaySuggestion) periodicRetryDelay).suggestDelay(retryAfter); + } + } + } + + Duration getNextDelay() { + return currentDelay.getNextDelay(); + } } private static class DaemonThreadFactory implements ThreadFactory {