Skip to content

feat(observability): [WIP] implement universal 4-path OpenTelemetry tracing - #18433

Draft
chalmerlowe wants to merge 63 commits into
mainfrom
feat/otel-tracing-universal-4path
Draft

chalmerlowe wants to merge 63 commits into
mainfrom
feat/otel-tracing-universal-4path

Conversation

@chalmerlowe

Copy link
Copy Markdown
Contributor

feat(observability): implement universal 4-path OpenTelemetry tracing

Problems Solved

Google Cloud Python client libraries support four communication paths: synchronous gRPC, asynchronous gRPC, synchronous REST (HTTP), and asynchronous REST (HTTP). Previously, distributed OpenTelemetry tracing was only wired for synchronous gRPC calls, leaving asynchronous and HTTP communications untraced. Additionally, earlier drafts of asynchronous tracing attempted to modify gRPC channels after creation, which violated the immutability rules of the underlying Python gRPC library, and did not consistently forward client options across all transport classes.

Solutions

This pull request provides a unified, cross-transport tracing implementation:

  1. Universal 4-Transport Support:

    • Synchronous gRPC (GrpcTransport): Continues using OpenTelemetry gRPC channel interceptors.
    • Asynchronous gRPC (GrpcAsyncIOTransport): Supplies OpenTelemetry interceptors directly during channel creation, respecting the immutable design of asynchronous gRPC channels.
    • Synchronous REST (RestTransport): Adds wire span tracking around HTTP requests with automatic W3C trace context header injection (traceparent).
    • Asynchronous REST (AsyncRestTransport): Integrates HTTP wire span tracking and async context lifecycle handling with W3C header propagation.
  2. Refined Transport Contracts & Cleanup:

    • Replaced per-instance signature inspection with module-level constants to eliminate overhead during method invocation.
    • Configured BaseTransport.kind to safely return an empty string by default instead of raising an exception.
    • Updated REST and base transport initializers to accept and store client options, ensuring options are forwarded uniformly across all transport classes.
    • Resolved an issue in asynchronous method wrappers where caller-specified compression settings were not correctly evaluated.
    • Added task cancellation handling (asyncio.CancelledError) so spans are closed accurately when asynchronous tasks are cancelled.

Notes for Reviewers

  • This pull request targets template definitions in packages/gapic-generator and core helper functions in packages/google-api-core.
  • Per team coordination, generated client library code and integration goldens are not included in this change and will be updated in a follow-up step.
  • All additions maintain backward compatibility and preserve zero overhead when tracing is disabled.

… 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
- Broaden transport check in client.py.j2 to allow gRPC transport subclasses.
- Align version comments in client.py.j2 and grpc.py.j2 to 2.36.0+.
- Synchronize all golden client and transport files with template updates.
- Harden zero-overhead and custom tracer provider isolation assertions in test_tracing.py.
- Add direct client initialization test to verify template injection end-to-end.
…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.
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.
- Implement distributed OpenTelemetry tracing across all four GAPIC transports:
  sync gRPC, async gRPC, sync REST, and async REST.
- Address Daniel Sanche review comments from PR #18342:
  * Remove dynamic inspect.signature introspection in favor of module-level constants.
  * Make BaseTransport.kind return empty string instead of raising NotImplementedError.
  * Remove unused inspect import in client.py.j2.
  * Uniformly pass client_options and kwargs across all REST and base transports.
- Address PR #18367 async gRPC defects:
  * Respect grpc.aio channel immutability by passing interceptors during channel creation.
  * Fix caller-supplied compression parameter evaluation in method_async.py.
  * Capture asyncio.CancelledError on async callables for trace lifecycle safety.
- Add HTTP wire tracing helpers (start_http_span, record_http_response, record_http_error)
  with W3C traceparent injection in google-api-core and Jinja macros.
- Add comprehensive unit tests in google-api-core and gapic-generator.

