Skip to content

Commit

Permalink
fix: httpjson callables to trace attempts (started, failed) (#3300)
Browse files Browse the repository at this point in the history
Fixes #2503 

### Approach
This PR uses the approach suggested by the same bug.
> We should move creating the TracedUnaryCallable to the last step for
HttpJson. This would require refactoring this file:
https://github.com/googleapis/sdk-platform-java/blob/7902a41c87240d607179d07c28cce462ea135c5f/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallableFactory.java

### Confirmation that it works
The confirmation that it works is in the modified tests [(e.g.
`retry()`)](https://github.com/googleapis/sdk-platform-java/blob/1edf55754d1f602a3bf70b3a44d1c42689ae961b/gax-java/gax-httpjson/src/test/java/com/google/api/gax/httpjson/RetryingTest.java#L158-L159).
When using the proposed test version against the version of
`HttpJsonCallableFactory` from the main branch, it will fail because it
will not record attempts started or failed whatsoever:


![image](https://github.com/user-attachments/assets/1cb89d0c-7f34-4b1c-93a5-25f2112040f7)

The image above shows all tests having failed when using the production
files from `main`, implying that no tracer attempts are recorded for any
of the tests.
  • Loading branch information
diegomarquezp authored Nov 13, 2024
1 parent 4c5a43c commit 15a64ee
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 17 deletions.
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
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import com.google.api.core.ApiFutures;
import com.google.api.gax.core.FakeApiClock;
import com.google.api.gax.core.RecordingScheduler;
import com.google.api.gax.httpjson.testing.TestApiTracerFactory;
import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.rpc.ApiCallContext;
import com.google.api.gax.rpc.ApiException;
Expand Down Expand Up @@ -80,6 +81,7 @@ class RetryingTest {

private final Integer initialRequest = 1;
private final Integer modifiedRequest = 0;
private TestApiTracerFactory tracerFactory;

private final HttpJsonCallSettings<Integer, Integer> httpJsonCallSettings =
HttpJsonCallSettings.<Integer, Integer>newBuilder()
Expand Down Expand Up @@ -115,8 +117,11 @@ class RetryingTest {
void resetClock() {
fakeClock = new FakeApiClock(System.nanoTime());
executor = RecordingScheduler.create(fakeClock);
tracerFactory = new TestApiTracerFactory();
clientContext =
ClientContext.newBuilder()
// we use a custom tracer to confirm whether the retrials are being recorded.
.setTracerFactory(tracerFactory)
.setExecutor(executor)
.setClock(fakeClock)
.setDefaultCallContext(HttpJsonCallContext.createDefault())
Expand All @@ -130,6 +135,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 +149,9 @@ void retry() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThat(callable.call(initialRequest)).isEqualTo(2);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isEqualTo(3);
assertThat(tracerFactory.getInstance().getAttemptsStarted().get()).isEqualTo(4);
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isFalse();

// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
Expand Down Expand Up @@ -180,6 +189,9 @@ void retryTotalTimeoutExceeded() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThrows(ApiException.class, () -> callable.call(initialRequest));
assertThat(tracerFactory.getInstance().getAttemptsStarted().get()).isEqualTo(1);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isEqualTo(0);
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isFalse();
// 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 +212,9 @@ void retryMaxAttemptsExceeded() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThrows(ApiException.class, () -> callable.call(initialRequest));
assertThat(tracerFactory.getInstance().getAttemptsStarted().get()).isEqualTo(2);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isEqualTo(2);
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isTrue();
// 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 +235,9 @@ void retryWithinMaxAttempts() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThat(callable.call(initialRequest)).isEqualTo(2);
assertThat(tracerFactory.getInstance().getAttemptsStarted().get()).isEqualTo(3);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isEqualTo(2);
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isFalse();
// 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 +264,9 @@ void retryOnStatusUnknown() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
assertThat(callable.call(initialRequest)).isEqualTo(2);
assertThat(tracerFactory.getInstance().getAttemptsStarted().get()).isEqualTo(4);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isEqualTo(3);
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isFalse();
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
verify(callInt, atLeastOnce()).futureCall(argumentCaptor.capture(), any(ApiCallContext.class));
Expand All @@ -264,6 +285,9 @@ void retryOnUnexpectedException() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
ApiException exception = assertThrows(ApiException.class, () -> callable.call(initialRequest));
assertThat(tracerFactory.getInstance().getAttemptsStarted().get()).isEqualTo(1);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isEqualTo(0);
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isFalse();
assertThat(exception).hasCauseThat().isSameInstanceAs(throwable);
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
Expand Down Expand Up @@ -293,6 +317,9 @@ void retryNoRecover() {
HttpJsonCallableFactory.createUnaryCallable(
callInt, callSettings, httpJsonCallSettings, clientContext);
ApiException exception = assertThrows(ApiException.class, () -> callable.call(initialRequest));
assertThat(tracerFactory.getInstance().getAttemptsStarted().get()).isEqualTo(1);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isEqualTo(0);
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isFalse();
assertThat(exception).isSameInstanceAs(apiException);
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
Expand All @@ -319,6 +346,10 @@ void retryKeepFailing() {

UncheckedExecutionException exception =
assertThrows(UncheckedExecutionException.class, () -> Futures.getUnchecked(future));
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isGreaterThan(0);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get())
.isEqualTo(tracerFactory.getInstance().getAttemptsStarted().get());
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isTrue();
assertThat(exception).hasCauseThat().isInstanceOf(ApiException.class);
assertThat(exception).hasCauseThat().hasMessageThat().contains("Unavailable");
// Capture the argument passed to futureCall
Expand Down Expand Up @@ -359,6 +390,9 @@ void testKnownStatusCode() {
callInt, callSettings, httpJsonCallSettings, clientContext);
ApiException exception =
assertThrows(FailedPreconditionException.class, () -> callable.call(initialRequest));
assertThat(tracerFactory.getInstance().getAttemptsStarted().get()).isEqualTo(1);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isEqualTo(0);
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isFalse();
assertThat(exception.getStatusCode().getTransportCode())
.isEqualTo(HTTP_CODE_PRECONDITION_FAILED);
assertThat(exception).hasMessageThat().contains("precondition failed");
Expand All @@ -383,6 +417,9 @@ void testUnknownStatusCode() {
UnknownException exception =
assertThrows(UnknownException.class, () -> callable.call(initialRequest));
assertThat(exception).hasMessageThat().isEqualTo("java.lang.RuntimeException: unknown");
assertThat(tracerFactory.getInstance().getAttemptsStarted().get()).isEqualTo(1);
assertThat(tracerFactory.getInstance().getAttemptsFailed().get()).isEqualTo(0);
assertThat(tracerFactory.getInstance().getRetriesExhausted().get()).isFalse();
// Capture the argument passed to futureCall
ArgumentCaptor<Integer> argumentCaptor = ArgumentCaptor.forClass(Integer.class);
verify(callInt, atLeastOnce()).futureCall(argumentCaptor.capture(), any(ApiCallContext.class));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Copyright 2024 Google LLC
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
* * Neither the name of Google LLC nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package com.google.api.gax.httpjson.testing;

import com.google.api.gax.tracing.ApiTracer;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import org.threeten.bp.Duration;

/**
* Test tracer that keeps count of different events. See {@link TestApiTracerFactory} for more
* details.
*/
public class TestApiTracer implements ApiTracer {

private final AtomicInteger attemptsStarted = new AtomicInteger();
private final AtomicInteger attemptsFailed = new AtomicInteger();
private final AtomicBoolean retriesExhausted = new AtomicBoolean(false);

public TestApiTracer() {}

public AtomicInteger getAttemptsStarted() {
return attemptsStarted;
}

public AtomicInteger getAttemptsFailed() {
return attemptsFailed;
}

public AtomicBoolean getRetriesExhausted() {
return retriesExhausted;
}

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

@Override
public void attemptStarted(Object request, int attemptNumber) {
attemptsStarted.incrementAndGet();
}

@Override
public void attemptFailed(Throwable error, Duration delay) {
attemptsFailed.incrementAndGet();
}

@Override
public void attemptFailedRetriesExhausted(Throwable error) {
attemptsFailed.incrementAndGet();
retriesExhausted.set(true);
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright 2024 Google LLC
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
* * Neither the name of Google LLC nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package com.google.api.gax.httpjson.testing;

import com.google.api.gax.tracing.ApiTracer;
import com.google.api.gax.tracing.ApiTracerFactory;
import com.google.api.gax.tracing.SpanName;

/**
* Produces a {@link TestApiTracer}, which keeps count of the attempts made and attempts
* made-and-failed. It also keeps count of the operations failed and when the retries have been
* exhausted.
*/
public class TestApiTracerFactory implements ApiTracerFactory {
private final TestApiTracer instance = new TestApiTracer();

public TestApiTracer getInstance() {
return instance;
}

@Override
public ApiTracer newTracer(ApiTracer parent, SpanName spanName, OperationType operationType) {
return instance;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;

/**
Expand Down Expand Up @@ -320,12 +319,10 @@ void testGrpc_operationSucceeded_recordsMetrics() throws InterruptedException {
verifyStatusAttribute(actualMetricDataList, statusCountList);
}

@Disabled("https://github.com/googleapis/sdk-platform-java/issues/2503")
@Test
void testHttpJson_operationSucceeded_recordsMetrics() throws InterruptedException {
int attemptCount = 1;
EchoRequest echoRequest =
EchoRequest.newBuilder().setContent("test_http_operation_succeeded").build();
EchoRequest echoRequest = EchoRequest.newBuilder().setContent("content").build();
httpClient.echo(echoRequest);

List<MetricData> actualMetricDataList = getMetricDataList();
Expand Down Expand Up @@ -373,7 +370,6 @@ void testGrpc_operationCancelled_recordsMetrics() throws Exception {
verifyStatusAttribute(actualMetricDataList, statusCountList);
}

@Disabled("https://github.com/googleapis/sdk-platform-java/issues/2503")
@Test
void testHttpJson_operationCancelled_recordsMetrics() throws Exception {
int attemptCount = 1;
Expand Down Expand Up @@ -430,7 +426,6 @@ void testGrpc_operationFailed_recordsMetrics() throws InterruptedException {
verifyStatusAttribute(actualMetricDataList, statusCountList);
}

@Disabled("https://github.com/googleapis/sdk-platform-java/issues/2503")
@Test
void testHttpJson_operationFailed_recordsMetrics() throws InterruptedException {
int attemptCount = 1;
Expand Down Expand Up @@ -526,7 +521,6 @@ void testGrpc_attemptFailedRetriesExhausted_recordsMetrics() throws Exception {
grpcClient.awaitTermination(TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS);
}

@Disabled("https://github.com/googleapis/sdk-platform-java/issues/2503")
@Test
void testHttpJson_attemptFailedRetriesExhausted_recordsMetrics() throws Exception {
int attemptCount = 3;
Expand Down Expand Up @@ -621,7 +615,6 @@ void testGrpc_attemptPermanentFailure_recordsMetrics() throws InterruptedExcepti
verifyStatusAttribute(actualMetricDataList, statusCountList);
}

@Disabled("https://github.com/googleapis/sdk-platform-java/issues/2503")
@Test
void testHttpJson_attemptPermanentFailure_recordsMetrics() throws InterruptedException {
int attemptCount = 1;
Expand Down Expand Up @@ -722,7 +715,6 @@ void testGrpc_multipleFailedAttempts_successfulOperation() throws Exception {
grpcClient.awaitTermination(TestClientInitializer.AWAIT_TERMINATION_SECONDS, TimeUnit.SECONDS);
}

@Disabled("https://github.com/googleapis/sdk-platform-java/issues/2503")
@Test
void testHttpJson_multipleFailedAttempts_successfulOperation() throws Exception {
int attemptCount = 3;
Expand All @@ -737,7 +729,7 @@ void testHttpJson_multipleFailedAttempts_successfulOperation() throws Exception

EchoStubSettings.Builder httpJsonEchoSettingsBuilder = EchoStubSettings.newHttpJsonBuilder();
httpJsonEchoSettingsBuilder
.echoSettings()
.blockSettings()
.setRetrySettings(retrySettings)
.setRetryableCodes(ImmutableSet.of(Code.DEADLINE_EXCEEDED));
EchoSettings httpJsonEchoSettings = EchoSettings.create(httpJsonEchoSettingsBuilder.build());
Expand Down Expand Up @@ -771,7 +763,7 @@ void testHttpJson_multipleFailedAttempts_successfulOperation() throws Exception
.setSuccess(BlockResponse.newBuilder().setContent("httpjsonBlockResponse"))
.build();

grpcClient.block(blockRequest);
httpClient.block(blockRequest);

List<MetricData> actualMetricDataList = getMetricDataList();
verifyPointDataSum(actualMetricDataList, attemptCount);
Expand Down

0 comments on commit 15a64ee

Please sign in to comment.