From e4fb1c1df99194bc0dc8874f2abe26ffb39615e3 Mon Sep 17 00:00:00 2001 From: Igor Rogoza Date: Mon, 15 Aug 2022 13:19:10 +0300 Subject: [PATCH 1/2] HH-157725 add path and responseCode parameters to ClientEventCallback interface methods --- .../monitoring/ClientEventCallback.java | 18 ++++++++++++ .../consul/monitoring/ClientEventHandler.java | 28 +++++++++++-------- .../java/com/orbitz/consul/util/Http.java | 14 ++++------ .../java/com/orbitz/consul/util/HttpTest.java | 8 +++--- 4 files changed, 45 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/orbitz/consul/monitoring/ClientEventCallback.java b/src/main/java/com/orbitz/consul/monitoring/ClientEventCallback.java index 6984b02e..fce1a170 100644 --- a/src/main/java/com/orbitz/consul/monitoring/ClientEventCallback.java +++ b/src/main/java/com/orbitz/consul/monitoring/ClientEventCallback.java @@ -6,12 +6,30 @@ public interface ClientEventCallback { + /** + * @deprecated use {@link #onHttpRequestSuccess(String, String, String, String, int)} instead. + */ + @Deprecated(forRemoval = true) default void onHttpRequestSuccess(String clientName, String method, String queryString) { } + default void onHttpRequestSuccess(String clientName, String method, String path, String queryString, int responseCode) { } + + /** + * @deprecated use {@link #onHttpRequestFailure(String, String, String, String, Throwable)} instead. + */ + @Deprecated(forRemoval = true) default void onHttpRequestFailure(String clientName, String method, String queryString, Throwable throwable) { } + default void onHttpRequestFailure(String clientName, String method, String path, String queryString, Throwable throwable) { } + + /** + * @deprecated use {@link #onHttpRequestInvalid(String, String, String, String, int, Throwable)} instead. + */ + @Deprecated(forRemoval = true) default void onHttpRequestInvalid(String clientName, String method, String queryString, Throwable throwable) { } + default void onHttpRequestInvalid(String clientName, String method, String path, String queryString, int responseCode, Throwable throwable) { } + default void onCacheStart(String clientName, CacheDescriptor cacheDescriptor) { } default void onCacheStop(String clientName, CacheDescriptor cacheDescriptor) { } diff --git a/src/main/java/com/orbitz/consul/monitoring/ClientEventHandler.java b/src/main/java/com/orbitz/consul/monitoring/ClientEventHandler.java index aad5a11f..adce09ef 100644 --- a/src/main/java/com/orbitz/consul/monitoring/ClientEventHandler.java +++ b/src/main/java/com/orbitz/consul/monitoring/ClientEventHandler.java @@ -2,14 +2,12 @@ import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.orbitz.consul.cache.CacheDescriptor; -import okhttp3.Request; - import java.time.Duration; import java.time.temporal.ChronoUnit; -import java.time.temporal.TemporalUnit; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; +import okhttp3.Request; +import retrofit2.Response; public class ClientEventHandler { @@ -24,18 +22,26 @@ public ClientEventHandler(String clientName, ClientEventCallback callback) { this.callback = callback; } - public void httpRequestSuccess(Request request) { - EVENT_EXECUTOR.submit(() -> callback.onHttpRequestSuccess(clientName, request.method(), request.url().query())); + public void httpRequestSuccess(Request request, Response response) { + EVENT_EXECUTOR.submit(() -> { + callback.onHttpRequestSuccess(clientName, request.method(), request.url().query()); + callback.onHttpRequestSuccess(clientName, request.method(), request.url().encodedPath(), request.url().query(), response.code()); + }); } - public void httpRequestInvalid(Request request, Throwable throwable) { - EVENT_EXECUTOR.submit(() -> - callback.onHttpRequestInvalid(clientName, request.method(), request.url().query(), throwable)); + public void httpRequestInvalid(Request request, Response response, Throwable throwable) { + EVENT_EXECUTOR.submit(() -> { + callback.onHttpRequestInvalid(clientName, request.method(), request.url().query(), throwable); + callback.onHttpRequestInvalid(clientName, request.method(), request.url().encodedPath(), request.url().query(), response.code(), + throwable); + }); } public void httpRequestFailure(Request request, Throwable throwable) { - EVENT_EXECUTOR.submit(() -> - callback.onHttpRequestFailure(clientName, request.method(), request.url().query(), throwable)); + EVENT_EXECUTOR.submit(() -> { + callback.onHttpRequestFailure(clientName, request.method(), request.url().query(), throwable); + callback.onHttpRequestFailure(clientName, request.method(), request.url().encodedPath(), request.url().query(), throwable); + }); } public void cacheStart(CacheDescriptor cacheDescriptor) { diff --git a/src/main/java/com/orbitz/consul/util/Http.java b/src/main/java/com/orbitz/consul/util/Http.java index 7f60a6a1..a04ea5e2 100644 --- a/src/main/java/com/orbitz/consul/util/Http.java +++ b/src/main/java/com/orbitz/consul/util/Http.java @@ -1,21 +1,19 @@ package com.orbitz.consul.util; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Optional; import com.google.common.collect.Sets; import com.orbitz.consul.ConsulException; import com.orbitz.consul.async.Callback; import com.orbitz.consul.async.ConsulResponseCallback; import com.orbitz.consul.model.ConsulResponse; import com.orbitz.consul.monitoring.ClientEventHandler; +import java.io.IOException; +import java.math.BigInteger; import okhttp3.Headers; import org.apache.commons.lang3.math.NumberUtils; import retrofit2.Call; import retrofit2.Response; -import java.io.IOException; -import java.math.BigInteger; - public class Http { private final ClientEventHandler eventHandler; @@ -56,10 +54,10 @@ private Response executeCall(Call call) { private void ensureResponseSuccessful(Call call, Response response, Integer... okCodes) { if(isSuccessful(response, okCodes)) { - eventHandler.httpRequestSuccess(call.request()); + eventHandler.httpRequestSuccess(call.request(), response); } else { ConsulException exception = new ConsulException(response.code(), response); - eventHandler.httpRequestInvalid(call.request(), exception); + eventHandler.httpRequestInvalid(call.request(), response, exception); throw exception; } } @@ -76,11 +74,11 @@ retrofit2.Callback createCallback(Call call, final ConsulResponseCallb @Override public void onResponse(Call call, Response response) { if (isSuccessful(response, okCodes)) { - eventHandler.httpRequestSuccess(call.request()); + eventHandler.httpRequestSuccess(call.request(), response); callback.onComplete(consulResponse(response)); } else { ConsulException exception = new ConsulException(response.code(), response); - eventHandler.httpRequestInvalid(call.request(), exception); + eventHandler.httpRequestInvalid(call.request(), response, exception); callback.onFailure(exception); } } diff --git a/src/test/java/com/orbitz/consul/util/HttpTest.java b/src/test/java/com/orbitz/consul/util/HttpTest.java index 453f7fc9..c1fd842d 100644 --- a/src/test/java/com/orbitz/consul/util/HttpTest.java +++ b/src/test/java/com/orbitz/consul/util/HttpTest.java @@ -152,7 +152,7 @@ private void checkSuccessEventIsSentWhenRequestSucceed(Function, httpCall.apply(call); - verify(clientEventHandler, only()).httpRequestSuccess(any(Request.class)); + verify(clientEventHandler, only()).httpRequestSuccess(any(Request.class), any(Response.class)); } @Test @@ -209,7 +209,7 @@ private void checkInvalidEventIsSentWhenRequestIsInvalid(Function //ignore } - verify(clientEventHandler, only()).httpRequestInvalid(any(Request.class), any(Throwable.class)); + verify(clientEventHandler, only()).httpRequestInvalid(any(Request.class), any(Response.class), any(Throwable.class)); } @Test @@ -237,7 +237,7 @@ public void onFailure(Throwable throwable) { } latch.await(1, TimeUnit.SECONDS); assertEquals(expectedBody, result.get().getResponse()); - verify(clientEventHandler, only()).httpRequestSuccess(any(Request.class)); + verify(clientEventHandler, only()).httpRequestSuccess(any(Request.class), any(Response.class)); } @Test @@ -261,7 +261,7 @@ public void onFailure(Throwable throwable) { callCallback.onResponse(call, response); latch.await(1, TimeUnit.SECONDS); - verify(clientEventHandler, only()).httpRequestInvalid(any(Request.class), any(Throwable.class)); + verify(clientEventHandler, only()).httpRequestInvalid(any(Request.class), any(Response.class), any(Throwable.class)); } @Test From fb3fbcf6dbc50ba32409b483fed3d1e28ac06d39 Mon Sep 17 00:00:00 2001 From: Igor Rogoza Date: Mon, 9 Jan 2023 16:05:42 +0700 Subject: [PATCH 2/2] delete forRemoval attribute, call old deprecated methods in new methods for backwards compatibility --- .../monitoring/ClientEventCallback.java | 21 +++++++++++++------ .../consul/monitoring/ClientEventHandler.java | 21 ++++++++----------- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/orbitz/consul/monitoring/ClientEventCallback.java b/src/main/java/com/orbitz/consul/monitoring/ClientEventCallback.java index fce1a170..b62bef74 100644 --- a/src/main/java/com/orbitz/consul/monitoring/ClientEventCallback.java +++ b/src/main/java/com/orbitz/consul/monitoring/ClientEventCallback.java @@ -9,26 +9,35 @@ public interface ClientEventCallback { /** * @deprecated use {@link #onHttpRequestSuccess(String, String, String, String, int)} instead. */ - @Deprecated(forRemoval = true) + @Deprecated default void onHttpRequestSuccess(String clientName, String method, String queryString) { } - default void onHttpRequestSuccess(String clientName, String method, String path, String queryString, int responseCode) { } + default void onHttpRequestSuccess(String clientName, String method, String path, String queryString, int responseCode) { + // Kept for compatibility at the moment + onHttpRequestSuccess(clientName, method, queryString); + } /** * @deprecated use {@link #onHttpRequestFailure(String, String, String, String, Throwable)} instead. */ - @Deprecated(forRemoval = true) + @Deprecated default void onHttpRequestFailure(String clientName, String method, String queryString, Throwable throwable) { } - default void onHttpRequestFailure(String clientName, String method, String path, String queryString, Throwable throwable) { } + default void onHttpRequestFailure(String clientName, String method, String path, String queryString, Throwable throwable) { + // Kept for compatibility at the moment + onHttpRequestFailure(clientName, method, queryString, throwable); + } /** * @deprecated use {@link #onHttpRequestInvalid(String, String, String, String, int, Throwable)} instead. */ - @Deprecated(forRemoval = true) + @Deprecated default void onHttpRequestInvalid(String clientName, String method, String queryString, Throwable throwable) { } - default void onHttpRequestInvalid(String clientName, String method, String path, String queryString, int responseCode, Throwable throwable) { } + default void onHttpRequestInvalid(String clientName, String method, String path, String queryString, int responseCode, Throwable throwable) { + // Kept for compatibility at the moment + onHttpRequestInvalid(clientName, method, queryString, throwable); + } default void onCacheStart(String clientName, CacheDescriptor cacheDescriptor) { } diff --git a/src/main/java/com/orbitz/consul/monitoring/ClientEventHandler.java b/src/main/java/com/orbitz/consul/monitoring/ClientEventHandler.java index adce09ef..a4078e58 100644 --- a/src/main/java/com/orbitz/consul/monitoring/ClientEventHandler.java +++ b/src/main/java/com/orbitz/consul/monitoring/ClientEventHandler.java @@ -23,25 +23,22 @@ public ClientEventHandler(String clientName, ClientEventCallback callback) { } public void httpRequestSuccess(Request request, Response response) { - EVENT_EXECUTOR.submit(() -> { - callback.onHttpRequestSuccess(clientName, request.method(), request.url().query()); - callback.onHttpRequestSuccess(clientName, request.method(), request.url().encodedPath(), request.url().query(), response.code()); - }); + EVENT_EXECUTOR.submit(() -> + callback.onHttpRequestSuccess(clientName, request.method(), request.url().encodedPath(), request.url().query(), response.code()) + ); } public void httpRequestInvalid(Request request, Response response, Throwable throwable) { - EVENT_EXECUTOR.submit(() -> { - callback.onHttpRequestInvalid(clientName, request.method(), request.url().query(), throwable); + EVENT_EXECUTOR.submit(() -> callback.onHttpRequestInvalid(clientName, request.method(), request.url().encodedPath(), request.url().query(), response.code(), - throwable); - }); + throwable) + ); } public void httpRequestFailure(Request request, Throwable throwable) { - EVENT_EXECUTOR.submit(() -> { - callback.onHttpRequestFailure(clientName, request.method(), request.url().query(), throwable); - callback.onHttpRequestFailure(clientName, request.method(), request.url().encodedPath(), request.url().query(), throwable); - }); + EVENT_EXECUTOR.submit(() -> + callback.onHttpRequestFailure(clientName, request.method(), request.url().encodedPath(), request.url().query(), throwable) + ); } public void cacheStart(CacheDescriptor cacheDescriptor) {