Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: httpjson callables to trace attempts (started, failed) #3300

Merged
merged 30 commits into from
Nov 13, 2024
Merged

fix: httpjson callables to trace attempts (started, failed) #3300

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
6635019
add test to confirm tracer attempts failed
diegomarquezp Oct 21, 2024
944e8d6
simplify test for retries
diegomarquezp Oct 21, 2024
1edf557
Merge remote-tracking branch 'origin/main' into fix-httpjson-retry-me…
diegomarquezp Oct 21, 2024
4db1d2b
extend tracer tests to every test
diegomarquezp Oct 21, 2024
9ab23d5
fix test vars initialization
diegomarquezp Oct 21, 2024
6364aab
add retrial exhaustion check
diegomarquezp Oct 21, 2024
ae1eb44
Merge branch 'main' into fix-httpjson-retry-metrics
diegomarquezp Oct 22, 2024
1ebc3a2
restore ApiException expected in test
diegomarquezp Oct 24, 2024
25e778f
Merge branch 'fix-httpjson-retry-metrics' of https://github.com/googl…
diegomarquezp Oct 24, 2024
b363a29
enable Disabled ITs
diegomarquezp Oct 24, 2024
5c11e10
try fixing multipleattempts test
diegomarquezp Oct 24, 2024
9b48dbe
Merge branch 'main' into fix-httpjson-retry-metrics
diegomarquezp Oct 24, 2024
c8e8a7d
fix multiple attempts httpjson test (thanks @lqiu96)
diegomarquezp Oct 25, 2024
af3adb5
Merge remote-tracking branch 'refs/remotes/origin/fix-httpjson-retry-…
diegomarquezp Oct 25, 2024
035be17
extract tracer factory to its own method
diegomarquezp Oct 25, 2024
8e2a58a
add comments
diegomarquezp Oct 25, 2024
8f3331e
Merge branch 'fix-httpjson-retry-metrics' of https://github.com/googl…
diegomarquezp Oct 25, 2024
f647320
Merge remote-tracking branch 'origin/main' into fix-httpjson-retry-me…
diegomarquezp Oct 25, 2024
2df2e9f
licence
diegomarquezp Oct 25, 2024
e7bc1c3
formalize tests
diegomarquezp Oct 30, 2024
e2385ad
rename vars, extend ApiTracer
diegomarquezp Oct 31, 2024
58e874a
move initialization to contained class
diegomarquezp Oct 31, 2024
87f807a
fix order of methods
diegomarquezp Oct 31, 2024
e57b4db
remove retries exhausted
diegomarquezp Nov 4, 2024
58a0daa
Revert "remove retries exhausted"
diegomarquezp Nov 4, 2024
325eba4
record attempt failed when retries are exhausted
diegomarquezp Nov 4, 2024
2120960
adjust retries count in retryingtest
diegomarquezp Nov 4, 2024
0630fdf
Merge branch 'main' into fix-httpjson-retry-metrics
diegomarquezp Nov 11, 2024
9697e8a
empty commit to retry tests
diegomarquezp Nov 11, 2024
065b01f
remove flaky tests
diegomarquezp Nov 11, 2024
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
11 changes: 5 additions & 6 deletions gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallableFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ static <RequestT, ResponseT> UnaryCallable<RequestT, ResponseT> createUnaryCalla
callable =
Callables.retrying(
callable, callSettings, clientContext, httpJsonCallSettings.getRequestMutator());
callable =
new TracedUnaryCallable<>(
callable,
clientContext.getTracerFactory(),
getSpanName(httpJsonCallSettings.getMethodDescriptor()));
return callable.withDefaultCallContext(clientContext.getDefaultCallContext());
}

Expand Down Expand Up @@ -136,12 +141,6 @@ public static <RequestT, ResponseT> UnaryCallable<RequestT, ResponseT> createUna
UnaryCallable<RequestT, ResponseT> innerCallable =
createDirectUnaryCallable(httpJsonCallSettings);

innerCallable =
new TracedUnaryCallable<>(
innerCallable,
clientContext.getTracerFactory(),
getSpanName(httpJsonCallSettings.getMethodDescriptor()));

return createUnaryCallable(innerCallable, callSettings, httpJsonCallSettings, clientContext);
}

Expand Down
87 changes: 86 additions & 1 deletion gax-java/gax-httpjson/src/test/java/com/google/api/gax/httpjson/RetryingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,22 @@
import com.google.api.gax.rpc.UnaryCallSettings;
import com.google.api.gax.rpc.UnaryCallable;
import com.google.api.gax.rpc.UnknownException;
import com.google.api.gax.tracing.ApiTracer;
import com.google.api.gax.tracing.ApiTracerFactory;
import com.google.api.gax.tracing.BaseApiTracer;
import com.google.common.collect.ImmutableSet;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.UncheckedExecutionException;
import com.google.protobuf.TypeRegistry;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.Mockito;
import org.threeten.bp.Duration;

