fix: Populate method level attributes in metrics recording#4149
fix: Populate method level attributes in metrics recording#4149
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 enhances the Golden Signals metrics recording by introducing the capability to incorporate method-level attributes into the tracing context. This change allows for more precise and detailed metrics by merging client-specific and method-specific tracing information, ultimately providing a richer dataset for observability without altering the core metrics collection mechanism. 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 correctly implements the population of method-level attributes for metrics recording by overriding newTracer to merge client-level and method-level ApiTracerContext. The changes look good overall. I've added a few suggestions to improve code maintainability and test robustness by reducing code duplication, improving a test name for clarity, and making a test assertion stronger.
| if (metricsRecorder == null) { | ||
| // This should never happen, in case it happens, create a no-op api tracer to not block | ||
| // regular requests. | ||
| return new BaseApiTracer(); | ||
| } |
There was a problem hiding this comment.
This null check for metricsRecorder is duplicated from the other newTracer method (lines 56-59). To improve maintainability and avoid code duplication, consider extracting this logic into a private helper method. For example:
private ApiTracer createTracerWithContext(ApiTracerContext context) {
if (metricsRecorder == null) {
// This should never happen...
return new BaseApiTracer();
}
return new GoldenSignalsMetricsTracer(metricsRecorder, context);
}Then both newTracer methods can call this helper.
| ApiTracer actual = tracerFactory.newTracer(mock(ApiTracer.class), methodLevelTracerContext); | ||
|
|
||
| assertThat(actual).isInstanceOf(GoldenSignalsMetricsTracer.class); |
There was a problem hiding this comment.
This test correctly checks that a GoldenSignalsMetricsTracer is created, but it doesn't verify that the merge method was actually called on the clientLevelTracerContext. To make the test more robust, you should add a verification step using Mockito.verify().
You'll also need to add the following import:
import static org.mockito.Mockito.verify;
ApiTracer actual = tracerFactory.newTracer(mock(ApiTracer.class), methodLevelTracerContext);
verify(clientLevelTracerContext).merge(methodLevelTracerContext);
assertThat(actual).isInstanceOf(GoldenSignalsMetricsTracer.class);| } | ||
|
|
||
| @Test | ||
| void newTracer1_createsBaseTracer_ifMetricsRecorderIsNull() { |
There was a problem hiding this comment.
The test name newTracer1_createsBaseTracer_ifMetricsRecorderIsNull is a bit confusing due to the 1 suffix. To improve clarity and distinguish it from the existing test for the other newTracer overload (newTracer_createsBaseTracer_ifMetricsRecorderIsNull), consider a more descriptive name like newTracerWithContext_createsBaseTracer_ifMetricsRecorderIsNull.
void newTracerWithContext_createsBaseTracer_ifMetricsRecorderIsNull() {
|
|
2 participants
Populate method level attributes in metrics recording. This is done by overriding
newTracer(ApiTracer, ApiTracerContext)and merge the method level ApiTracerContext with the client level ApiTracerContext.