-
Notifications
You must be signed in to change notification settings - Fork 55
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.
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
feat: MetricsTracer implementation #2421
Conversation
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerTest.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerTest.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerTest.java
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerTest.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerFactoryTest.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerTest.java
Outdated
Show resolved
Hide resolved
I see. I think given the current interface of MetricsRecorder this makes sense. I wonder if MetricsRecorder should be enhanced with some methods like:
I don't have a list of pro/cons with this, but it seems cleaner (?) or at least makes sense in my head. I think that recording the attempt should occur when the attempt started and not be dependent on getting a result. Given that we need to record the status, maybe that can be done separately from recording the attempt count? This probably doesn't need to be fixed now (or fixed at all), but just throwing some thoughts out. |
That's exactly what came to my mind when I thought about multiple attempts for a successful operation, should we keep a track of attempt_count somehow, with a counter maybe? But then I realized that we are calculating the metrics for each attempt, so keeping a track of number of attempts is irrelevant for us. For every attempt that resulted in something, we just increment the count by 1 with the corresponding attributes. |
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerTest.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerTest.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerTest.java
Outdated
Show resolved
Hide resolved
We don't have to mention OpenTelemetry in the title, as this implementation is framework neutral. Please add more details to the description as well. |
Understood, changed title and added a detailed description. |
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/MetricsTracerFactoryTest.java
Outdated
Show resolved
Hide resolved
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.
LGTM. I left a few nits if you could take a quick look at.
All changed and pushed. thank you. |
Quality Gate failed for 'gapic-generator-java-root'Failed conditions 0.0% Coverage on New Code (required ≥ 80%) |
Quality Gate failed for 'java_showcase_integration_tests'Failed conditions 0.0% Coverage on New Code (required ≥ 80%) |
This PR adds framework-neutral
MetricsTracer
to Gax. This class adds logics during the start and end of an RPC call to calculate the latencies and count the number of RPC calls. Metric recording responsibilities are delegated to theMetricsRecorder
, providing flexibility for diverse observability frameworks.Unit tests are added for every implemented method, including tests for
MetricsTracerFactory
(class responsible for creating one MetricsTacer per operation).