feat(o11y): introduce http.request.method and span name for HTTP requests#4127
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 significantly enhances the observability of HTTP requests within the system by introducing detailed HTTP-specific attributes to tracing spans. It refactors the mechanism for generating span context, allowing for the inclusion of HTTP method and path template, which provides more granular and actionable insights into the performance and behavior of HTTP interactions. Highlights
Changelog
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 observability improvements for HTTP requests by adding the http.request.method attribute to traces and defining a more informative span name format (<HTTP_METHOD> <HTTP_PATH_TEMPLATE>). The changes are well-structured and include corresponding updates to unit and integration tests. However, I've identified a bug in SpanTracerFactory where an incorrect context is used to generate the new span name, which could lead to errors. My review includes a specific comment with a suggested fix for this issue.
gax-java/gax/src/main/java/com/google/api/gax/tracing/SpanTracerFactory.java
Outdated
Show resolved
Hide resolved
…nto observability/tracing-attr/http-method-definition
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the ApiTracerContext to include HTTP method and path template, and updates the SpanTracerFactory to use these values when creating span names for HTTP requests. Specifically, the getApiTracerContext method in HttpJsonCallableFactory is updated to include http method and path template. The newTracer method in SpanTracerFactory is updated to use the HTTP method and path template to construct the span name. The review comments suggest to handle null values for httpMethod and httpPathTemplate more gracefully in SpanTracerFactory and to update the assertion message in ITOtelTracing to reflect the new expected span name.
| attemptSpanName = | ||
| String.format("%s %s", mergedContext.httpMethod(), mergedContext.httpPathTemplate()); |
There was a problem hiding this comment.
Using String.format directly with potentially null values for httpMethod and httpPathTemplate will result in the literal string "null" in the span name. It's better to construct the span name more carefully, handling nulls gracefully and providing a sensible fallback, like the full method name.
String httpMethod = mergedContext.httpMethod() != null ? mergedContext.httpMethod() : "";
String httpPathTemplate =
mergedContext.httpPathTemplate() != null ? mergedContext.httpPathTemplate() : "";
attemptSpanName = (httpMethod + " " + httpPathTemplate).trim();
if (attemptSpanName.isEmpty()) {
attemptSpanName = mergedContext.fullMethodName();
}References
- While
java.util.Optionalcan offer conciseness for null-handling, explicitifchecks may be preferred for better readability in simple cases. The suggested code uses explicit checks for null values, aligning with this preference.
| .filter(span -> span.getName().equals("Echo/Echo/attempt")) | ||
| .filter(span -> span.getName().equals("POST v1beta1/echo:echo")) | ||
| .findFirst() | ||
| .orElseThrow(() -> new AssertionError("Attempt span 'Echo/Echo/attempt' not found")); |
There was a problem hiding this comment.
The assertion message still refers to the old span name Echo/Echo/attempt. It should be updated to reflect the new expected span name POST v1beta1/echo:echo to avoid confusion when the test fails.
| .orElseThrow(() -> new AssertionError("Attempt span 'Echo/Echo/attempt' not found")); | |
| .orElseThrow(() -> new AssertionError("Attempt span 'POST v1beta1/echo:echo' not found")); |
1 participant
This PR introduces enhancements to the observability layer (tracing and metrics) by capturing additional context during RPC calls. Specifically, it adds support for:
http.request.methodfor REST-based transports.gcp.client.repoandgcp.client.artifactto identify the source library of the RPC.Verification Results
http.request.methodis correctly populated for REST calls in Showcase integration tests.