Implement otel retry metrics #12064
Implement otel retry metrics #12064
Conversation
opentelemetry/src/main/java/io/grpc/opentelemetry/internal/OpenTelemetryConstants.java
Outdated
Show resolved
Hide resolved
opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java
Show resolved
Hide resolved
| @@ -64,8 +65,8 @@ public Stopwatch get() { | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
the gRFC has had recent updates, waiting for it to get merged before doing further changes...
There was a problem hiding this comment.
Yeah, there had been discussions yesterday about the recent changes to the retry metrics and whether they helped and such. The implementation should be mostly the same, but it isn't probably worth chasing the gRFC for the moment.
AgraVator
force-pushed
the
implement-otel-retry-metric
branch
from
a7a16ce to
3529769
Compare
AgraVator
force-pushed
the
implement-otel-retry-metric
branch
from
3529769 to
bd69ed5
Compare
| long hedgesPerCall = 0; | ||
| long attempts = hedgedAttemptsPerCall.get(); | ||
| if (attempts > 0) { | ||
| hedgesPerCall = attempts - 1; | ||
| } |
There was a problem hiding this comment.
should hedges be subtracted by 1 ?
There was a problem hiding this comment.
Hedged attempts don't include the original attempt so we shouldn't subtract.
| module.resource.clientCallDurationCounter() | ||
| .record(callLatencyNanos * SECONDS_PER_NANO, attribute); | ||
| module.resource.clientCallDurationCounter().record( | ||
| callLatencyNanos * SECONDS_PER_NANO, |
There was a problem hiding this comment.
callLatencyNanos is already in nanos.
There was a problem hiding this comment.
yep, nanos * seconds / nanos = seconds (duration unit is "s")
There was a problem hiding this comment.
This comment is not addressed.
opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryMetricsModule.java
Outdated
Show resolved
Hide resolved
opentelemetry/src/main/java/io/grpc/opentelemetry/OpenTelemetryMetricsModule.java
Show resolved
Hide resolved
| Optional<MetricData> metric = openTelemetryTesting.getMetrics().stream() | ||
| .filter(m -> m.getName().equals(metricName)) | ||
| .findFirst(); | ||
| if (metric.isPresent()) { |
There was a problem hiding this comment.
When will this metric not be present for the operations done by the tested on the tracer?
There was a problem hiding this comment.
Whenever they are not enabled or have default values
There was a problem hiding this comment.
As this is test code, it is risky to have the assertion inside an if statement. If the absence of the metric is not expected, then it should not cause the test to pass.
| module.resource.clientCallDurationCounter() | ||
| .record(callLatencyNanos * SECONDS_PER_NANO, attribute); | ||
| module.resource.clientCallDurationCounter().record( | ||
| callLatencyNanos * SECONDS_PER_NANO, |
There was a problem hiding this comment.
yep, nanos * seconds / nanos = seconds (duration unit is "s")
| Optional<MetricData> metric = openTelemetryTesting.getMetrics().stream() | ||
| .filter(m -> m.getName().equals(metricName)) | ||
| .findFirst(); | ||
| if (metric.isPresent()) { |
There was a problem hiding this comment.
Whenever they are not enabled or have default values
| if (info.isTransparentRetry()) { | ||
| transparentRetriesPerCall.incrementAndGet(); | ||
| } else if (info.isHedging()) { | ||
| hedgedAttemptsPerCall.incrementAndGet(); | ||
| } else { | ||
| attemptsPerCall.incrementAndGet(); |
There was a problem hiding this comment.
Should this not assume them to be mutually exclusive ?
implements A96