class RetryingTest {

Expand All @@ -88,6 +94,11 @@ class RetryingTest {
.setTypeRegistry(TypeRegistry.newBuilder().build())
.build();

private AtomicInteger tracerAttempts;
private AtomicInteger tracerAttemptsFailed;
private AtomicBoolean tracerOperationFailed;
private AtomicBoolean tracerFailedRetriesExhausted;

private RecordingScheduler executor;
private FakeApiClock fakeClock;
private ClientContext clientContext;
Expand All @@ -113,10 +124,16 @@ class RetryingTest {

@BeforeEach
void resetClock() {
tracerAttempts = new AtomicInteger();
tracerAttemptsFailed = new AtomicInteger();
tracerOperationFailed = new AtomicBoolean(false);
tracerFailedRetriesExhausted = new AtomicBoolean(false);
fakeClock = new FakeApiClock(System.nanoTime());
executor = RecordingScheduler.create(fakeClock);
clientContext =
ClientContext.newBuilder()
// we use a custom tracer to confirm whether the retrials are being recorded.
.setTracerFactory(getApiTracerFactory())
.setExecutor(executor)
.setClock(fakeClock)
.setDefaultCallContext(HttpJsonCallContext.createDefault())
Expand All @@ -130,6 +147,7 @@ void teardown() {

@Test
void retry() {
// set a retriable that will fail 3 times before returning "2"
ImmutableSet<StatusCode.Code> retryable = ImmutableSet.of(Code.UNAVAILABLE);
Mockito.when(callInt.futureCall((Integer) any(), (ApiCallContext) any()))
.thenReturn(ApiFutures.<Integer>immediateFailedFuture(HTTP_SERVICE_UNAVAILABLE_EXCEPTION))
Expand All @@ -143,6 +161,10 @@ void retry() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThat(callable.call(initialRequest)).isEqualTo(2);
assertThat(tracerAttemptsFailed.get()).isEqualTo(3);
assertThat(tracerAttempts.get()).isEqualTo(4);
assertThat(tracerOperationFailed.get()).isEqualTo(false);
assertThat(tracerFailedRetriesExhausted.get()).isEqualTo(false);

// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
Expand Down Expand Up @@ -180,6 +202,9 @@ void retryTotalTimeoutExceeded() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThrows(ApiException.class, () -> callable.call(initialRequest));
assertThat(tracerAttempts.get()).isEqualTo(1);
assertThat(tracerAttemptsFailed.get()).isEqualTo(0);
assertThat(tracerOperationFailed.get()).isEqualTo(true);
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
verify(callInt, atLeastOnce()).futureCall(argumentCaptor.capture(), any(ApiCallContext.class));
Expand All @@ -200,6 +225,9 @@ void retryMaxAttemptsExceeded() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThrows(ApiException.class, () -> callable.call(initialRequest));
assertThat(tracerAttempts.get()).isEqualTo(2);
assertThat(tracerAttemptsFailed.get()).isEqualTo(1);
assertThat(tracerFailedRetriesExhausted.get()).isEqualTo(true);
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
verify(callInt, atLeastOnce()).futureCall(argumentCaptor.capture(), any(ApiCallContext.class));
Expand All @@ -220,6 +248,9 @@ void retryWithinMaxAttempts() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThat(callable.call(initialRequest)).isEqualTo(2);
assertThat(tracerAttempts.get()).isEqualTo(3);
assertThat(tracerAttemptsFailed.get()).isEqualTo(2);
assertThat(tracerOperationFailed.get()).isEqualTo(false);
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
verify(callInt, atLeastOnce()).futureCall(argumentCaptor.capture(), any(ApiCallContext.class));
Expand All @@ -246,6 +277,9 @@ void retryOnStatusUnknown() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThat(callable.call(initialRequest)).isEqualTo(2);
assertThat(tracerAttempts.get()).isEqualTo(4);
assertThat(tracerAttemptsFailed.get()).isEqualTo(3);
assertThat(tracerOperationFailed.get()).isEqualTo(false);
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
verify(callInt, atLeastOnce()).futureCall(argumentCaptor.capture(), any(ApiCallContext.class));
Expand All @@ -263,7 +297,12 @@ void retryOnUnexpectedException() {
UnaryCallable<Integer, Integer> callable =
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
ApiException exception = assertThrows(ApiException.class, () -> callable.call(initialRequest));
// Should an unexpected RuntimeException be received as an UnknownException?
UnknownException exception =
assertThrows(UnknownException.class, () -> callable.call(initialRequest));
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
assertThat(tracerAttempts.get()).isEqualTo(1);
assertThat(tracerAttemptsFailed.get()).isEqualTo(0);
assertThat(tracerOperationFailed.get()).isEqualTo(true);
assertThat(exception).hasCauseThat().isSameInstanceAs(throwable);
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
Expand Down Expand Up @@ -293,6 +332,9 @@ void retryNoRecover() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
ApiException exception = assertThrows(ApiException.class, () -> callable.call(initialRequest));
assertThat(tracerAttempts.get()).isEqualTo(1);
assertThat(tracerAttemptsFailed.get()).isEqualTo(0);
assertThat(tracerOperationFailed.get()).isEqualTo(true);
assertThat(exception).isSameInstanceAs(apiException);
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
Expand All @@ -319,6 +361,12 @@ void retryKeepFailing() {

UncheckedExecutionException exception =
assertThrows(UncheckedExecutionException.class, () -> Futures.getUnchecked(future));
// the number of attempts varies. Here we just make sure that all of them except the last are
// considered as failed
Copy link
Contributor

Choose a reason for hiding this comment

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

qq, isn't the last one also considered a failed attempt? Why is the tracerAttemptsFailed != tracerAttempts?

Also, can we add the operationfailed == true check here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is the tracerAttemptsFailed != tracerAttempts?

I'm not sure and I agree they should be the same value since this test just fetches an exception until it can't retry anymore (retries exhausted).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from discussion with @blakeli0, @lqiu96 and @zhumin8, let's double check TestApiTracer, confirm with showcase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

showcase has another tracer we can use to cross check TestApiTracer

// attempts and that the operation was considered as failed.
assertThat(tracerAttemptsFailed.get()).isGreaterThan(0);
assertThat(tracerAttemptsFailed.get()).isEqualTo(tracerAttempts.get() - 1);
assertThat(tracerFailedRetriesExhausted.get()).isEqualTo(true);
assertThat(exception).hasCauseThat().isInstanceOf(ApiException.class);
assertThat(exception).hasCauseThat().hasMessageThat().contains("Unavailable");
// Capture the argument passed to futureCall
Expand Down Expand Up @@ -359,6 +407,9 @@ void testKnownStatusCode() {
callInt, callSettings, httpJsonCallSettings, clientContext);
ApiException exception =
assertThrows(FailedPreconditionException.class, () -> callable.call(initialRequest));
assertThat(tracerAttempts.get()).isEqualTo(1);
assertThat(tracerAttemptsFailed.get()).isEqualTo(0);
assertThat(tracerOperationFailed.get()).isEqualTo(true);
assertThat(exception.getStatusCode().getTransportCode())
.isEqualTo(HTTP_CODE_PRECONDITION_FAILED);
assertThat(exception).hasMessageThat().contains("precondition failed");
Expand All @@ -383,6 +434,9 @@ void testUnknownStatusCode() {
UnknownException exception =
assertThrows(UnknownException.class, () -> callable.call(initialRequest));
assertThat(exception).hasMessageThat().isEqualTo("java.lang.RuntimeException: unknown");
assertThat(tracerAttempts.get()).isEqualTo(1);
assertThat(tracerAttemptsFailed.get()).isEqualTo(0);
assertThat(tracerOperationFailed.get()).isEqualTo(true);
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
verify(callInt, atLeastOnce()).futureCall(argumentCaptor.capture(), any(ApiCallContext.class));
Expand All @@ -396,4 +450,35 @@ public static UnaryCallSettings<Integer, Integer> createSettings(
.setRetrySettings(retrySettings)
.build();
}

private ApiTracerFactory getApiTracerFactory() {
ApiTracer tracer =
new BaseApiTracer() {
@Override
public void attemptFailed(Throwable error, Duration delay) {
tracerAttemptsFailed.incrementAndGet();
super.attemptFailed(error, delay);
}

@Override
public void attemptStarted(int attemptNumber) {
tracerAttempts.incrementAndGet();
super.attemptStarted(attemptNumber);
}

@Override
public void operationFailed(Throwable error) {
tracerOperationFailed.set(true);
super.operationFailed(error);
}

@Override
public void attemptFailedRetriesExhausted(Throwable error) {
tracerFailedRetriesExhausted.set(true);
super.attemptFailedRetriesExhausted(error);
}
};
ApiTracerFactory tracerFactory = (parent, spanName, operationType) -> tracer;
return tracerFactory;
}
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
}
Loading