Skip to content
This repository has been archived by the owner on Jul 27, 2023. It is now read-only.

HH-157725 add path and responseCode parameters to ClientEventCallback interface methods #464

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

HH-157725 add path and responseCode parameters to ClientEventCallback interface methods #464

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4fb1c1
HH-157725 add path and responseCode parameters to ClientEventCallback…
rogoza-igor Aug 15, 2022
fb3fbcf
delete forRemoval attribute, call old deprecated methods in new metho…
rogoza-igor Jan 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/main/java/com/orbitz/consul/monitoring/ClientEventCallback.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,30 @@

public interface ClientEventCallback {

/**
* @deprecated use {@link #onHttpRequestSuccess(String, String, String, String, int)} instead.
*/
@Deprecated(forRemoval = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is is ok to use Java 9 style @deprecated in this project?

Copy link
Author

Choose a reason for hiding this comment

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

Deleted forRemoval attribute

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) { }
Expand Down
28 changes: 17 additions & 11 deletions src/main/java/com/orbitz/consul/monitoring/ClientEventHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, is it good to have both callbacks executed?

Maybe it would be better to call the old deprecated method in new default one:

default void onHttpRequestSuccess(String clientName, String method, String path, String queryString, int responseCode) {
        // Kept for compatibility at the moment
        onHttpRequestSuccess(clientName, method, queryString);        
}

Then replacing calls here would still keep the backwards compatibility?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's good idea, thank you!

});
}

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) {
Expand Down
14 changes: 6 additions & 8 deletions src/main/java/com/orbitz/consul/util/Http.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -56,10 +54,10 @@ private <T> Response<T> executeCall(Call<T> call) {

private <T> void ensureResponseSuccessful(Call<T> call, Response<T> 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;
}
}
Expand All @@ -76,11 +74,11 @@ <T> retrofit2.Callback<T> createCallback(Call<T> call, final ConsulResponseCallb
@Override
public void onResponse(Call<T> call, Response<T> 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);
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/test/java/com/orbitz/consul/util/HttpTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ private <U, V> void checkSuccessEventIsSentWhenRequestSucceed(Function<Call<U>,

httpCall.apply(call);

verify(clientEventHandler, only()).httpRequestSuccess(any(Request.class));
verify(clientEventHandler, only()).httpRequestSuccess(any(Request.class), any(Response.class));
}

@Test
Expand Down Expand Up @@ -209,7 +209,7 @@ private <U, V> void checkInvalidEventIsSentWhenRequestIsInvalid(Function<Call<U>
//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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down