feat(o11y): Introduce `rpc.system.name` and `rpc.method` in gRPC by diegomarquezp · Pull Request #4121 · googleapis/sdk-platform-java · GitHub
Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat(o11y): Introduce rpc.system.name and rpc.method in gRPC#4121

Merged
diegomarquezp merged 44 commits into
mainfrom
observability/tracing-attr/rpc.method-and-rpc.system
Mar 10, 2026
Merged

feat(o11y): Introduce rpc.system.name and rpc.method in gRPC#4121
diegomarquezp merged 44 commits into
mainfrom
observability/tracing-attr/rpc.method-and-rpc.system

Conversation

@diegomarquezp

@diegomarquezp diegomarquezp commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

This PR introduces the rpc.system.name and rpc.method span attributes in gRPC.

To achieve this, all Traced*Callable classes were modified with a constructor overload that accepts an ApiTracerContext instance. Such an instance will be preferred over SpanName when deciding which (also newly introduced) overload of ApiTracerFactory.newTracer() will be called.

The new method in ApiTracerFactory, newTracer(ApiTracer parent, ApiTracerContext tracerContext, OperationType operationType) will by default merge the existing factory's context with the new one and simply create a new SpanName.

However, the logic in SpanTracerFactory will use the context to create the span name directly.

The rpc.system.name and rpc.method properties are obtained from GrpcCallableFactory and stored in ApiTracerContext, which now holds the regexes for parsing client and method names (used when building SpanNames).

@product-auto-label product-auto-label Bot added the size: m Pull request size is medium. label Feb 25, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 significant and beneficial refactoring to the tracing infrastructure. By replacing the explicit SpanName with a more structured ApiTracerContext carried by ApiTracerFactory, the code becomes more maintainable and extensible. The introduction of rpc.system.name and rpc.method attributes aligns with modern observability practices.

However, there are a couple of points that need attention:

  1. Missing Tests (Critical): The PR adds new logic (e.g., in ApiTracerContext) and refactors existing components, but it lacks corresponding unit tests. The repository's contribution guidelines (line 148: Testing: All new logic should be accompanied by tests.) are not being met. Please add tests for the new functionality, especially for the parsing logic in ApiTracerContext.fromGrpcMethodName and ApiTracerContext.fromHttpJsonMethodName, and ensure existing tests are updated to reflect the API changes.

  2. Robustness: There's a potential for a NullPointerException in SpanTracerFactory which can be addressed with a defensive check.

Overall, this is a good direction, but the lack of tests is a critical issue that must be addressed before merging.

Comment thread gax-java/gax/src/main/java/com/google/api/gax/tracing/SpanTracerFactory.java Outdated
@diegomarquezp diegomarquezp added do not merge Indicates a pull request not ready for merge, due to either quality or timing. needs work This is a pull request that needs a little love. wip Work in Progress labels Feb 25, 2026
Base automatically changed from observability/tracing-attr/repo-artifact to main February 26, 2026 19:03
@diegomarquezp diegomarquezp force-pushed the observability/tracing-attr/rpc.method-and-rpc.system branch from b32f09a to e5b927a Compare February 26, 2026 20:13
@product-auto-label product-auto-label Bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Feb 27, 2026
Comment thread gax-java/gax/src/main/java/com/google/api/gax/tracing/TracedUnaryCallable.java Outdated
Comment thread gax-java/gax/src/main/java/com/google/api/gax/tracing/ApiTracerFactory.java Outdated
@product-auto-label product-auto-label Bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Mar 2, 2026
@diegomarquezp diegomarquezp requested a review from blakeli0 March 4, 2026 16:29
@diegomarquezp

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 rpc.system.name and rpc.method span attributes for gRPC tracing, which is a great enhancement for observability. The implementation is well-structured, refactoring tracing context creation into a new ApiTracerContext class and adding new overloads to the Traced*Callable classes and ApiTracerFactory. The test coverage is excellent, with new parameterized tests to handle both old and new code paths, and an integration test to verify the new attributes are correctly added to spans. I have a couple of minor suggestions to improve documentation and code consistency.

