feat(api-core): add tracer_provider to ClientOptions for OTel support - #18139
feat(api-core): add tracer_provider to ClientOptions for OTel support#18139chalmerlowe wants to merge 15 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry helper functions to resolve and instantiate gRPC client interceptors, and adds a tracer_provider attribute to ClientOptions along with corresponding unit tests. The review feedback suggests simplifying the attribute access on client_options by avoiding defensive getattr calls since the attribute is guaranteed to be initialized on the class.
…iding getattr(None)
…injection helpers
|
|
||
|
|
||
| def test_apply_otel_capabilities_to_channel_enabled_otel_installed(monkeypatch): | ||
| mock_channel = mock.Mock() |
There was a problem hiding this comment.
NOTE TO REVIEWERS:
We are aware that there is significant duplication in these three tests.
We are looking for confirmation that the approach shown in the PR is acceptable.
With confirmation in hand we are happy to update these tests to simplify them, create mock fixtures, parametrize them where possible, but do not want to invest the time until we are confident that the approach will be approved.
Happy to consider updating the tests here OR in a fast follow PR.
| # limitations under the License. | ||
| # | ||
|
|
||
| """OpenTelemetry helpers for resolving and instantiating interceptors.""" |
There was a problem hiding this comment.
nit: I'd imainge "helpers" to be small stand-alone functions, used only within the library. These seem more important than that.
I'm not sure what changes are coming in the future, so I don't know the big picture, but consider something like _otel.py, _tracing.py, or even _observability.py
|
|
||
| Returns: | ||
| Any: The intercepted channel. | ||
| """ |
There was a problem hiding this comment.
this is expected raise ImportError, right? That should be mentioned in the docstring
| return False | ||
|
|
||
|
|
||
| def apply_otel_capabilities_to_channel( |
There was a problem hiding this comment.
should we have an async version of this?
There was a problem hiding this comment.
@daniel-sanche
As per the design docs, async is not part of this first phase.
It will be a following phase of the project.
There was a problem hiding this comment.
I don't think we need to include async code now, but I do think we need to at least consider it in our designs, so it doesn't diverge too much.
IIRC, async channels can't be mutated, so we probably shouldn't build a system around this API. Maybe we should just build and return the interceptor to the client, and let it decide how to add it to the channel?
There was a problem hiding this comment.
It may be worth clarifying a couple of specific constraints we bumped into regarding how OpenTelemetry handles gRPC in Python.
No Mutation in Place: the otel_grpc.intercept_channel does not mutate the channel in place; it returns a new intercepted channel instance. This aligns with standard gRPC behavior (both sync and async), so we aren't relying on in-place mutation here.
The TypeError Trap: The main reason we can't just build and return the interceptor to be processed by standard transport pipelines is a specific quirk of the OTel Python SDK. OTel's gRPC interceptors use custom protocols (grpcext) under the hood and are not compatible with standard grpc.ClientInterceptor type checks. If we put an OTel interceptor in a standard list and pass it to standard grpc.intercept_channel, it will crash with a TypeError. NOTE: we originally intended to build and return the interceptor but ran into these errors and are now pivoting.
Why Eager Wrapping: We are forced to use OTel's specialized otel_grpc.intercept_channel wrapper function. By handling this "eager wrapping" in the Client, we avoid leaking OTel-specific application logic into the "dumb" generated Transport, keeping the core transport pipeline cleaner and less complicated.
Regarding Async Forward-Compatibility: When we tackle async in the next phase, we may have more flexibility. OTel's async interceptors (aio_client_interceptors) are designed to be compatible with standard grpc.aio.ClientInterceptor interfaces. This means they will likely be able to be passed directly to standard pipelines without hitting the same TypeError hurdles we see in sync. However, if we prefer architectural consistency across the codebase, the "eager wrapping" pattern (using OTel's async helpers) remains a viable option to keep OTel logic centralized in the Client. There is some discussion about this at this bookmark in our internal documentation.
| then `api_endpoint` is used as the service endpoint. If `api_endpoint` is | ||
| not specified, the format will be `{service}.{universe_domain}`. | ||
| tracer_provider (Optional[object]): The OpenTelemetry tracer provider to use | ||
| for tracing. If not set, the global tracer provider will be used. |
There was a problem hiding this comment.
It seems like you're using None to represent the global tracer. But won't that cause issues, since resolve_feature_flags checks for None to determine whether the feature was enabled? And shouldn't users be able to pass in None to disable tracing per-client?
Maybe we should use a DEFAULT_PROVIDER sentinel for this?
There was a problem hiding this comment.
This PR treats resolve_feature_flags as a black box that determines whether to enable OR not enable.
I agree after further consideration that there is some room in our feature flag resolution to handle some edge cases we did not previously consider. Nonetheless, because it is a black box, I would suggest that we allow this PR to continue as it is focused on logic found outside that box and is blocking additional PRs AND revisit the design and implementation of feature gate resolution to handle those additional edge cases as an enhancement to the Feature Gating LLD and a future PR. This has been captured in Issue #18214
| `googleapis.com`. If both `api_endpoint` and `universe_domain` are set, | ||
| then `api_endpoint` is used as the service endpoint. If `api_endpoint` is | ||
| not specified, the format will be `{service}.{universe_domain}`. | ||
| tracer_provider (Optional[object]): The OpenTelemetry tracer provider to use |
There was a problem hiding this comment.
is there a better type that we can use here, instead of object?
| then `api_endpoint` is used as the service endpoint. If `api_endpoint` is | ||
| not specified, the format will be `{service}.{universe_domain}`. | ||
| tracer_provider (Optional[object]): The OpenTelemetry tracer provider to use | ||
| for tracing. If not set, the global tracer provider will be used. |
There was a problem hiding this comment.
Should we add something like "in libraries that support it?"? It seems to be advertising functionality that doesn't exist yet
|
|
||
| tracer_provider = None | ||
| if isinstance(client_options, dict): | ||
| tracer_provider = client_options.get("tracer_provider") |
There was a problem hiding this comment.
nit: "tracer_provider" is used a couple times in this file. Could be made into a constant
Problem
Generated client libraries require a mechanism to enable OpenTelemetry (OTel) capabilities (such as tracing) without introducing hard dependencies on the OTel SDK in the core runtime or cluttering generated code with complex feature flag resolution and fallback logic.
Solution
This PR introduces foundational helpers in
google-api-coreto centralize the resolution and application of OTel capabilities.tracer_providertoClientOptionsto allow programmatic configuration of the tracer provider.google.api_core._otel_helperscontaining:is_otel_capabilities_enabled(): Checks environment variables and configuration to determine if OTel is requested and available.apply_otel_capabilities_to_channel(): Wraps a gRPC channel with OTel interception using OTel's specializedintercept_channelto avoid compatibility issues with standardgrpc.intercept_channel.Notes to Reviewers
grpcextcompatibility) away from generated transports, keeping them standard and maintainable.