Skip to content

feat(gapic): add OpenTelemetry channel tracing to generator templates - #18342

Open
chalmerlowe wants to merge 44 commits into
mainfrom
feat/otel-tracing-gapic-showcase
Open

chalmerlowe wants to merge 44 commits into
mainfrom
feat/otel-tracing-gapic-showcase

Conversation

@chalmerlowe

@chalmerlowe chalmerlowe commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Problem

Generated client libraries currently lack OpenTelemetry tracing interceptor support for gRPC channels. When OpenTelemetry is configured in client options, generated clients cannot automatically create and pass interceptors down to their transport and gRPC channel.

Solution

Updates the code in the following ways:

  • Imports the centralized ClientInterceptor in the gRPC transport template and updates the transport initializer signature and docstrings to accept channel interceptors.
  • Updates the client template to look up OpenTelemetry interceptors via google.api_core._observability and pass them into the gRPC transport arguments.
  • Adds showcase-unit tests to the generated unit test templates to verify interceptor injection and transport application.
  • Adds showcase (system tests) to the generated system test templates.

Non-Goals & Future Work

  • AsyncIO Transport (grpc_asyncio): Channel tracing for grpc_asyncio and AsyncClient is intentionally deferred to an upcoming follow-up PR to maintain a focused review scope and ensure isolated test coverage.
  • Streaming RPC Assertions: While gRPC client interceptors intercept streaming calls, dedicated verification tests for client-, server-, and bidirectional-streaming spans will be added in a subsequent observability milestone.

Notes for Reviewers

  • The changes here are based on the feat/otel-tracing-t4-resource-attributes branch which introduces the required core observability helpers.

@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 adds OpenTelemetry tracing capabilities (Tier 3 observability) for gRPC channels in the GAPIC generator and google-api-core. It integrates client-side span creation and attribute extraction for non-streaming gRPC calls. The review feedback highlights two important improvements: handling potential ValueError exceptions gracefully when parsing malformed ports from api_endpoint in _observability.py, and supporting client_options when passed as a dictionary when resolving the custom tracer_provider in method.py.

Comment thread packages/google-api-core/google/api_core/_observability.py Outdated
Comment thread packages/google-api-core/google/api_core/gapic_v1/method.py Outdated
@chalmerlowe
chalmerlowe added this pull request to stack #18275 September 11, 2026 11:18
@chalmerlowe
chalmerlowe force-pushed the feat/otel-tracing-t4-resource-attributes branch from 5f64897 to daeed98 Compare September 11, 2026 11:20
@chalmerlowe
chalmerlowe force-pushed the feat/otel-tracing-gapic-showcase branch from 84d77b0 to cdd5eb8 Compare September 11, 2026 11:23
@chalmerlowe chalmerlowe changed the title [WIP] feat(gapic): add OpenTelemetry channel tracing to generator templates feat(gapic): add OpenTelemetry channel tracing to generator templates Sep 11, 2026
# Note: ClientInterceptor was added in google-api-core 2.25.0+; fallback for older versions
try:
from google.api_core.grpc_helpers import ClientInterceptor # type: ignore[attr-defined]
except ImportError: # pragma: NO COVER

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.

Added do-not-merge

Until the most recent version of google-api-core is published to PyPI, the ClientInterceptor object is unavailable. While working on this PR, this is a temporary workaround to enable testing, etc. Will be removed before merge.

@chalmerlowe chalmerlowe self-assigned this Sep 11, 2026
@chalmerlowe chalmerlowe added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 11, 2026
@chalmerlowe
chalmerlowe force-pushed the feat/otel-tracing-t4-resource-attributes branch from daeed98 to c68dab6 Compare September 11, 2026 12:07
@chalmerlowe
chalmerlowe force-pushed the feat/otel-tracing-gapic-showcase branch 3 times, most recently from c6a30e5 to 52176ce Compare September 11, 2026 14:18
@chalmerlowe
chalmerlowe force-pushed the feat/otel-tracing-t4-resource-attributes branch from 8775743 to d6307d6 Compare September 11, 2026 14:18
@chalmerlowe
chalmerlowe force-pushed the feat/otel-tracing-gapic-showcase branch from 52176ce to 59ae00b Compare September 11, 2026 14:19
@chalmerlowe

This comment was marked as resolved.

@chalmerlowe
chalmerlowe marked this pull request as ready for review September 11, 2026 16:17
@chalmerlowe
chalmerlowe requested a review from a team as a code owner September 11, 2026 16:17
@chalmerlowe
chalmerlowe marked this pull request as draft September 11, 2026 16:17
@chalmerlowe
chalmerlowe force-pushed the feat/otel-tracing-t4-resource-attributes branch from d6307d6 to b3931e9 Compare September 11, 2026 18:41
@chalmerlowe
chalmerlowe force-pushed the feat/otel-tracing-gapic-showcase branch from 86b7b10 to 72ad56d Compare September 11, 2026 18:41
@chalmerlowe
chalmerlowe removed this pull request from stack #18275 September 11, 2026 22:17
… hook

- Add rpc.system.name: 'grpc'
- Extract server.address and server.port from client options endpoint
- Extract gcp.grpc.resend_count from request resend count
- Extract gcp.resource.destination.id from request name or parent
- Add _client_response_hook for status code, error.type, and status.message
- Plumb response_hook into get_otel_interceptor and get_otel_async_interceptor
- Test endpoint attribute parsing across host/port variations
- Test destination id and resend count extraction
- Test client request and response hooks covering all status and error cases
- Test interceptor creation and custom endpoint attribute propagation
- Achieve 100% statement and branch coverage on _observability.py
…and hooks

- Rename _extract_t4_attributes to _extract_grpc_request_attributes
- Rename _make_client_request_hook to _make_grpc_client_request_hook
- Rename _client_request_hook to _grpc_client_request_hook
- Rename _client_response_hook to _grpc_client_response_hook
- Preserve generic _extract_endpoint_attributes for shared transport usage
…ntion

- Rename test_extract_t4_attributes to test_extract_grpc_request_attributes
- Rename test_client_request_hook to test_grpc_client_request_hook
- Rename test_client_response_hook to test_grpc_client_response_hook
- Update interceptor hook references to _grpc_client_* hooks
- Add url.domain extraction from universe_domain or default to googleapis.com
- Add _extract_error_attributes helper to extract gcp.errors.domain and gcp.errors.metadata.<key>
- Omit server.port when port matches scheme defaults (443 for https/grpc, 80 for http)
- Remove redundant _grpc_client_response_hook and _STATUS_CODE_NAMES
- Deduplicate name and parent resource lookup for gcp.resource.destination.id
- Add comprehensive parametrized unit tests and update interceptor test suites
…tem attribute

- Strip leading slash from gRPC attempt span names via span.update_name
- Set rpc.method to the fully qualified method name per PRD specification
- Retain rpc.system.name: 'grpc' and remove legacy rpc.system attribute to avoid duplication
- Update unit tests to verify span name normalization and attribute deduplication
- Remove gcp.resource.destination.id extraction from _extract_grpc_request_attributes
- Update unit tests to reflect attribute removal per July Strategy Update
{% if 'grpc' in opts.transport %}
if (
isinstance(transport_init, type)
and issubclass(transport_init, {{ service.grpc_transport_name }})

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 is a good change, but looking at this more, I'm thinking we should probably just append the custom interceptors inside the Transport.__init__, instead of trying to build the interceptor list here. I forgot that this same method is shared for sync/async/rest, which all have different interceptor formats. And this logic loses the interceptors if a Callable is passed

What do you think?

grpc_helpers,
"apply_channel_interceptors",
lambda channel, interceptors: channel,
)

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.

suggestion: If you wanted to make sure this is present, you could make use of the _compat file until we get the right version of api_core in place

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.

Not recommended

Regarding _compat.py: Because OpenTelemetry tracing requires google-api-core >= 2.36.0 anyway (for _observability and method spans), generated clients on older api_core versions will never emit or pass interceptors. Since interceptors is a new parameter that didn't exist in older client releases, the getattr fallback lambda safely avoids AttributeError without needing to generate and maintain a duplicate polyfill in _compat.py across 100+ libraries. When google-api-core >= 2.36.0 is eventually set as a minimum dependency in setup.py, we can drop the getattr entirely.

@chalmerlowe
chalmerlowe force-pushed the feat/otel-tracing-t4-resource-attributes branch from 0fb354a to c0fadc4 Compare September 15, 2026 08:52
@chalmerlowe chalmerlowe added this to the [o11y] Tracing milestone Sep 15, 2026
Base automatically changed from feat/otel-tracing-t4-resource-attributes to main September 15, 2026 13:06
…port template

- Place ClientInterceptor import under if TYPE_CHECKING: in grpc.py.j2 to eliminate runtime import overhead and avoid import failures on older google-api-core versions.
- String-quote "ClientInterceptor" in the interceptors type annotation for GrpcTransport.__init__.
- Regenerate and synchronize all golden gRPC transport files.
Align if TYPE_CHECKING: in golden gRPC transport files with # pragma: NO COVER to match grpc.py.j2 template output.
…ic-showcase

# Conflicts:
#	packages/google-api-core/google/api_core/_observability.py
#	packages/google-api-core/tests/unit/test_observability.py
@chalmerlowe
chalmerlowe requested review from a team as code owners September 16, 2026 19:59
@chalmerlowe
chalmerlowe requested review from sindhuvy and removed request for a team September 16, 2026 19:59
Pass canonical method_name to _wrap_method for mixin methods (Operations, IAM, Locations) so that client calls to mixin methods emit OpenTelemetry Tier 3 method spans.
…ementedError

Update test comment in template and goldens to explain testing of NotImplementedError when accessing transport.kind.
…ons in system tracing tests

Leverage otel_echo_client and span_exporter fixtures to eliminate boilerplate and unify span extraction patterns.
@chalmerlowe

Copy link
Copy Markdown
Contributor Author

This is a good change, but looking at this more, I'm thinking we should probably just append the custom interceptors inside the Transport.init, instead of trying to build the interceptor list here. I forgot that this same method is shared for sync/async/rest, which all have different interceptor formats. And this logic loses the interceptors if a Callable is passed

What do you think?

I made this change.

@daniel-sanche daniel-sanche 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.

Looking good for the most part, but take a look at my comments before merging. Particularly around client_options

return self._host

def _wrap_method(self, func, *args, **kwargs):
if self._wrap_with_tracing:

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.

nit: can't this just be if _WRAP_METHOD_SUPPORTS_TRACING? It doesn't seem like it should be tied to the transport

default_timeout=60.0,
client_info=client_info,
method_name="google.cloud.asset.v1.AssetService/ExportAssets",
),

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.

If the point of the wrapper is just to strip out unsupported args, we should be able to avoid the extra indirection, and do something like this:

try:
    kind = self.kind
except NotImplementedError:
    kind = None

self.export_assets: self._wrap_method(
    self.export_assets,
    default_timeout=60.0,
    client_info=client_info,
    **({
           "method_name": "google.cloud.asset.v1.AssetService/ExportAssets",
           "kind": kind,
        } if _WRAP_METHOD_SUPPORTS_TRACING and kind else {}
    )
),

kwargs["client_options"] = self._client_options
try:
kwargs["kind"] = self.kind
# The abstract BaseTransport class raises NotImplementedError for the kind property.

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 wonder if it would be worth adding a value for the base class, so we don't have to worry about this edge case. Maybe an empty string

_observability is not None
and (otel_interceptor := _observability.get_otel_interceptor(self._client_options)) is not None
and otel_interceptor not in channel_interceptors
and not any(getattr(i, "_is_otel_interceptor", None) is True for i in channel_interceptors)

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.

relying on an internal property like this seems a little ugly, since it could change at any time. I'd say this check probably isn't worth it, and we can just trust our inputs

But this is protected by a getattr guard, so not a big deal either way

import functools
{% endif %}
from http import HTTPStatus
import inspect

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 seems to be unused

client_info: gapic_v1.client_info.ClientInfo = DEFAULT_CLIENT_INFO,
always_use_jwt_access: Optional[bool] = False,
api_audience: Optional[str] = None,
client_options: Optional[Union[client_options_lib.ClientOptions, dict]] = None,

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.

Does this need to be added to http too, to be consistent?

We should at least add a **kwarg, so new arguments can be passed through?

and _observability.is_otel_capabilities_enabled(self._client_options)
and (
not isinstance(transport_init, type)
or issubclass(transport_init, {{ service.grpc_transport_name }})

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.

Do we have to be so selective about which transports we pass client_options to? I was thinking by moving the interceptor to the transport, we could pass the same init args to them all

(I wonder if grpc_transport_name here could cause problems specifically, if there are any http-only clients without this set)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants