Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new client request duration metric, Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new metric for client request duration, gcp.client.request.duration, implemented through a new GoldenTelemetryMetricsTracer. The changes also include a valuable refactoring that centralizes utility methods like extractStatus and toOtelAttributes into a new ObservabilityUtils class, improving code organization. The new functionality is well-tested. My review includes a couple of suggestions to improve robustness and error handling in the new tracer and its factory, which are still valid. Overall, this is a solid contribution.
| public ApiTracer newTracer(ApiTracer parent, SpanName spanName, OperationType operationType) { | ||
| return new GoldenTelemetryMetricsTracer(openTelemetry, apiTracerContext); | ||
| } |
There was a problem hiding this comment.
The apiTracerContext field is not initialized in the constructor and can be null if newTracer is called before withContext. This would lead to a NullPointerException when apiTracerContext.libraryMetadata() is called inside the GoldenTelemetryMetricsTracer constructor. To make the factory more robust against misuse, please add a null-check for apiTracerContext and throw an IllegalStateException if it hasn't been set.
public ApiTracer newTracer(ApiTracer parent, SpanName spanName, OperationType operationType) {
if (apiTracerContext == null) {
throw new IllegalStateException(
"ApiTracerContext has not been set. Call withContext() before creating a new tracer.");
}
return new GoldenTelemetryMetricsTracer(openTelemetry, apiTracerContext);
}References
- Explicit
ifchecks for null handling are preferred for better readability in simple cases, aligning with the preference overjava.util.Optionalfor such scenarios.
There was a problem hiding this comment.
I think this should not happen with the current setup but +1 for the extra safety "against misuse" a null check brings.
| @Override | ||
| public void operationSucceeded() { | ||
| if (clientRequestFinished.getAndSet(true)) { | ||
| throw new IllegalStateException(); |
There was a problem hiding this comment.
For better diagnostics, the IllegalStateException should include a descriptive message, for example: "Operation has already been completed". This feedback also applies to the exceptions thrown in operationCancelled() (line 92) and operationFailed() (line 102). Consider defining this message as a shared constant for reuse.
| throw new IllegalStateException(); | |
| throw new IllegalStateException("Operation has already been completed"); |
diegomarquezp
left a comment
diegomarquezp
left a comment
There was a problem hiding this comment.
Thanks for the PR.
external/com_google_api_gax_java/gax/src/test/java/com/google/api/gax/tracing/GoldenSignalMetricsTracerTest.java:59: error: cannot find symbol
private InMemoryMetricReader metricReader;
^
symbol: class InMemoryMetricReader
location: class GoldenSignalMetricsTracerTest
external/com_google_api_gax_java/gax/src/test/java/com/google/api/gax/tracing/GoldenSignalMetricsTracerTest.java:133: error: cannot find symbol
private void verifyMetricDataContainsMeterInfo(MetricData metricData) {
I think we need to add an entry to gax-java/dependencies.properties and related BUILD files.
|
|
||
| @Override | ||
| public void operationSucceeded() { | ||
| if (clientRequestFinished.getAndSet(true)) { |
There was a problem hiding this comment.
I remember at some point we mentioned that we don't expect race conditions when operating at attempt level. Specifically, we may process separate attempts in separate threads, but attempts will always be sequential.
Could you explain why we need an atomic boolean in this case?
There was a problem hiding this comment.
Agreed. This is mostly copied from MetricsTracer. I confirmed with @lqiu96 as well that we don't need this.
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| public class GoldenSignalMetricsTracer implements ApiTracer { |
There was a problem hiding this comment.
I think the GoldenSignal prefix may require a justification to represent a direct implementation of the OpenTelemetry API, since the connection between these two may be a bit obscure at least to my understanding.
Maybe we can add a Javadoc in the class to explain its purpose?
There was a problem hiding this comment.
Also, I'd like to reintroduce the discussion of naming in this effort. The Google Cloud Documentaiton for Observability defines this effort as Application Centric Observability. AppCentric is then a valid public-facing convention.
There was a problem hiding this comment.
Maybe we can add a Javadoc in the class to explain its purpose?
Added. Also I extracted the metric recording logic to a new class GoldenSignalMetricsRecorder to
- Wrap the OpenTelemetry recording logic.
- Make sure the meter and histogram are only created once per client.
| "Operation has already been completed"; | ||
|
|
||
| private final Stopwatch clientRequestTimer = Stopwatch.createStarted(); | ||
| private final AtomicBoolean clientRequestFinished; |
There was a problem hiding this comment.
A nit on public vs internal semantics: public documentation refers to operations as "client requests", but our implementation historically defines these as operations. Maybe operationFinished is more consistent with the tracing structure for such a private variable?
| public ApiTracer newTracer(ApiTracer parent, SpanName spanName, OperationType operationType) { | ||
| return new GoldenTelemetryMetricsTracer(openTelemetry, apiTracerContext); | ||
| } |
There was a problem hiding this comment.
I think this should not happen with the current setup but +1 for the extra safety "against misuse" a null check brings.
| } | ||
|
|
||
| @Test | ||
| void operationSucceeded_recordsDuration() { |
There was a problem hiding this comment.
What if we separate operationSucceeded_recordsDuration from something like operationSucceeded_recordsOKStatus?
There was a problem hiding this comment.
Great idea! Updated.
I also extracted the unit testing of meter to the recorder's unit tests.
| clientRequestTimer.elapsed(TimeUnit.SECONDS), toOtelAttributes(attributes)); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Note: I believe showcase cannot emulate all error behavior out of the box. I'm planning to add unit tests with a local server in order to emulate possible error codes and other behavior.
There was a problem hiding this comment.
Thanks, that's a great idea! On the other hand, I would still prioritize adding more attributes. Because we are not changing any existing functionality, we are relying on them to add traces.
| } | ||
| attributes.put(RPC_RESPONSE_STATUS_ATTRIBUTE, StatusCode.Code.OK.toString()); | ||
| clientRequestDurationRecorder.record( | ||
| clientRequestTimer.elapsed(TimeUnit.SECONDS), toOtelAttributes(attributes)); |
There was a problem hiding this comment.
elapsed returns a long, we need to do something to make this a double fraction like:
elapsed(NANOSECONDS)/1_000_000_000.0
Otherwise we won't record things below 1s
| meter | ||
| .histogramBuilder(CLIENT_REQUEST_DURATION_METRIC_NAME) | ||
| .setDescription(CLIENT_REQUEST_DURATION_METRIC_DESCRIPTION) | ||
| .setUnit("s") |
There was a problem hiding this comment.
Needs .setExplicitBucketBoundariesAdvice()
[0, 0.0001, 0.0005, 0.0010, 0.005, 0.010, 0.050, 0.100, 0.5, 1.0, 5.0, 10.0, 60.0, 300.0, 900.0, 3600.0]
|
|
||
| @Override | ||
| public ApiTracer newTracer(ApiTracer parent, SpanName spanName, OperationType operationType) { | ||
| return new GoldenSignalMetricsTracer(openTelemetry, apiTracerContext); |
There was a problem hiding this comment.
Where do we capture things like rpc.method?
There was a problem hiding this comment.
It will be populated in apiTracerContext. Then the tracer will get it from apiTracerContext and record it.
|
|
3 participants
This PR adds basic implementation for client request duration metric.
The following attributes are also added:
The overall design follows
ApiTracer/ApiTracerFactorypattern, createsGoldenSignalMetricsTracer/GoldenSignalMetricsTracerFactorythat implement ApiTracer/ApiTracerFactory respectively.GoldenSignalMetricsTracerFactoryis expected to be passed in during client initialization.GoldenSignalMetricsTracertracks the lifecycle of a request and calculates the duration of client requests.