gapic-generator-java icon indicating copy to clipboard operation
gapic-generator-java copied to clipboard

HttpJson Implementation is recording Operation(Success|Fail) metric before retries are attempted

Open lqiu96 opened this issue 1 year ago • 0 comments

Discovered in the Otel Draft PR: https://github.com/googleapis/sdk-platform-java/pull/2500

Issue

For Otel, this is causing the number of attempts to always be recorded as 1 instead of the number of retries actually attempted. It does not impact retries being invoked, but will record incorrect metrics.

Steps for creating a UnaryCallable (1 is the inner most callable and 5 is the last callable invoked):

HttpJson Echo Unary:

  1. HttpJsonDirectCallable
  2. HttpJsonUnaryRequestParamCallable (optional -- created if params exist)
  3. TracedUnaryCallable
  4. HttpJsonExceptionCallable
  5. RetryingCallable

gRPC Echo Unary:

  1. GrpcDirectCallable
  2. GrpcUnaryRequestParamCallable (optional -- created if params exist)
  3. GrpcExceptionCallable
  4. RetryingCallable
  5. TracedUnaryCallable

TracedUnaryCallable

Inside the TracedUnaryCallable, the callback is invoked once in innerCallable has been invoked. The callback implementation is the TraceFinisher.

From the implementation, it'll record an operation success or failure, which is intended to invoked after all the retries are exhausted (as the retries are attempts).

Possible Solution

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

Concerns

Would this impact existing behavior? Do we need to do this for the other Callable types (StreamingCallables?)

lqiu96 avatar Feb 26 '24 18:02 lqiu96