Implement otel retry metrics #12064
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,7 @@ | |
| */ | ||
| final class OpenTelemetryMetricsModule { | ||
| private static final Logger logger = Logger.getLogger(OpenTelemetryMetricsModule.class.getName()); | ||
| private static final double NANOS_PER_SEC = 1_000_000_000.0; | ||
|
There was a problem hiding this comment. SECONDS_PER_NANO already exists. Use multiplication instead of division. |
||
| public static final ImmutableSet<String> DEFAULT_PER_CALL_METRICS_SET = | ||
| ImmutableSet.of( | ||
| "grpc.client.attempt.started", | ||
|
|
@@ -292,9 +293,12 @@ static final class CallAttemptsTracerFactory extends ClientStreamTracer.Factory | |
| private final String fullMethodName; | ||
| private final List<OpenTelemetryPlugin.ClientCallPlugin> callPlugins; | ||
| private Status status; | ||
| private long retryDelayNanos; | ||
| private long callLatencyNanos; | ||
| private final Object lock = new Object(); | ||
| private final AtomicLong attemptsPerCall = new AtomicLong(); | ||
| private final AtomicLong hedgedAttemptsPerCall = new AtomicLong(); | ||
| private final AtomicLong transparentRetriesPerCall = new AtomicLong(); | ||
| @GuardedBy("lock") | ||
| private int activeStreams; | ||
| @GuardedBy("lock") | ||
|
|
@@ -331,6 +335,7 @@ public ClientStreamTracer newClientStreamTracer(StreamInfo info, Metadata metada | |
| } | ||
| if (++activeStreams == 1 && attemptStopwatch.isRunning()) { | ||
| attemptStopwatch.stop(); | ||
|
There was a problem hiding this comment. This attemptStopwatch looks to be for There was a problem hiding this comment. CallAttemptsTracerFactory.attemptEnded is for recording a stream ended (StatsTraceContext.streamClosed calls it), so for the first attempt and each retry attempt end the attemptStopWatch is started, so it does measure the time between stream attempts, not per call. But there is a problem. RetriableStream creates its own anonymous tracerFactory and not take the one put by I have a question. A call can only have 1 stream, and the only way the activeStreams can be > 1 is if the tracer factory is shared between calls (and that would mess up the calculation for time between stream attempts anyway). The way the OpenTelemetryMetricsModule.MetricsClientInterceptor creates the factory though is per call, not shared between calls. So we we can never have > 1 streams and there should be no need for synchronization either. There was a problem hiding this comment. What I said about RetriableStream's anonymous tracerFactory is not a problem, StatsTraceContext.streamClosed iterates over all tracers and calls stream closed on each of them. |
||
| retryDelayNanos = attemptStopwatch.elapsed(TimeUnit.NANOSECONDS); | ||
|
There was a problem hiding this comment. I can't tell how |
||
| } | ||
| } | ||
| // Skip recording for the first time, since it is already recorded in | ||
|
|
@@ -344,7 +349,11 @@ public ClientStreamTracer newClientStreamTracer(StreamInfo info, Metadata metada | |
| module.resource.clientAttemptCountCounter().add(1, attribute); | ||
| } | ||
| } | ||
| if (info.isTransparentRetry()) { | ||
| transparentRetriesPerCall.incrementAndGet(); | ||
| } else if (info.isHedging()) { | ||
| hedgedAttemptsPerCall.incrementAndGet(); | ||
| } else { | ||
| attemptsPerCall.incrementAndGet(); | ||
|
Comment on lines
+352
to
357
There was a problem hiding this comment. Should this not assume them to be mutually exclusive ? |
||
| } | ||
| return newClientTracer(info); | ||
|
|
@@ -407,14 +416,53 @@ void recordFinishedCall() { | |
| tracer.recordFinishedAttempt(); | ||
| } | ||
| callLatencyNanos = callStopWatch.elapsed(TimeUnit.NANOSECONDS); | ||
|
|
||
| // Base attributes | ||
| io.opentelemetry.api.common.Attributes baseAttributes = | ||
| io.opentelemetry.api.common.Attributes.of( | ||
| METHOD_KEY, fullMethodName, | ||
| TARGET_KEY, target | ||
| ); | ||
|
|
||
| // Duration | ||
| if (module.resource.clientCallDurationCounter() != null) { | ||
| module.resource.clientCallDurationCounter().record( | ||
| callLatencyNanos * SECONDS_PER_NANO, | ||
kannanjgithub marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| baseAttributes.toBuilder() | ||
| .put(STATUS_KEY, status.getCode().toString()) | ||
| .build() | ||
| ); | ||
| } | ||
|
|
||
| // Retry counts | ||
| if (module.resource.clientCallRetriesCounter() != null) { | ||
| long retriesPerCall = attemptsPerCall.get() - 1 >= 0 ? attemptsPerCall.get() - 1 : 0; | ||
|
There was a problem hiding this comment.
|
||
| if (retriesPerCall > 0) { | ||
| module.resource.clientCallRetriesCounter().record(retriesPerCall, baseAttributes); | ||
kannanjgithub marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| // Hedge counts | ||
| if (module.resource.clientCallHedgesCounter() != null) { | ||
| if (hedgedAttemptsPerCall.get() > 0) { | ||
|
There was a problem hiding this comment. Read atomics ( |
||
| module.resource.clientCallHedgesCounter() | ||
| .record(hedgedAttemptsPerCall.get(), baseAttributes); | ||
| } | ||
| } | ||
|
|
||
| // Transparent Retry counts | ||
| if (module.resource.clientCallTransparentRetriesCounter() != null | ||
| && transparentRetriesPerCall.get() > 0) { | ||
| module.resource.clientCallTransparentRetriesCounter().record( | ||
| transparentRetriesPerCall.get(), baseAttributes); | ||
| } | ||
|
|
||
| // Retry delay | ||
| if (module.resource.clientCallRetryDelayCounter() != null) { | ||
| module.resource.clientCallRetryDelayCounter().record( | ||
| retryDelayNanos / NANOS_PER_SEC, | ||
| baseAttributes | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isHedgingis just whether there is a hedging policy. It doesn't mean this particular stream is a hedge.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, are you suggesting to change the var name to better reflect the same ?