@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 implements OpenTelemetry tracing capabilities across gRPC and REST transports for generated clients. It introduces _observability modules, updates transport constructors to accept client_options, and wires tracing interceptors into the transport layers. The reviewer identified a critical issue in the _shared_macros.j2 template where start_http_span was called with incorrect arguments, which would cause a runtime TypeError. The suggested fix involves passing a properly constructed request object to the tracing utility.

Comment on lines +172 to +179
with _observability.start_http_span(
client_options,
method=method,
url=url,
url_template=uri,
headers=headers,
body=body,
) as span:

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.

critical

The current call to _observability.start_http_span passes client_options as the first positional argument (which maps to request in the function signature) and passes method, url, headers, and body as keyword arguments. However, start_http_span does not accept these keyword arguments and expects a request object with those attributes. This will result in a TypeError at runtime when tracing is enabled.

To fix this, dynamically construct a lightweight request object using Python's built-in type() constructor and pass it as the request argument, while correctly passing url_template and client_options as keyword arguments.

        with _observability.start_http_span(
            type("Request", (), {"method": method, "url": url, "headers": headers, "body": body})(),
            url_template=uri,
            client_options=client_options,
        ) as span:

"DeleteOperation",
request_type="operations_pb2.DeleteOperationRequest",
response_type="None",
rpc_name="google.longrunning.Operations/DeleteOperation",

@chalmerlowe chalmerlowe Sep 21, 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.

Note

As context for the reviewer:

For native methods (like Echo or GetSecret), the generator reads the service's .proto file directly, so constructing the name in the template is straightforward:

method_name="{{ '.'.join(method.meta.address.package) }}.{{ service.name }}/{{ method.name }}"

However, mixins don't live in the service’s proto. Mixin methods (GetOperation, GetIamPolicy, ListLocations) are synthetic—they are injected by the generator from the static catalog in gapic/schema/mixins.py

Without a name attribute, any mixin call (like polling an operation or checking IAM permissions) would be unable to start an OpenTelemetry method span, or would emit an unknown/nameless span that failed our contract checks.

… flexible HTTP span invocations

- Update start_http_span in google-api-core to accept both bundled request objects and unpacked keyword arguments (method, url, headers, body, client_options) to avoid dummy object overhead in GAPIC templates.
- Properly unpack async gRPC interceptors into separate unary/stream lists in grpc_asyncio.py.j2.
- Forward _client_options to RestStub and AsyncRestStub instances.
- Supply method_name and is_streaming to async method wrappers in _shared_macros.j2 for Tier 3 method span creation.
- Add download retries and offline caching for Showcase descriptors in noxfile.py.
- Update system tracing tests and verify 100% pass rate against live Showcase across all 4 transports.
@chalmerlowe chalmerlowe self-assigned this Sep 21, 2026
@chalmerlowe chalmerlowe changed the title feat(observability): implement universal 4-path OpenTelemetry tracing feat(observability): [WIP] implement universal 4-path OpenTelemetry tracing Sep 21, 2026
…bazel goldens

- Accept BaseException in _observability.record_http_error for async cancellation safety
- Dynamic attribute lookup for observability functions in Jinja templates to prevent mypy failures against older core
- Unify HTTP dispatch pipeline in _shared_macros.j2 under span_context to ensure 100% statement and branch coverage across Python 3.10-3.14
- Add pragma NO COVER to version-dependent observability fallback branches
- Regenerate Bazel integration test goldens for all 8 test suites
…ntegration baselines

- Exclude packages/gapic-generator/tests/integration/goldens/ from pre-commit hooks to preserve byte-for-byte fidelity with Bazel outputs
- Re-sync goldens cleanly via Bazel across all 8 integration suites

import abc
import inspect
from typing import {% if service.any_extended_operations_methods %}Any, {% endif %}Awaitable, Callable, Dict, Optional, Sequence, Union

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.

The focus for this file is to get fundamental/core elements into the Base class to:

  • Help eliminate some checks that we were originally considering placing in the Transport classes, etc.
  • Cut down on some of the boilerplate in the Transport classes

…lize _observability compat

