feat(gapic): add OpenTelemetry channel tracing to generator templates - #18342
chalmerlowe wants to merge 44 commits into
Conversation
There was a problem hiding this comment.
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.
5f64897 to
daeed98
Compare
84d77b0 to
cdd5eb8
Compare
| # 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 |
There was a problem hiding this comment.
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.
daeed98 to
c68dab6
Compare
c6a30e5 to
52176ce
Compare
8775743 to
d6307d6
Compare
52176ce to
59ae00b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
d6307d6 to
b3931e9
Compare
86b7b10 to
72ad56d
Compare
… 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 }}) |
There was a problem hiding this comment.
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, | ||
| ) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
0fb354a to
c0fadc4
Compare
…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
…p method coverage
…or error.type in method tracing
…and client options
…ic-showcase # Conflicts: # packages/google-api-core/google/api_core/_observability.py # packages/google-api-core/tests/unit/test_observability.py
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.
I made this change. |
| return self._host | ||
|
|
||
| def _wrap_method(self, func, *args, **kwargs): | ||
| if self._wrap_with_tracing: |
There was a problem hiding this comment.
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", | ||
| ), |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 }}) |
There was a problem hiding this comment.
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)
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:
ClientInterceptorin the gRPC transport template and updates the transport initializer signature and docstrings to accept channel interceptors.google.api_core._observabilityand pass them into the gRPC transport arguments.Non-Goals & Future Work
grpc_asyncio): Channel tracing forgrpc_asyncioandAsyncClientis intentionally deferred to an upcoming follow-up PR to maintain a focused review scope and ensure isolated test coverage.Notes for Reviewers
feat/otel-tracing-t4-resource-attributesbranch which introduces the required core observability helpers.