Experiment(secret-manager): add tracing transport logic to google-cloud-secret-manager

[chalmerlowe]

Problem

Client libraries currently lack built-in support for OpenTelemetry tracing interceptors. We need a flexible, explicit mechanism to inject these interceptors into the transport pipeline without tying the core low-level helpers too tightly to specific observability features.

Note

This PR is NOT intended to be merged. It is a proof-of-concept intended to inform the design and implementation of changes that need to be made in the GAPIC Generator templates.

Solution

This PR implements explicit interceptor injection in the SecretManagerServiceClient and its gRPC transport.

• Updated SecretManagerServiceClient to resolve OpenTelemetry interceptors using google-api-core helpers and explicitly pass them to the transport.
• Added unit tests to verify that interceptors are correctly injected and applied.

Notes to Reviewers

• Async Deferred: As per the design plan, Async gRPC updates are deferred to a separate PR to keep this implementation small and clean.
• Stacked PRs: This work builds on the foundational helpers introduced in the google-api-core.
• Explicit Injection: This demonstrates the "active orchestrator" role of the generated client, delegating resolution to core helpers but explicitly controlling the pipeline.

Fixes #18139 (partially, this is Phase 2 of the larger effort)

[gemini-code-assist]

Code Review

This pull request introduces OpenTelemetry (OTel) tracing support by adding a helper module _otel_helpers.py to resolve and instantiate OTel gRPC interceptors, extending ClientOptions to accept a tracer_provider, and updating the Secret Manager gRPC transport to inject these interceptors. The review feedback highlights Python compatibility issues in _otel_helpers.py due to the use of the | union operator, which is unsupported in older Python versions, and suggests using typing.Union instead. Additionally, it recommends adding a defensive None check for client_options to avoid calling getattr on a None object.

[chalmerlowe]

Warning

Do not merge label added because this is a prototype implementation to discover the needed edits to the GAPIC generator templates. This is not intended to be merged.

[chalmerlowe]

Happy to fine tune the tests with fixtures, reusable mocks and/or functions, etc, but prefer to wait on going there until we get some buy-in on the overall approach in the code to avoid premature optimization.

[daniel-sanche]

Do you need to modify any of these kwargs? If not, it might be best to keep this as a callable, and let the Transport pass in the rest of the channel-init arguments:

transport_kwargs["channel"] = functools.partial(
   _observability.create_channel_with_otel,
   channel_factory=SecretManagerServiceGrpcTransport.create_channel
   client_options=self._client_options
)

But this would only work if create_channel_with_otel supported passthrough positional args, along with the kwargs

[chalmerlowe]

✅ Agree

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, ...).

Using a partial function allows us to preserve lazy channel initialization in the transport. That also eliminates the need for the client to duplicate extracting and passing credentials, scopes, quota_project_id, etc.

[daniel-sanche]

A problem with this is that if anyone needs to use a custom channel (like bigtable), they lose the otel interceptors.

I was hoping there was a way that we could decouple the interceptors, instead of baking them into the channel. But it sounds like otel makes that difficult?

[daniel-sanche]

Here are a couple ideas on how to get around this:

A (preferred). Maybe when creating a channel, the interceptors argument could accept functions structured as Callable[[Channel], Channel], and then it can apply both styles of interceptor separately

B. If we really need to treat this otel interceptor as a special case, we could add another argument to the transport for this. Something like enable_otel, and if set, it will add the extra interceptor to whatever channel it sends up with

I haven't looked too deeply into this though, so maybe there are flaws, or maybe when we start thinking about async, that would cause even more complication

[chalmerlowe]

❌ Not Recommended

A problem with this is that if anyone needs to use a custom channel (like bigtable), they lose the otel interceptors.

When a user supplies their own custom channel (e.g. Transport(channel=custom_channel)), they are intentionally taking ownership of channel lifecycle, SSL credentials, socket options, and interceptors.

In line with standard SDK conventions, the client avoids mutating or re-wrapping a pre-built channel. Callers who bring their own channel and want OTel tracing are able to wrap their own channel with otel_grpc.intercept_channel(custom_channel, ...) before passing it in.

There is also the Principle of Least Surprise: silently intercepting or mutating a caller's pre-configured channel behind the scenes can break custom channel pooling, mock testing channels, and custom interceptor pipelines.

Option A: Maybe when creating a channel, the interceptors argument could accept functions structured as Callable[[Channel], Channel], and then it can apply both styles of interceptor separately

See this note for reasons why doing Option A is Not recommended.

Option B: If we really need to treat this otel interceptor as a special case, we could add another argument to the transport for this. Something like enable_otel, and if set, it will add the extra interceptor to whatever channel it sends up with

• Adding provider-specific telemetry flags (enable_otel, otel_tracer_provider) to Transport.__init__ couples the low-level wire transport layer to OpenTelemetry.
• Configuration and telemetry feature flags belong in ClientOptions (or similar) at the Client layer.
• Transports should remain clean wire transports accepting standard channel or interceptors.

[daniel-sanche]

When a user supplies their own custom channel (e.g. Transport(channel=custom_channel)), they are intentionally taking ownership of channel lifecycle, SSL credentials, socket options, and interceptors.

Interceptors aren't part of a channel, they wrap around the channel, producing a new instance. That's why we need to control this a layer up in the Transport class: it can determine which wrappers are applied.

If you disagree with this, you can try to change the code around to remove the interceptors argument from the Transport class, so we can handle this consistently. but I'm having a hard time picturing what that design would look like

In line with standard SDK conventions, the client avoids mutating or re-wrapping a pre-built channel.

The transport is currently already currently wrapping passed in channel with the LoggingClientInterceptor., so this would be a new convention

There is also the Principle of Least Surprise: silently intercepting or mutating a caller's pre-configured channel behind the scenes can break custom channel pooling, mock testing channels, and custom interceptor pipelines.

I think the expectation is if you make a custom channel, it may be wrapped. But if you make a custom Transport, you control the wrapping. That feels clean and consistent to me

#18188 (comment) for reasons why doing Option A is Not recommended.

I'll leave a separate response there, but the argument doesn't feel convincing to me

[daniel-sanche]

I left a comment here, suggesting that we may be able to accept Callable[[Channel], Channel] here to support otel's interceptor

[chalmerlowe]

❌ Not Recommended

There are a couple of structural reasons why keeping apply_interceptors focused strictly on Sequence[ClientInterceptor] is preferred over also accepting Callable[[Channel], Channel]:

1. Separation of Concerns:
   Keeping channel as the channel factory parameter (supporting functools.partial) and interceptors as the standard gRPC RPC interceptor parameter gives each argument a single clear responsibility across both sync and async transports.
2. Wrapper Overhead ($N$ Nested Proxy Channels):
   Calling grpc.intercept_channel(channel, *interceptors) in batch produces a single _InterceptedChannel dispatcher. If we intersperse true interceptors with callables to create (OR modify) channels, things get complicated. We would likely need an intervening step to loop through and examine each item in the interceptor parameter to decide whether it needs calling OR not.
   • Whereas looping sequentially and calling grpc.create_channel over and over creates $N$ nested proxy channel objects, which adds stack frames and wrapper overhead to every RPC.
3. Incompatibility with Async gRPC (grpc.aio):
   In grpc.aio, channels are immutable once constructed, and there is no post-creation interceptor wrapper. OTel's async interceptor must be passed directly into grpc.aio.secure_channel(..., interceptors=[...]) during channel creation. A Callable[[Channel], Channel] pattern cannot execute in async, which would force sync and async transports to diverge in how they handle interceptors OR necessitate that we inject an intermediary step to handle Channels versus Interceptors.

[daniel-sanche]

1. Separation of Concerns:

I'm not sure I understand. keeping channel creation and interceptor application separate is what I'm arguing for. The current design couples the grpc interceptor into the channel creation logic, and decouples the rest, which I find to be a confusing design

2. Wrapper Overhead

I also don't understand this argument either, and it makes me wonder if you misunderstand what I'm suggesting here.

• If you're worried about applying interceptors in a loop is less efficient than a single grpc.intercept_channel call, that's already what grpc.intercept_channel does under the hood. Note that each interceptor applied creates a new _Channel. intercept_channel itself is fundamentally a Callable[[Channel], Channel] operation, which is why I think this design works well. We're just extending off of grpc's existing wrapping logic
• If you're worried about creating more overhead on each rpc, don't be. This applies once when setting up the channel. No matter how we apply the interceptor, the end result is the same after construction
• if you're worried that adding stacks of interceptors adds overhead, that is true. But that's the cost of adding an interceptor, and we decided that these interceptors are worth adding

Incompatibility with Async gRPC

This is true, but that's because grpc.aio gives us a different API to work with, so I think it makes sense that we also expose a different