- Move _wrap_async_method into base.py.j2 alongside _wrap_method, delegating _wrap_method in grpc_asyncio and rest_asyncio to _wrap_async_method.
- Remove redundant wrap_async_method_macro from _shared_macros.j2.
- Add test_{service}_base_transport_wrap_async_method in test_%service.py.j2.
- Centralize _observability import in _compat.py.j2, replacing repetitive try/except blocks across client and all transport templates with clean _compat imports.
- Add test_observability_compat unit test in test_compat.py.j2.
- Regenerate and verify all 8 integration test goldens.
…est dispatch

Introduce trace_http_request context manager in google.api_core._observability to manage HTTP span lifecycle and error recording automatically. Export trace_http_request and record_http_response from _compat with graceful no-op fallbacks for older versions of google-api-core. Refactor REST dispatch macro in _shared_macros.j2 to eliminate duck-typing and line-by-line coverage pragmas, and regenerate golden tests.
…pragmas, and streamline noxfile

Consolidate _wrap_method and _wrap_async_method using a shared _wrap helper on base transport class in base.py.j2. Remove NO COVER pragmas from _compat.py.j2 observability block and add test_observability_compat_fallback in test_compat.py.j2 to test older environments. Streamline noxfile.py by reverting local caching logic to keep remote PR diff focused on OpenTelemetry dependencies. Regenerate all goldens.
Correctly route client interceptors to their corresponding channel
interceptor lists (_unary_unary_interceptors, _unary_stream_interceptors,
etc.) based on implemented methods. Update fallback helper in
grpc_asyncio.py.j2 and regenerate integration goldens.
}
{% endmacro %}

{# TODO: This helper logic to check whether `kind` needs to be configured in wrap_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.

Comment for Reviewers:

This logic got moved to the base transport.

… symmetry, and update templates

Add full type specifications to all Args blocks in google.api_core._observability.
Enforce transport kind symmetry across sync (_GapicCallable: "grpc", "rest")
and async (_AsyncGapicCallable: "grpc_asyncio", "rest_asyncio") method wrappers.
Add explanatory docstrings to _get_response in _shared_macros.j2 and _wrap_method
in async transports, restore backwards compatibility interceptor fallback in
grpc_asyncio.py.j2, and regenerate all integration goldens.
…it in compat tests

- Add explanatory comments across core observability error handlers and callable span managers clarifying fail-open invariants.
- Add test_observability_compat_present to test_compat.py.j2 and goldens to exercise and validate the tracing initialization path when _observability is present, achieving 100% coverage in showcase_unit.
- Regenerate and verify all 8 golden targets.
…propagator, and narrow exception handling

- Dynamically set rpc.system.name to 'http' for REST and Async REST transports in _GapicCallable and _AsyncGapicCallable.
- Support dual status lookups (status_code and status) in record_http_response to accommodate both requests and aiohttp responses.
- Narrow exception catching in trace_http_request to (Exception, asyncio.CancelledError) to prevent trapping system signals while preserving task cancellation.
- Cache module-level TraceContextTextMapPropagator to reduce hot-path allocation during distributed tracing header injection.
- Add Iterator[Any] return type annotation to trace_http_request.
- Update unit and system test assertions to expect rpc.system.name 'http' on REST method spans.
…ks and update goldens

Add # pragma: NO COVER to import and attribute fallback branches for
_observability in _compat.py.j2, update corresponding Bazel integration
goldens, and isolate the module-level _TRACE_CONTEXT_PROPAGATOR singleton
in google-api-core unit tests using monkeypatch.
Add tests for custom interceptors without intercept_* methods against
mock channels to cover fallback branches in apply_channel_interceptors,
restoring 100% test and branch coverage for google-api-core.
…eptors

Explicitly test both inner mapping loop and outer fallback when channel
target attributes are non-appendable, achieving 100% statement and branch
coverage in grpc_helpers_async.py.
…e test suite

- Standardize OpenTelemetry semantic span contracts across Tier 3 and Tier 4 for gRPC and HTTP/REST.
- Centralize all 18 feature contract specifications in span_contract.py with self-validating contracts and metadata introspection.
- Add test_span_compliance.py asserting full compliance against the Cloud Observability Tracing Test Plan across gRPC and HTTP/REST transports.
…rune redundant tests

- Consolidate all 18 feature contract specifications and 10 scenarios directly into test_span_compliance.py for a unified, single source of truth.
- Streamline span_contract.py into a lightweight, generic contract validator and scorecard reporter engine.
- Prune redundant scenario tests from test_tracing.py, retaining 5 core diagnostic, feature-gating, and wiring tests.

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

There's a lot I haven't looked at yet, but wanted to get some of my first comments out

"WaitOperation",
request_type="operations_pb2.WaitOperationRequest",
response_type="operations_pb2.Operation",
rpc_name="google.longrunning.Operations/WaitOperation",

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: This works, but it seems like the name component is duplicated in both

Should we store a package_name instead, then build this as {package_name}/{name}?

# The fallback below strips tracing-specific arguments when an older version
# of google-api-core is installed (which does not accept client_options, etc.).
# Excluded from coverage because our CI and testing environments always install
# a modern version of google-api-core that supports 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.

   # Excluded from coverage because our CI and testing environments always install
   # a modern version of google-api-core that supports tracing.

Doesn't the lower constraints.txt file installed the lower bounds for each library? This seems like a testing gap we'd want to address

return self._wrap(gapic_v1.method.wrap_method, _WRAP_METHOD_SUPPORTS_TRACING, func, *args, **kwargs)

def _wrap_async_method(self, func, *args, **kwargs):
return self._wrap(gapic_v1.method_async.wrap_method, _ASYNC_WRAP_METHOD_SUPPORTS_TRACING, func, *args, **kwargs)

@daniel-sanche daniel-sanche Sep 24, 2026

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'm finding all these wrappers very hard to follow. Is there any way we can filter these out instead of wrapping, like I suggested in the other PR?

If we do add extra helpers, I don't think we should need these three, along with additional _prep_wrapped_messages and _wrap_method implementations in the async transport. Can we try to simplify the wrapping logic?


if TYPE_CHECKING: # pragma: NO COVER
# ClientInterceptor was added in google-api-core 2.36.0+; ignore attribute-defined for older api-core versions during type checking
from google.api_core.grpc_helpers import ClientInterceptor # type: ignore[attr-defined]

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: TYPE_CHECKING blocks should probably come at the end of the import list, not the middle

self._grpc_channel = apply_interceptors(self._grpc_channel, channel_interceptors)

self._interceptor = _LoggingClientInterceptor()
self._logged_channel = grpc.intercept_channel(self._grpc_channel, self._interceptor)

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: should we apply this one the same way? Or save that for a follow-up?

)
self._grpc_channel = apply_interceptors(self._grpc_channel, channel_interceptors)

self._logged_channel = self._grpc_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.

It's not really clear to me why we have these two internal pointers. And it seems to make less sense now that we have even more interceptors being applied. Do you know the reason? Should we keep _logged_channel?

# In GAPIC templates, requests are assembled from local strings and dictionaries before
# hitting the session. Supporting keyword arguments avoids the CPU and memory overhead
# of instantiating a throwaway dummy request object on every single RPC execution.
@contextlib.contextmanager

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.

My first thought was that we'd also need an async context manager, but some brief research seems like that's not the case. Did you confirm this?

pass


@contextlib.contextmanager

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: have you considered making this context manager into a class? There's a lot of complex state and wrapping logic here, and it's a bit difficult to track all the state here.

I have a feeling it could be easier to work with by implementing __enter__ and __exit__ methods instead, and then some helpers like _build_attributes()

Don't feel like you have to re-write it, but something to consider. If you do want to keep the current code, is it possible to at least flatten some of the wrappers, or break the attribute creation into a separate function?

except (
Exception
): # Fail-open: telemetry failures must never disrupt core RPC execution
yield 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.

Context managers have an expectation that they yield only once. It looks like this could be called after the span was already yielded

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants