feat(observability): implement universal 4-path OpenTelemetry tracing - #18433
chalmerlowe wants to merge 125 commits into
Conversation
… 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
…ments without grpc
…parsing, and attribute handling
…nv version in response hook
…and fix mypy comment
- 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.
…t_options to wrapped methods
| def host(self): | ||
| return self._host | ||
|
|
||
| def _wrap_method(self, func, *args, **kwargs): |
There was a problem hiding this comment.
@ohmayr
Following up on our discussion:
We created _wrap_method in base.py primarily because google-api-core and the generated client libraries are released independently.
-
Preventing
TypeErroron older environments:
If newly generated client code passed our new tracing arguments (client_options,kind,method_name) directly intowrap_method, any user running the library with an older version ofgoogle-api-corewould immediately hitTypeError: wrap_method() got an unexpected keyword argument. We can’t fix that insidewrap_methodbecause older versions ofgoogle-api-coreare already deployed in the wild. -
Serving as a runtime bridge:
Sitting inside the generated transport,_wrap_methodchecks once at module import whether the installedgoogle-api-coreunderstands tracing. If it does, it passes the tracing configuration through; if it doesn’t, it cleanly strips those new kwargs so existing RPC calls continue working without error. -
Keeping generated code DRY:
The transport instance already knows its own_client_options(tracer providers) andkind("grpc","rest", etc.). Having_wrap_methodbind those automatically means the code generator doesn't have to repeat that boilerplate across every single RPC in the service's method table.
| # The fallback below strips tracing-specific arguments when an older version | ||
| # of google-api-core is installed (which does not accept client_options, etc.). | ||
| for k in ["client_options", "method_name", "is_streaming", "kind"]: | ||
| kwargs.pop(k, None) |
There was a problem hiding this comment.
It looks like this change puts kind behind the _WRAP_METHOD_SUPPORTS_TRACING flag. But isn't kind already in use? Wouldn't stripping it here cause issues for async rest?
There was a problem hiding this comment.
I agree with this. If we pop kind like this, we lose the logic where we call wrap_errors for grpc (by checking kind) which will result in a bug.
There was a problem hiding this comment.
RESOLVED:
So this got complicated quick.
Turns out, since we support google-api-core all the way back to 2.14-ish...
2.14 to 2.19-ish no support for kind OR any tracing arguments.
In 2.19-ish Omair added support for kind to distinguish between gRPC and REST and prevent REST callables from being wrapped with gRPC callables, but still no support for tracing.
Now, in 2.36 we are introducing client_options, method_name, is_streaming AND continuing the original use of kind.
So we have three different scenarios to support when trying to pass args back to google-api-core and avoid a TypeError.
This now adapts to any of those situations.
There was a problem hiding this comment.
NOTE: @daniel-sanche @ohmayr
I would like to move all of this into _compat and just have a three line stub here in Transport to call _compat._wrap_method OR _compat._wrap_async_method.
That will cut down on a lot of duplicate boilerplate and seems like the right place to deal with these version-specific intricacies.
I don't really wanna do that in this PR. I would rather focus as much as we can on bringing this PR to a close and doing a fast follow OR stripping this out of this PR and doing it separately.
I created an issue to track the overhaul and deduplication of all things wrap.
| 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) |
There was a problem hiding this comment.
There is still severe code duplication between prep_wrappd_messages sync/async, and _wrap_method/wrap_method_async. And it seems surprising that in some places, async code is in shared_macros, and sometimes it is baked into the base class. I think there should be a way to generalize this a bit more, so logic doesn't need to be copied in multiple locations
This isn't really related to your observability changes though, so we don't have to spend too much time on this now. But if you want, I can try to suggest a patch for this
| _DEFAULT_ASYNC_TRANSPORT_KIND = _TRANSPORT_KIND_GRPC_ASYNC | ||
|
|
||
|
|
||
| class _AsyncGapicCallable(object): |
There was a problem hiding this comment.
There's a lot of duplicated code here. Can't we sub-class GapicCallable, and just change the parts that are needed?
IIUC, It seems like there are only a couple lines that need to differ, so we could pull those the rest out into shared helpers
There was a problem hiding this comment.
RESOLVED
We subclassed GapicCallable.
| return span_name, span_attributes, resolved_headers | ||
|
|
||
|
|
||
| class trace_http_request: |
There was a problem hiding this comment.
nit: For debugging purposes, I'd still suggest giving the class a PascalCase name, but have it accessed trough a snake_case method:
def trace_http_request(*args, **kwargs):
return _TraceManager(*args, **kwargs)
class _TraceContext:
...
| except Exception: # Fail-open on header injection failure | ||
| pass | ||
|
|
||
| return self._span |
There was a problem hiding this comment.
I'd suggest returning self here (in all branches, even if no span is created), and keeping the span encapsulated as internal state
Instead of:
with trace_http_request(...) as span:
....
record_http_response(span, response)
return response
You could do:
with trace_http_request(...) as trace_context:
....
trace_context.record_http_response(response)
return response
There was a problem hiding this comment.
RESOLVED
_observability.py now has trace_http_request AND _TraceContext.
_compat.py has the necessary code to enable trace_http_request for use in a with block if the correct version of google-api-core is present.
| resolved_headers, "__setitem__" | ||
| ): | ||
| try: | ||
| _get_trace_context_propagator().inject(resolved_headers) |
There was a problem hiding this comment.
I'd recommend making these helpers into static class methods, if this is the only place they're used (_get_trace_context_propagator, _build_http_span_attributes)
There was a problem hiding this comment.
RESOLVED
Both of these are now static methods on _TraceContext.
_get_trace_context_propagator
_build_http_span_attributes
| channel_interceptors.extend(otel_list) # pragma: NO COVER | ||
|
|
||
| # Fallback for older versions of google-api-core where apply_channel_interceptors is unavailable. | ||
| def _fallback_apply_interceptors(channel, interceptors): # pragma: NO COVER |
There was a problem hiding this comment.
Shouldn't this be in _compat, instead of putting it in every generated service?
There was a problem hiding this comment.
Resolved: Moved to _compat.py
| Returns: | ||
| tuple[str, dict[str, Any], Any]: A tuple of (span_name, attributes, resolved_headers). | ||
| """ | ||
| # Defensively handle case where client_options was passed as the first positional argument |
There was a problem hiding this comment.
Why do we need to defend against this? Isn't this new code?
There was a problem hiding this comment.
RESOLVED: Removed the comment and checks.
| message = getattr(target_exc, "message", None) | ||
| if not message and hasattr(target_exc, "details") and callable(target_exc.details): | ||
| message = target_exc.details() | ||
| if not message and isinstance(target_exc, Exception): |
There was a problem hiding this comment.
Should this be BaseException, to support asyncio.CancelledError (and maybe some others)?
There was a problem hiding this comment.
RESOLVED: Changed to BaseException.
| ) | ||
| for k, v in _extract_error_attributes(exc).items(): | ||
| span.set_attribute(k, v) | ||
| raise |
There was a problem hiding this comment.
Gemini pointed out that there's been some inconsistencies on how OTel handles asyncio.CancelledError and other BaseExceptions (there was a PR to record BaseException types as errors, and then another to revert it)
I haven't had time to dig through this yet, and I don't know if you've considered any of this, but wanted to raise this here
There was a problem hiding this comment.
RESOLVED: I believe we have reasonable checks in place to ensure that we can handle asyncio.CancelledError while still not sucking in any of the other errors that fall under BaseException (KeyboardInterrupt, etc).
…lable, and centralize async interceptor compat - Filter out process signals (KeyboardInterrupt, SystemExit, GeneratorExit) from tracing error capture before catching BaseException for cancellation and standard RPC errors. - Refactor _AsyncGapicCallable in google-api-core to inherit setup logic from _GapicCallable, eliminating duplicate preparation while preserving async-specific tracing kinds. - Encapsulate HTTP tracing context and clean up parameter handling in _observability.py. - Move async channel interceptor fallback into generated _compat.py rather than inlining across service transports, and regenerate all integration goldens. - Add unit tests verifying process signals bypass span error recording.
…nd centralize async interceptor compat - Encapsulate HTTP tracing context into PascalCase _TraceContext with trace_http_request factory. - Move record_response into _TraceContext and remove standalone record_http_response. - Update GAPIC templates (_shared_macros.j2, rest.py.j2, rest_asyncio.py.j2, _compat.py.j2, test_compat.py.j2). - Export apply_channel_interceptors with fallback in generated _compat.py. - Regenerate all 8 GAPIC integration goldens and verify tests pass.
…asyncio.CancelledError messages
…orts, and encapsulate http span helpers - Gate transport kind injection behind _WRAP_METHOD_SUPPORTS_TRACING in base.py.j2 and pop kind in fallback to prevent unexpected keyword argument errors on older google-api-core. - Assert kind is stripped in older core fallback in test_%service.py.j2. - Remove unused import contextlib in _compat.py.j2. - Regenerate all 8 GAPIC integration goldens. - Encapsulate _get_trace_context_propagator and _build_http_span_attributes as static methods on _TraceContext in _observability.py. - Update _extract_status_code exception type annotation to Optional[BaseException] in method.py to resolve mypy typing error.
…and document version checks - Add _ASYNC_WRAP_METHOD_SUPPORTS_KIND to detect support for the kind parameter in gapic_v1.method_async.wrap_method. - Support all three historical eras of google-api-core in _wrap_async_method: 1. Modern core with OpenTelemetry tracing: inject client_options, kind, method_name, and is_streaming. 2. Intermediate core (>= 2.19.1, PR #688): strip tracing-only arguments, but preserve and inject kind so REST callables are not erroneously wrapped with gRPC error handlers. 3. Ancient core (< 2.19.1): strip both tracing-only arguments and kind to avoid TypeError. - Add comprehensive docstring and inline comments to _wrap_async_method explaining each check and the runtime versions supported. - Update unit tests in test_%service.py.j2 to test both intermediate and ancient core fallback paths. - Regenerate all 8 GAPIC integration goldens.
|
@daniel-sanche I concur that we can prolly do more to reduce some duplication, but I believe some of this is in main. I don't wanna lose focus on getting this PR merged, so let's defer this action, I have created an Issue to track this.
|
…diffs - Remove unused contextlib import in _compat.py.j2 and goldens - Add NO COVER to apply_channel_interceptors in _compat.py.j2 - Add tests for apply_channel_interceptors in test_compat.py.j2 - Expand _wrap_async_method tests across all 3 google-api-core generations in test_%service.py.j2 - Streamline _TraceContext.__exit__ and test record_http_error with _TraceContext in google-api-core - Regenerate all 8 GAPIC integration goldens
| self._start_span_fn = None | ||
| if ( | ||
| not is_streaming | ||
| and kind == "grpc" | ||
| and kind in self._SUPPORTED_TRACING_KINDS |
There was a problem hiding this comment.
Note
Comment for reviewers:
@ohmayr asked a question in another venue that was something like this "do we need to check these? the values are coming from the GAPIC layer so aren't they already constrained?"
wrap_method and method_async.wrap_method are public APIs in google-api-core, exposed to handwritten clients, custom transports, and test mocks (not just generator code).
Public API Contract & Guardrails:
Because callers can pass arbitrary inputs to kind (or inherit BaseTransport.kind == ""), we need an explicit whitelist of supported tracing kinds. Without it, an arbitrary or mock transport (kind="mock", kind="custom") would fall through and blindly emit OpenTelemetry spans with "rpc.system.name" set to "grpc", violating semantic conventions and generating dirty telemetry.
Controlled Scope:
We only want method tracing to initialize when we know the transport's wire semantics and how to map its attributes properly. Whitelisting supported kinds acts as a fail-safe boundary.
Clean Polymorphism:
Because _AsyncGapicCallable inherits from _GapicCallable, leveraging _SUPPORTED_TRACING_KINDS across the classes allows us to enforce this contract gracefully—letting sync allow ("grpc", "rest") and async allow ("grpc_asyncio", "rest_asyncio") using standard object-oriented design without duplicating the initialization or span-creation logic.
…d align method names - Inline exception recording, status setting, and semantic attributes directly into _TraceContext.record_error - Add record_http_response and record_http_error method aliases on _TraceContext for convention parity - Remove loose standalone record_http_error function and temporary module-level aliases - Add comprehensive docstrings to trace_http_request detailing calling conventions - Mirror method aliases in _FallbackTraceContext in _compat.py.j2 - Update unit tests in test_observability.py to test _TraceContext.record_error directly - Regenerate all GAPIC integration goldens including showcase
…summary - Replace test_z_dump_raw_spans pseudo-test with clean session-scoped autouse fixture dump_raw_spans - Add pytest_terminal_summary hook in system conftest.py to output dedicated compliance test names and outcomes without verbose noise for other tests - Add actions/upload-artifact step in gapic-generator-tests.yml to publish raw_spans_output.json in GHA UI
…n summary - Pin actions/upload-artifact to SHA ea165f8d65b6e75b540449e92b4886f43607fa02 (# v4) to satisfy zizmor CI security audit - Update pytest_terminal_summary in system conftest.py to capture 'error' outcomes and deduplicate across test phases
…nx-docfx-yaml from CI
… and update goldens - Inspect and check _WRAP_METHOD_SUPPORTS_KIND in _wrap_method fallback to preserve kind on intermediate google-api-core versions while stripping tracing-only arguments - Update test_%service.py.j2 to test Generation 1, Generation 2, and Generation 3 fallback behaviors - Regenerate all 8 Bazel integration goldens
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:
Universal 4-Transport Support:
GrpcTransport): Continues using OpenTelemetry gRPC channel interceptors.GrpcAsyncIOTransport): Supplies OpenTelemetry interceptors directly during channel creation, respecting the immutable design of asynchronous gRPC channels.RestTransport): Adds wire span tracking around HTTP requests with automatic W3C trace context header injection (traceparent).AsyncRestTransport): Integrates HTTP wire span tracking and async context lifecycle handling with W3C header propagation.Refined Transport Contracts & Cleanup:
BaseTransport.kindto safely return an empty string by default instead of raising an exception.asyncio.CancelledError) so spans are closed accurately when asynchronous tasks are cancelled.Notes for Reviewers
packages/gapic-generatorand core helper functions inpackages/google-api-core.