-
Notifications
You must be signed in to change notification settings - Fork 70
chore: [wip] tracing implementation POC #4094
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
…metryTracingRecorder.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…eapis/sdk-platform-java into observability/tracing-noop
…eapis/sdk-platform-java into observability/tracing-noop
…eapis/sdk-platform-java into observability/tracing-noop
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [com.google.cloud:grpc-gcp](https://togithub.com/GoogleCloudPlatform/grpc-gcp-java/tree/master/grpc-gcp) ([source](https://togithub.com/GoogleCloudPlatform/grpc-gcp-java)) | patch | `1.9.0` -> `1.9.1` | | [com.google.errorprone:error_prone_annotations](https://errorprone.info) ([source](https://togithub.com/google/error-prone)) | minor | `2.45.0` -> `2.46.0` | | [com.google.http-client:google-http-client](https://togithub.com/googleapis/google-http-java-client) | patch | `2.0.2` -> `2.0.3` | | [com.google.protobuf:protobuf-java](https://developers.google.com/protocol-buffers/) ([source](https://togithub.com/protocolbuffers/protobuf)) | patch | `4.33.1` -> `4.33.4` | | [commons-codec:commons-codec](https://commons.apache.org/proper/commons-codec/) ([source](https://togithub.com/apache/commons-codec)) | minor | `1.19.0` -> `1.20.0` | | [io.opentelemetry:opentelemetry-bom](https://togithub.com/open-telemetry/opentelemetry-java) | minor | `1.57.0` -> `1.58.0` | | [org.apache.httpcomponents.client5:httpclient5](https://hc.apache.org/) | minor | `5.5.1` -> `5.6` | | [org.apache.httpcomponents.core5:httpcore5](https://hc.apache.org/) | minor | `5.3.6` -> `5.4` | --- ### Release Notes <details> <summary>GoogleCloudPlatform/grpc-gcp-java (com.google.cloud:grpc-gcp)</summary> ### [`v1.9.1`](https://togithub.com/GoogleCloudPlatform/grpc-gcp-java/releases/tag/v1.9.1) [Compare Source](https://togithub.com/GoogleCloudPlatform/grpc-gcp-java/compare/v1.9.0...v1.9.1) #### What's Changed - fix: race for metrics values. Plus fix log metrics when reported via otel callbacks by [@​nimf](https://togithub.com/nimf) in [https://github.com/GoogleCloudPlatform/grpc-gcp-java/pull/228](https://togithub.com/GoogleCloudPlatform/grpc-gcp-java/pull/228) **Full Changelog**: GoogleCloudPlatform/grpc-gcp-java@v1.9.0...v1.9.1 </details> <details> <summary>google/error-prone (com.google.errorprone:error_prone_annotations)</summary> ### [`v2.46.0`](https://togithub.com/google/error-prone/releases/tag/v2.46.0): Error Prone 2.46.0 [Compare Source](https://togithub.com/google/error-prone/compare/v2.45.0...v2.46.0) Changes: - The javac flag `-XDaddTypeAnnotationsToSymbol=true` is now required for Error Prone invocations on JDK 21, to enable the javac fix for [JDK-8225377: type annotations are not visible to javac plugins across compilation boundaries](https://bugs.openjdk.org/browse/JDK-8225377). See [https://github.com/google/error-prone/issues/5426](https://togithub.com/google/error-prone/issues/5426) for details. - Remove deprecated `value` attribute from `@IncompatibleModifiers` and `@RequiredModifiers` ([https://github.com/google/error-prone/issues/2122](https://togithub.com/google/error-prone/issues/2122)) - Error Prone API changes to encapsulate references to internal javac APIs for end position handling (`EndPosTable`, `DiagnosticPosition`) (google/error-prone@5440bb4, google/error-prone@06c2905, google/error-prone@f3915ec) New checks: - [`DuplicateAssertion`](https://errorprone.info/bugpattern/DuplicateAssertion): detect duplicated assertion lines where the argument to `assertThat` is pure - [`IfChainToSwitch`](https://errorprone.info/bugpattern/IfChainToSwitch): suggest converting chains of if-statements into arrow switches - [`ScannerUseDelimiter`](https://errorprone.info/bugpattern/ScannerUseDelimiter): discourage `Scanner.useDelimiter("\\A")` - [`AddNullMarkedToClass`](https://errorprone.info/bugpattern/AddNullMarkedToClass): refactoring to add `@NullMarked` annotation to top level classes </details> <details> <summary>googleapis/google-http-java-client (com.google.http-client:google-http-client)</summary> ### [`v2.0.3`](https://togithub.com/googleapis/google-http-java-client/blob/HEAD/CHANGELOG.md#203-2025-12-19) [Compare Source](https://togithub.com/googleapis/google-http-java-client/compare/v2.0.2...v2.0.3) ##### Bug Fixes - **apache5:** Set connection request timeout on setTimeout ([#​2129](https://togithub.com/googleapis/google-http-java-client/issues/2129)) ([d11d794](https://togithub.com/googleapis/google-http-java-client/commit/d11d794d452a662bddd4402cd5d3895bcbe769f3)) </details> <details> <summary>apache/commons-codec (commons-codec:commons-codec)</summary> ### [`v1.20.0`](https://togithub.com/apache/commons-codec/blob/HEAD/RELEASE-NOTES.txt#Apache-Commons-Codec-1200-Release-Notes) The Apache Commons Codec team is pleased to announce the release of Apache Commons Codec 1.20.0. The Apache Commons Codec component contains encoders and decoders for formats such as Base16, Base32, Base64, digest, and Hexadecimal. In addition to these widely used encoders and decoders, the codec package also maintains a collection of phonetic encoding utilities. This is a feature and maintenance release. Java 8 or later is required. </details> <details> <summary>open-telemetry/opentelemetry-java (io.opentelemetry:opentelemetry-bom)</summary> ### [`v1.58.0`](https://togithub.com/open-telemetry/opentelemetry-java/blob/HEAD/CHANGELOG.md#Version-1580-2026-01-09) [Compare Source](https://togithub.com/open-telemetry/opentelemetry-java/compare/v1.57.0...v1.58.0) ##### API ##### Incubator - Delete GlobalConfigProvider in favor of access via ExtendedOpenTelemetry ([#​7914](https://togithub.com/open-telemetry/opentelemetry-java/pull/7914)) - Add DeclarativeConfigProperties#get method ([#​7923](https://togithub.com/open-telemetry/opentelemetry-java/pull/7923)) - Update ConfigProvider#getInstrumentationConfig response to be non-null ([#​7954](https://togithub.com/open-telemetry/opentelemetry-java/pull/7954)) - Add declarative config utility methods for common operations ([#​7927](https://togithub.com/open-telemetry/opentelemetry-java/pull/7927)) ##### SDK ##### Traces - Implement SDK metrics for trace ([#​7895](https://togithub.com/open-telemetry/opentelemetry-java/pull/7895), [#​7930](https://togithub.com/open-telemetry/opentelemetry-java/pull/7930)) - Emit warning when TraceIdRatioBasedSampler is used as child sampler ([#​7937](https://togithub.com/open-telemetry/opentelemetry-java/pull/7937)) ##### Logs - Implement SDK metrics for logs ([#​7931](https://togithub.com/open-telemetry/opentelemetry-java/pull/7931)) ##### Exporters - Prom exporter update ([#​7934](https://togithub.com/open-telemetry/opentelemetry-java/pull/7934)) ##### Extensions - Declarative config: update to opentelemetry-configuration 1.0.0-rc.3 ([#​7861](https://togithub.com/open-telemetry/opentelemetry-java/pull/7861)) - Declarative config: update jaeger remote sampler to require endpoint, initial_sampler ([#​7943](https://togithub.com/open-telemetry/opentelemetry-java/pull/7943)) - Declarative config: add support for view unit ([#​7942](https://togithub.com/open-telemetry/opentelemetry-java/pull/7942)) - Declarative config: add support for new logger config minimum_severity and trace_based properties ([#​7940](https://togithub.com/open-telemetry/opentelemetry-java/pull/7940)) - Declarative config: add support for composable parent threshold sampler ([#​7941](https://togithub.com/open-telemetry/opentelemetry-java/pull/7941)) - Declarative config: improve pattern for validating and loading SDK extension plugins ([#​7947](https://togithub.com/open-telemetry/opentelemetry-java/pull/7947)) ##### Project tooling - Use develocity build cache in PRs and local builds ([#​7906](https://togithub.com/open-telemetry/opentelemetry-java/pull/7906)) - Configure japicmp classpath to avoid false positives ([#​7945](https://togithub.com/open-telemetry/opentelemetry-java/pull/7945)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
chore: update googleapis commit at Wed Dec 10 02:37:02 UTC 2025
🤖 I have created a release *beep* *boop* --- <details><summary>2.65.0</summary> ## [2.65.0](v2.64.2...v2.65.0) (2026-01-12) ### Features * add org.json:json to third-party-dependencies pom ([#4047](#4047)) ([ffa432e](ffa432e)), closes [#4046](#4046) * remove dependency management of graal-sdk ([#4033](#4033)) ([ad05c34](ad05c34)) ### Bug Fixes * add api_version breadcrumb to client docs ([#4018](#4018)) ([a2b2179](a2b2179)) * Create a single S2AChannelCredentials per application ([#3989](#3989)) ([3758b43](3758b43)) * provide API to share the same background executor for channel po… ([#4030](#4030)) ([178182c](178182c)) ### Dependencies * update dependencies.txt for grpc-gcp to 1.9.0 ([#4025](#4025)) ([b68791d](b68791d)) * update google api dependencies ([#3917](#3917)) ([480cf13](480cf13)) * update google.http-client.version to 2.0.3 ([#4054](#4054)) ([b9a8c89](b9a8c89)) ### Documentation * Fix retry guide link in javadocs ([#4029](#4029)) ([b43f77c](b43f77c)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
[GoogleCredentialsProvider#setScopesToApply](https://github.com/googleapis/sdk-platform-java/blob/b9a8c8924e7b03bca8a97e476abfd012b86f6d45/gax-java/gax/src/main/java/com/google/api/gax/core/GoogleCredentialsProvider.java#L123) would only work if the underlying credentials require the scopes to be set. Update the docs to reflect the intended behavior. Fixes #3738
🤖 I have created a release *beep* *boop* --- <details><summary>2.65.1-SNAPSHOT</summary> ### Updating meta-information for bleeding-edge SNAPSHOT release. </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
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 lays the groundwork for integrating OpenTelemetry tracing into the Highlights
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 proof-of-concept for OpenTelemetry-based tracing in gax-java. The changes include new interfaces for tracing (TracingRecorder, SpanHandle), OpenTelemetry implementations for these interfaces, and integration into the client lifecycle via a new ApiTracerFactory. The overall approach is sound and lays a good foundation.
My review includes a few key points:
- A high-level design suggestion to make the attribute system more flexible by not limiting it to string values. This is crucial for capturing rich, typed trace data.
- A refactoring suggestion to reduce code duplication in the new tracer implementation.
- Several improvements for the integration tests to make them more robust and to address commented-out code that indicates potential issues.
The changes are a good start for this feature.
| SpanHandle startSpan(String name, Map<String, String> attributes); | ||
|
|
||
| /** Starts a span with a parent and returns a handle to manage its lifecycle. */ | ||
| default SpanHandle startSpan(String name, Map<String, String> attributes, SpanHandle parent) { | ||
| return startSpan(name, attributes); | ||
| } | ||
|
|
||
| /** | ||
| * Installs the span into the current thread-local context. | ||
| * | ||
| * @return a scope that must be closed to remove the span from the context. | ||
| */ | ||
| default ApiTracer.Scope inScope(SpanHandle handle) { | ||
| return () -> {}; | ||
| } | ||
|
|
||
| interface SpanHandle { | ||
| void end(); | ||
|
|
||
| void recordError(Throwable error); | ||
|
|
||
| void setAttribute(String key, String value); |
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.
The use of String for attribute values throughout this new tracing API (in startSpan and SpanHandle.setAttribute) is overly restrictive. Tracing attributes are often numeric or boolean, and converting them to strings loses important type information, hindering analysis in tracing backends (e.g., attemptNumber should be an integer).
To make this API more powerful and aligned with OpenTelemetry, I recommend changing attribute values from String to a more flexible type. Some options:
- Use
io.opentelemetry.api.common.Attributes: This is the most idiomatic OTel approach. - Use
Map<String, Object>: A more generic approach if you want to avoid a hard dependency on the OTelAttributesclass in the interface. - Overload
setAttribute: ForSpanHandle, you could add overloads for common types (long,double,boolean).
This change would need to be propagated through ApiTracerFactory, TracingRecorder, and OpenTelemetryTracingTracer.
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.
This is true and a pending task: Otel SemConv specifies other types such as int
| public void attemptFailedDuration(Throwable error, java.time.Duration delay) { | ||
| if (attemptHandle != null) { | ||
| attemptHandle.recordError(error); | ||
| } | ||
| endAttempt(); | ||
| } | ||
|
|
||
| @Override | ||
| public void attemptFailedRetriesExhausted(Throwable error) { | ||
| if (attemptHandle != null) { | ||
| attemptHandle.recordError(error); | ||
| } | ||
| endAttempt(); | ||
| } | ||
|
|
||
| @Override | ||
| public void attemptPermanentFailure(Throwable error) { | ||
| if (attemptHandle != null) { | ||
| attemptHandle.recordError(error); | ||
| } | ||
| endAttempt(); | ||
| } |
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.
The methods attemptFailedDuration, attemptFailedRetriesExhausted, and attemptPermanentFailure contain identical logic for recording an error on the attempt span and ending it. This duplication can be removed by extracting the logic into a private helper method.
For example:
private void failAttempt(Throwable error) {
if (attemptHandle != null) {
attemptHandle.recordError(error);
}
endAttempt();
}Then, each of the attemptFailed... methods can simply call failAttempt(error).
@Override
public void attemptFailedDuration(Throwable error, java.time.Duration delay) {
failAttempt(error);
}
@Override
public void attemptFailedRetriesExhausted(Throwable error) {
failAttempt(error);
}
@Override
public void attemptPermanentFailure(Throwable error) {
failAttempt(error);
}
private void failAttempt(Throwable error) {
if (attemptHandle != null) {
attemptHandle.recordError(error);
}
endAttempt();
}| // Verify operation span (low-cardinality) | ||
| // SpanData operationSpan = | ||
| // spans.stream() | ||
| // .filter(span -> span.getName().equals("Echo.Echo/operation")) | ||
| // .findFirst() | ||
| // .orElseThrow(() -> new AssertionError("Operation span 'Echo.Echo/operation' | ||
| // not found")); | ||
| // assertThat(operationSpan.getStatus().getStatusCode()).isEqualTo(StatusCode.OK); |
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.
This block of code for verifying the operation span is commented out, which suggests it might not be working as expected. As this is a work-in-progress, it's understandable, but commented-out tests should be avoided in the final version. Could you please either fix the underlying issue so these assertions can be enabled, or remove the commented-out code if it's no longer relevant?
|
|
||
| List<SpanData> spans = spanExporter.getFinishedSpanItems(); | ||
| assertThat(spans).isNotEmpty(); | ||
| assertThat(spans.size() == 10); // 10 retires |
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.
This assertion is not using the Truth API idiomatically and is brittle.
- The expression
spans.size() == 10evaluates to a boolean. You are essentially doingassertThat(true). The idiomatic way isassertThat(spans).hasSize(10);. - Asserting a hardcoded number of retries makes the test fragile to changes in retry configuration.
Since you already verify that spans are not empty and all have an error status, consider if this exact size check is necessary. If so, please update to the idiomatic Truth syntax and fix the typo in 'retires'.
| assertThat(spans.size() == 10); // 10 retires | |
| assertThat(spans).hasSize(10); // 10 retries |
| // SpanData operationSpan = | ||
| // spans.stream() | ||
| // .filter(span -> span.getName().equals("Echo.Echo/operation")) | ||
| // .findFirst() | ||
| // .orElseThrow(() -> new AssertionError("Operation span 'Echo.Echo/operation' | ||
| // not found")); | ||
| // assertThat(operationSpan.getAttributes().get(AttributeKey.stringKey("op-key"))) | ||
| // .isEqualTo("op-value"); |
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.
…gleapis/sdk-platform-java into observability/tracing-noop-2
| ApiTracerFactory tracerFactory = settings.getTracerFactory(); | ||
| if (tracerFactory != null) { | ||
| String rpcSystem = ""; | ||
| if ("grpc".equals(transportChannel.getTransportName())) { |
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.
This logic would likely be better in the tracer factory constructor.
|
|





No description provided.