Comment thread gax-java/gax/src/main/java/com/google/api/gax/tracing/TracedUnaryCallable.java Outdated
diegomarquezp and others added 3 commits March 4, 2026 12:26
…ilityAttributes.java

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* @return a new {@link SpanName} with the parsed client name and method name
*/
public static SpanName of(ApiTracerContext apiTracerContext) {
Preconditions.checkState(apiTracerContext.fullMethodName() != null, "rpcMethod must be set");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think fullMethodName() is nullable? Also can we only pass a String to this method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think fullMethodName() is nullable?

I made it @Nullable considering the client-level vs method-def-level contexts we can have. ClientContext will not populate fullMethodName().

Also can we only pass a String to this method?

The problem with SpanName.of(apiTracerContext.getFullMethodName()) is that the callable factories cannot access the package-private method.

@InternalApi("Visible for testing")
static SpanName getSpanName(@Nonnull ApiMethodDescriptor<?, ?> methodDescriptor) {
ApiTracerContext apiTracerContext =
ApiTracerContext.newBuilder()
.setFullMethodName(methodDescriptor.getFullMethodName())
.setTransport(ApiTracerContext.Transport.HTTP)
.setLibraryMetadata(LibraryMetadata.empty())
.build();
return SpanName.of(apiTracerContext);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it @nullable considering the client-level vs method-def-level contexts we can have. ClientContext will not populate fullMethodName()

I see, makes sense. This might be another data point that it's time to create a ClientTracerContext that only contains client level tracer info. And we can merge it with method/request level before we create a request level tracer. I feel we have a lot of nullable fields in ApiTracerContext only because we don't have the info initially. But we can do it in another PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll follow up with separating the contexts.

this.innerCallable = innerCallable;
this.tracerFactory = tracerFactory;
this.apiTracerContext = apiTracerContext;
this.spanName = null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can create a spanName from ApiTracerContext now.

@diegomarquezp diegomarquezp Mar 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we wouldn't use it if we have the context, right?

AFAIU we are trying to provide logic that only handles a context object as the SoT for SpanName. That's why we made this change in ApiTracer:

if (apiTracerContext != null) {
tracer = tracerFactory.newTracer(context.getTracer(), apiTracerContext, OperationType.Unary);
} else {
tracer = tracerFactory.newTracer(context.getTracer(), spanName, OperationType.Unary);
}

default ApiTracer newTracer(
ApiTracer parent, ApiTracerContext tracerContext, OperationType operationType) {
SpanName spanName = SpanName.of(tracerContext);
return newTracer(parent, spanName, operationType);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Even if we don't use it, I think it's safer to populate it instead of making it nullable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, done.

private final UnaryCallable<RequestT, ResponseT> innerCallable;
private final ApiTracerFactory tracerFactory;
private final SpanName spanName;
@Nullable private final SpanName spanName;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep this spanName not nullable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do it once we confirm my question in #4121 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See answers below. Ideally we probably want to remove SpanName as a field, and only use ApiTracerContext as the source of truth. But it can not be done until we migrated all Callables to use ApiTracerContext.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

until we migrated all Callables to use ApiTracerContext

I thought the problem was with other classes that create the callables, such as GrpcCallableFactory. Are there usages of Traced*Callable in downstream libraries?

* @param tracerContext the method-definition-specific tracer context
* @param operationType the type of operation that the tracer will trace
*/
default ApiTracer newTracer(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downstream failure in bigtable is legit. One possible solution is to move OperationType to ApiTracerContext

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diegomarquezp Thanks for adding a new overloaded method! We need to remove this method as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace L85 with this method impl. and delete this method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding Bigtable: Removed method.

Regarding Spanner: The traced callables were not setting apiTracerContext.setOperationType and a null check in the Spanner tracer was failing. I was surprised our tests didn't catch it, so I added a null check in MetricsTracerFactory and OpencensusTracerFactory.

this.tracerFactory = Preconditions.checkNotNull(tracerFactory, "tracerFactory can't be null");
this.apiTracerContext =
Preconditions.checkNotNull(apiTracerContext, "apiTracerContext can't be null").toBuilder()
.setOperationType(OperationType.ClientStreaming)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ServerStreaming

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add unit tests for these default OperationType?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch. Done.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add unit tests for these default OperationType?

Done in 6b67410

@sonarqubecloud

Copy link
Copy Markdown

@sonarqubecloud

Copy link
Copy Markdown

@diegomarquezp diegomarquezp merged commit 7ab6d2e into main Mar 10, 2026
55 of 57 checks passed
@diegomarquezp diegomarquezp deleted the observability/tracing-attr/rpc.method-and-rpc.system branch March 10, 2026 21:35
lqiu96 pushed a commit that referenced this pull request Mar 17, 2026
🤖 I have created a release *beep* *boop*
---


<details><summary>2.68.0</summary>

##
[2.68.0](v2.67.0...v2.68.0)
(2026-03-17)


### Features

* Add client request duration metric.
([#4132](#4132))
([6a76397](6a76397))
* Add more attributes to golden signals metrics.
([#4135](#4135))
([59d0624](59d0624))
* **gax-httpjson:** add HttpJsonErrorParser utility
([#4137](#4137))
([a1b7565](a1b7565))
* **generator:** add extra allowed modules that will not be removed from
the monorepo if they are present
([#4124](#4124))
([774fe6e](774fe6e))
* **o11y:** introduce `gcp.client.repo` and `gcp.client.artifact`
attributes
([#4120](#4120))
([105f644](105f644))
* **o11y:** Introduce `rpc.system.name` and `rpc.method` in gRPC
([#4121](#4121))
([7ab6d2e](7ab6d2e))
* **o11y:** introduce server.port attribute
([#4128](#4128))
([56aa343](56aa343))


### Bug Fixes

* add null checks for ApiTracerFactory in ClientContext
([#4122](#4122))
([4b3dbe2](4b3dbe2))
* Decrease log level for directpath warnings outside GCE
([#4139](#4139))
([c9651e7](c9651e7))
* **gax-grpc:** add pick_first fallback to direct path service config
([#4143](#4143))
([b150fe9](b150fe9))
* Populate method level attributes in metrics recording
([#4149](#4149))
([7b7e6c9](7b7e6c9))
* suppress warnings in generated projects for non-idiomatic durations
([#4119](#4119))
([4206e6e](4206e6e))
* Use ServiceName + MethodName as the regex for Otel
([#2543](#2543))
([b9ae73f](b9ae73f))


### Documentation

* **hermetic_build:** fix config field name in readme
([#4130](#4130))
([a0c8f67](a0c8f67))
</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>
chingor13 pushed a commit to googleapis/google-cloud-java that referenced this pull request Mar 20, 2026
🤖 I have created a release *beep* *boop*
---


<details><summary>2.68.0</summary>

##
[2.68.0](googleapis/sdk-platform-java@v2.67.0...v2.68.0)
(2026-03-17)


### Features

* Add client request duration metric.
([#4132](googleapis/sdk-platform-java#4132))
([487650e](googleapis/sdk-platform-java@487650e))
* Add more attributes to golden signals metrics.
([#4135](googleapis/sdk-platform-java#4135))
([bc82dcb](googleapis/sdk-platform-java@bc82dcb))
* **gax-httpjson:** add HttpJsonErrorParser utility
([#4137](googleapis/sdk-platform-java#4137))
([6fe2446](googleapis/sdk-platform-java@6fe2446))
* **generator:** add extra allowed modules that will not be removed from
the monorepo if they are present
([#4124](googleapis/sdk-platform-java#4124))
([6a440da](googleapis/sdk-platform-java@6a440da))
* **o11y:** introduce `gcp.client.repo` and `gcp.client.artifact`
attributes
([#4120](googleapis/sdk-platform-java#4120))
([4954de5](googleapis/sdk-platform-java@4954de5))
* **o11y:** Introduce `rpc.system.name` and `rpc.method` in gRPC
([#4121](googleapis/sdk-platform-java#4121))
([3593c30](googleapis/sdk-platform-java@3593c30))
* **o11y:** introduce server.port attribute
([#4128](googleapis/sdk-platform-java#4128))
([1b10e02](googleapis/sdk-platform-java@1b10e02))


### Bug Fixes

* add null checks for ApiTracerFactory in ClientContext
([#4122](googleapis/sdk-platform-java#4122))
([ded1922](googleapis/sdk-platform-java@ded1922))
* Decrease log level for directpath warnings outside GCE
([#4139](googleapis/sdk-platform-java#4139))
([5151f34](googleapis/sdk-platform-java@5151f34))
* **gax-grpc:** add pick_first fallback to direct path service config
([#4143](googleapis/sdk-platform-java#4143))
([4934ad8](googleapis/sdk-platform-java@4934ad8))
* Populate method level attributes in metrics recording
([#4149](googleapis/sdk-platform-java#4149))
([69aabf8](googleapis/sdk-platform-java@69aabf8))
* suppress warnings in generated projects for non-idiomatic durations
([#4119](googleapis/sdk-platform-java#4119))
([8a0c565](googleapis/sdk-platform-java@8a0c565))
* Use ServiceName + MethodName as the regex for Otel
([#2543](googleapis/sdk-platform-java#2543))
([8196b8f](googleapis/sdk-platform-java@8196b8f))


### Documentation

* **hermetic_build:** fix config field name in readme
([#4130](googleapis/sdk-platform-java#4130))
([0d98c37](googleapis/sdk-platform-java@0d98c37))
</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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

size: xl Pull request size is extra large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants