feat(api-core): add channel orchestration for OpenTelemetry - #18237
feat(api-core): add channel orchestration for OpenTelemetry#18237chalmerlowe wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors OpenTelemetry channel instrumentation in google/api_core/_observability.py by replacing apply_otel_capabilities_to_channel with dedicated helpers for creating synchronous and asynchronous channels with OTel capabilities (create_channel_with_otel and create_async_channel_with_otel). Unit tests are updated accordingly. The review feedback highlights an inconsistency in interceptor execution order between the sync and async implementations, suggesting that the async OTel interceptor should be prepended rather than appended to the interceptors list to maintain consistent tracing semantics across both environments.
| mock_channel = mock.Mock() | ||
| mock_intercepted_channel = mock.Mock() | ||
| def test_get_otel_interceptor_sync_default(monkeypatch): | ||
| mock_otel = mock.Mock() |
There was a problem hiding this comment.
There is plenty of room for deduplicating some of the inner workings of these tests (using fixtures, reusable functions, etc). Happy to revise these but would prefer to get some initial buy-in on the overall approach in the body of the code before investing in what might end up being premature optimization.
| def create_channel_with_otel( | ||
| channel_factory: Callable[..., Any], | ||
| client_options: Optional[Union[ClientOptions, dict[str, Any]]] = None, | ||
| **channel_kwargs: Any, |
There was a problem hiding this comment.
Can we also accept *channel_args? That would make this easier to pass into the transport init: #18188 (comment)
There was a problem hiding this comment.
Adding *channel_args to create_channel_with_otel (and create_async_channel_with_otel) is a good improvement.
Transports pass self._host positionally to channel_init(self._host, ...). Supporting *channel_args allows us to pass functools.partial(_observability.create_channel_with_otel, Transport.create_channel, client_options=self._client_options) as the channel argument in the client.
This preserves true lazy channel initialization in the transport and eliminates the need for the client to duplicate extracting and passing credentials, scopes, quota_project_id, etc.
| return otel_grpc.client_interceptor(tracer_provider=tracer_provider) | ||
|
|
||
|
|
||
| def create_channel_with_otel( |
There was a problem hiding this comment.
I left some comments in your other PR, but if it's possible to decouple the interceptor more from the channel, that could make thinks a lot easier for composition in the future.
I think the previous apply_otel_capabilities_to_channel would be better suited for this. If we go with option A, the client could do something like
grpc_interceptor = functools.partial(apply_otel_capabilities_to_channel, client_options=options)
interceptor_list = [grpc_interceptor, logging_interceptor]
Transport(interceptors=interceptor_list, ...)
- Implement create_channel_with_otel and create_async_channel_with_otel helpers - Deduplicate interceptor instantiation via internal _get_otel_interceptor - Add unit tests in test_observability.py
… tests
- Use list(channel_kwargs.pop('interceptors', None) or []) in create_async_channel_with_otel
- Add unit tests for None and omitted interceptors arguments
…ion in channel factories - Add *channel_args to create_channel_with_otel and create_async_channel_with_otel - Make client_options keyword-only to prevent argument collision with functools.partial - Add TDD unit tests with detailed docstrings for positional forwarding and partial binding
1a04d27 to
cf95523
Compare
This pull request introduces channel orchestration helper functions in
google.api_core._observabilityto support OpenTelemetry (OTel) client interceptors for synchronous gRPC channels (asynchronous gRPC channels are included for comparison).Problem
Generated client libraries need a centralized, maintainable way to create and instrument gRPC channels with OpenTelemetry tracing when enabled via environment variables or client options.
Because synchronous gRPC (
grpc) and asynchronous gRPC (grpc.aio) have fundamentally different channel creation lifecycles (sync channels can be intercepted post-creation via OpenTelemetry's custom applier, whereas asyncgrpc.aiochannels are immutable and require interceptors at construction time), handling these differences directly in every generated client or transport creates boilerplate and risk of drift.Solution
This PR introduces channel creation orchestration helpers in
google.api_core._observability:create_channel_with_otel(Sync gRPC):channel_factory(*channel_args, **channel_kwargs)to construct the raw channel.*channel_argsand keyword-onlyclient_options, allowing clients to pass a lazy factory viafunctools.partial(create_channel_with_otel, Transport.create_channel, client_options=...)directly intoTransport(channel=...).create_async_channel_with_otel(Async gRPC - For Comparison):aio_client_interceptor) intokwargs['interceptors']prior to invokingchannel_factory(*channel_args, **channel_kwargs)._get_otel_interceptor:tracer_providerextraction fromClientOptionsand instantiating sync (client_interceptor) or async (aio_client_interceptor) interceptors without duplication.Testing
tests/unit/test_observability.py:*channel_args).functools.partialbinding and lazy execution for both sync and async.None, empty list, or pre-existing interceptors).GOOGLE_SDK_EXPERIMENTAL_PYTHON_TRACING_ENABLEDfeature gating.unit-3.10andlintsessions.Notes for Reviewers
*channel_argsalongside keyword-onlyclient_optionsenablesclient.pytemplates to passfunctools.partialas thechannelparameter, preserving true lazy channel initialization inTransport.__init__and eliminating the need to duplicate channel parameters (credentials,scopes, etc.) inClient.__init__.