-
Notifications
You must be signed in to change notification settings - Fork 72
feat(observability): introduce minimal tracing implementation #4105
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
base: main
Are you sure you want to change the base?
Conversation
This reverts commit b7b9e31.
Summary of ChangesHello @diegomarquezp, 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 capabilities of GAX by integrating a robust tracing system based on OpenTelemetry. The design emphasizes extensibility and clear separation of concerns, allowing GAX to record detailed RPC lifecycle events as spans and attributes. It introduces a mechanism to automatically enrich these traces with contextual information, providing deeper insights into client-server interactions. 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
|
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.
Code Review
This pull request introduces a new OpenTelemetry-based tracing system into GAX. Key changes include adding ApiTracerContext to provide endpoint information to tracers, enhancing EndpointContext to extract and store server addresses, and implementing TracingRecorder, TracingTracer, and TracingTracerFactory to integrate OpenTelemetry for operation and attempt spans. Review comments highlight the need to address thread-safety in TracingTracer by using ConcurrentHashMap for attributes, improve the robustness of EndpointContext's server address parsing using java.net.URL, remove a potentially problematic default implementation in TracingRecorder to enforce explicit parent span handling, standardize span naming conventions in TracingTracerFactory to align with OpenTelemetry, and correct the copyright year across several new files.
gax-java/gax/src/main/java/com/google/api/gax/tracing/TracingTracer.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/TraceRecorder.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/OpenTelemetryTraceRecorder.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/TracingRecorder.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/AppCentricTracerFactory.java
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/OpenTelemetryTraceRecorderTest.java
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/AppCentricTracerFactoryTest.java
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/tracing/AppCentricTracerTest.java
Show resolved
Hide resolved
java-showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelTracing.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/AppCentricTracerFactory.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/TracingTracer.java
Outdated
Show resolved
Hide resolved
| return () -> {}; | ||
| } | ||
|
|
||
| interface SpanHandle { |
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.
Do we have to create a separate interface? Can this be flattened?
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.
Discussed offline. We still need a reference to the Span created earlier in the request. Rename this to something like GaxSpan that wraps Span from Opentelemetry.
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.
Renamed to GaxSpan
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.
After renaming some classes to AppCentric* and since other classes are note prefixed with Gax*, a name like TraceSpan to indicate "it's the span of a trace" could work for our purposes. WDYT?
| } | ||
|
|
||
| @Override | ||
| public SpanHandle startSpan(String name, Map<String, String> attributes) { |
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.
Are there any attributes specific to the start of Span? If not, I think we may want to make them a field of the tracer, instead of always passing in as a parameter.
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.
resend_count may be the only attribute specific to the start of the span.
I think a more accurate name here would be createSpan to make more clear the need of the attributes field. Starting the span is more of a detail of the recorder implementation.
A field of the tracer
TracingTracer keeps most attributes as a field, and this recorder class is provided by the user. I think due to resend_count we may want to keep the attributes as a parameter.
gax-java/gax/src/main/java/com/google/api/gax/tracing/TracingRecorder.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/TracingRecorder.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/OpenTelemetryTracingRecorder.java
Outdated
Show resolved
Hide resolved
- Rename SpanHandle to GaxSpan - Simplify adding op and attempt attributes - Use HashMap instead of ConcurrentHashMap - Remove error handling (for now)
|
/gemini review |
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.
Code Review
This pull request introduces a new tracing mechanism based on OpenTelemetry, abstracting the implementation behind a TraceRecorder interface. While the integration tests provide good coverage, a significant thread-safety issue was identified in the AppCentricTracer class. A shared HashMap is modified concurrently in the constructor, which could lead to map corruption and Denial of Service in multi-threaded environments. Additionally, there is a potential bug in endpoint parsing for IPv6 addresses.
gax-java/gax/src/main/java/com/google/api/gax/tracing/AppCentricTracer.java
Outdated
Show resolved
Hide resolved
…icTracer.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…om/googleapis/sdk-platform-java into observability/initial-tracing-impl
|
/gemini review |
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.
Code Review
This pull request introduces a new tracing mechanism based on OpenTelemetry, which is a great addition for observability. However, a security concern has been identified regarding how server addresses are parsed for tracing attributes. The current implementation may leak sensitive information (such as credentials or tokens) from the endpoint URL into the tracing system if the URL contains userinfo, paths, or query parameters. A suggestion has been provided to improve the parsing logic to strip these sensitive components. Additionally, feedback has been provided to improve consistency, specifically regarding span naming conventions across different span types and enhancing integration tests for consistent attribute verification across different transport protocols.
gax-java/gax/src/main/java/com/google/api/gax/tracing/AppCentricTracerFactory.java
Show resolved
Hide resolved
java-showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelTracing.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/AppCentricTracer.java
Outdated
Show resolved
Hide resolved
|
|



Summary
This PR introduces a new tracing mechanism in GAX that allows recording traces using OpenTelemetry. It provides a way of recording spans and attributes, following the existing
ApiTracerclass pattern with a few tracing-specific additions. The implementation is meant to be extensible to support other implementations.New Classes
TracingRecorder: An interface for recording spans and attributes; can be implemented by observability frameworks.OpenTelemetryTracingRecorder: An implementation ofTracingRecorderthat uses the OpenTelemetry API.AppCentricTracer: AnApiTracerimplementation that delegates span management to aTracingRecorder.AppCentricTracerFactory: A factory for creatingTracingTracerinstances.ApiTracerContext: A context object that carries information (likeEndpointContext's server address property) used to infer common attributes for all tracers.TraceSpan: A handle returned byTracingRecorderto manage the lifecycle of a specific span (ending it, recording errors, or setting attributes).Approach
Connecting Tracer with Recorder
The implementation aims to decouple
AppCentricTracerfromTracingRecorder. When a tracer starts an operation or an attempt, it requests aTraceSpanfrom the recorder. This handle allows the tracer to update the span (e.g., adding attributes or recording errors) to keepAppCentricTracerseparated from specific recorder implementations (like OpenTelemetry'sSpanobject).Attribute Inference via
ApiTracerContextTo provide a source of Span Attributes that are common to all operations, we introduced
ApiTracerContext. This context is passed to theApiTracerFactoryand contains information such as serverAddress (provided byEndpointContext). It is operated byClientContext.Initially, only
serverAddressis contained in this class and it's meant to obtain theserver.addressattribute.Integration Tests
A new integration test,
ITOtelTracing, was added to thejava-showcasemodule:server.addressandgcp.client.language).Note on java-bigtable downstream check
Since SkipTrailersTest mocks the tracer factory, the
EndpointContextcall toapiTracerFactory.withContext()returns a null factory, causing a null pointer exception when building the client context.We expect the test to be adjusted with this change with the next release.
Confirmation in Cloud Trace