feat: Extract resource name from unary requests for tracing#4159
feat: Extract resource name from unary requests for tracing#4159
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 telemetry capabilities within the GAPIC client libraries by introducing automatic extraction of destination resource names from unary request payloads. The changes span the code generation layer, the core GAX library's tracing context, and the underlying callable implementations for both gRPC and HTTP/JSON transports. This ensures that critical resource identification is consistently captured and propagated for improved observability without requiring manual configuration. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new client resource name extractor mechanism across the GAX (Google API eXtensions) libraries to enhance tracing and observability. The changes involve defining a ClientResourceNameExtractor interface, integrating a destinationResourceName field into ApiTracerContext for attribute propagation, and modifying GrpcCallSettings and HttpJsonCallSettings to incorporate this extractor. The TracedUnaryCallable now utilizes this extractor to populate the resource name in the ApiTracerContext. The gapic-generator-java has been updated to automatically generate the necessary code for creating and setting these extractors in gRPC and HTTP JSON client stub classes, based on resource reference annotations in proto files. This is validated through new test cases and updates to numerous golden files across various client libraries, ensuring consistent resource name extraction for tracing purposes. Minor Bazel build system updates are also included.
599a77d to
1320276
Compare
diegomarquezp
left a comment
There was a problem hiding this comment.
Thanks for the PR!
...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
Show resolved
Hide resolved
...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
Show resolved
Hide resolved
...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
Show resolved
Hide resolved
...om/google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/TracedUnaryCallable.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Can we add an assertion to the ITOtelTracing test to confirm the resource name attribute is being collected in the client?
There was a problem hiding this comment.
Done. I changed the EchoClient to IdentityClient because EchoClient does not have any message that is configured with resource_reference. The changes in GrpcEchoStub is due to mixins.
510bb27 to
444e00a
Compare
444e00a to
3e1b4e1
Compare
diegomarquezp
left a comment
There was a problem hiding this comment.
LGTM. I'll wait for @lqiu96.
gax-java/gax/src/main/java/com/google/api/gax/tracing/ApiTracerContext.java
Show resolved
Hide resolved
gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallSettings.java
Outdated
Show resolved
Hide resolved
Two general questions:
|
|
It gets the first field that has
As long as a field is configured with
I think we have validations during generation time to validate the resources are valid?
The generation applies to all calls so it does affect streaming calls. The extraction is only done for UnaryCalls currently. It will be done for other streaming calls in the future. |
|
|
Taking a step back to make sure that we are on the same page. IIUC, what is is doing is something like this: RPC Request It will extract the the resourceId as If the resource has multiple patterns, it just extracts the value. So the value inside the request matches with any of the possible Ok, I think that makes sense. LGTM. |
lqiu96
left a comment
There was a problem hiding this comment.
If possible, maybe a small showcase test in a follow up PR?
ITOtelTracing has been updated already. |


This PR updates the
gapic-generator-javaand GAPIC core telemetry layer to extract destination resource names from request payloads viagoogle.api.resource_reference.Changes:
AbstractTransportServiceStubClassComposerwas updated to securely parseresource_referenceannotations and emit.setResourceNameExtractor(...)logic onto stub settings upon compilation. Added safeguard logic for repeated fields.GrpcCallSettings,HttpJsonCallSettings, andTracedUnaryCallableto securely intercept requests right before initiating the trace, cloning and stamping the resource value to the active Observability map.destinationResourceIdtoApiTracerContextto propagate upstream context.IdentityClientinstead ofEchoClient. BecauseEchoClientdoes not have any message that is configured withresource_reference.This is a first PR for implementing Destination resource names. There will be a follow up PR that extract resource names based on heuristics if resource names are not explicitly configured.