Is this tracing extra required for customers to use tracing? Could we move the otel-api dependency into this extra?

@westarle

I reviewed the decisions we made in the draft HLD, the HLD, and the LLD where we decided to make opentelemetry-api required (since it's NOOP) while keeping opentelemetry-instrumentation-grpc optional.

If we move otel-api to the extra too, it seems we might introduce friction for customers who only want our Logical spans (T3) but have custom transport instrumentation (so they don't want otel-inst-grpc). If they are both in the same extra, they can't choose.

Also, it means we'll need to inject more try...except ImportError fallback logic into all GAPICs for T3 spans, which we were hoping to avoid.

Thoughts?

The solution should work by default for most customers who don't know about gRPC and aren't installing instrumentation. So I'd like to enable comprehensive spans T3/T4 with a single opt-in without needing to support and document the tracing extra.

If there's a strong technical reason to put otel-inst-grpc in a tracing extra (like diamond dependency issues or if it's unstable) then I'd prefer to make the tracing extra serve as opt-in for all tracing instrumentation.

We should test the interaction of our implementation in case customers have otel-inst-grpc monkey-patched in so we can provide guidance (either configure the monkeypatching to avoid our instances or avoid it).

Generally we would recommend customers with their own custom transport tracing not enable our tracing at all, in the future we might recommend some attributes and stuff if this is common.

@westarle

• opentelemetry-instrumentation-grpc 0.65b0 is beta.
• It is not a lightweight as opentelemetry-api.

We can create a [tracing] extra that carries both:

• opentelemetry-api
• opentelemetry-instrumentation-grpc

As context:
I am doing my best to keep this PR small and tight. I am happy to add more sophisticated tests and such to some follow-on PRs, but I would very much prefer that we do what we can to incrementally add bits and bobs and multiple testing approaches.

I have a PR in the works already that will add a more integration level of E2E testing using a fake endpoint as you mention in one of your other comments.

I would feel more comfortable following that with addition test PRs to focus on more complicated issues related to interactions we might have if global monkey-patching is turned on by default in the customer ecosystem.

There is a Testing Section in the LLD where I am capturing this feedback and defining what we expect more complex testing to look like.

Let's put everything in the tracing extra for now and consider using this as an enablement signal. Maybe call it tracing-experimental for now to prevent dependencies in case we want to refactor it.

No need to add test automation for the "what happens if the customer has already enabled grpc instrumentation" -- just try it out and account for it in the design (i.e. with docs for the customer to exclude our instrumentation).

My understanding was that we were planning to use the otel conventions, by making the api package required by default, and leaving the heavier sdk/instrumentation implementations as optional. I'm open to adding new required dependencies for Observability, if you think it's necessary for the intended experience @westarle. But seeing the beta label does give me pause

Let's put everything in the tracing extra for now and consider using this as an enablement signal. Maybe call it tracing-experimental for now to prevent dependencies in case we want to refactor it.

I think we should avoid adding temporary experimental extras like this though. The only purpose of advertising extra dependencies is as a contract to our users. If we want to keep this kind of thing internal while we work out the details, we can just make a private requirements.txt configuration we can include in our tests for now

I think a more robust test with less stubbing could be:

1. add a tracer provider via the configure API
2. make a gRPC request to a fake endpoint that just returns success
3. ensure the tracer provider has received a span

See this WIP PR for future additions of tests focused on integration versus simple unit tests.

That test looks much better. I would consider deleting these mocking tests since they depend on knowledge of internals and submitting the WIP PR when it's ready.

can all of this and is_tracing_enabled be resolved in a function on ClientOptions?

It isn't clear how ClientOptions are connecting to the kwargs["configuration"] here, or why we wouldn't pass ClientOptions directly to this function.

@westarle

Thanks for the feedback. Let's look at the design decisions captured in the Feature Gating Low-Level Design (LLD) to clarify why we ended up here, as regards this question:

can all of this and is_tracing_enabled be resolved in a function on ClientOptions?

The core reason for this structure is Separation of Concerns and Generalization:

1. The Resolver's Role: _feature_gating_helpers isn't just checking a boolean; it is resolving precedence (Environment Variable vs. Object Attribute vs. Dictionary Key). If we move is_tracing_enabled() to ClientOptions, then ClientOptions suddenly has to become aware of environment variables and resolution logic, which bloats its responsibilities.
2. Generalization (beyond ClientOptions): As discussed during the design phase (Item 11 in the Alternatives section and as noted in the Out of Scope section), we generalized this so it can gate any feature (not just o11y features), whether the configuration comes from ClientOptions, a plain dict, or another *Options object (e.g., custom retry settings). Naming the parameter configuration instead of "provider_key" in create_channel was a nod to this duck-typing/flexibility.

If we want to pivot and tightly couple all feature gating exclusively to ClientOptions, we can certainly return to the design doc and re-evaluate the testing and architectural footprint, but the current approach was chosen specifically to keep ClientOptions lean and the gating logic reusable across different types of configuration.

Regarding this question:

It isn't clear how ClientOptions are connecting to the kwargs["configuration"] here, or why we wouldn't pass ClientOptions directly to this function.

Why did we extract configuration from kwargs instead of making it an explicit argument?

We needed a way to get the tracer_provider or feature flags down into this helper.

Several factors played into our decision:

• Avoiding Signature Churn: create_channel is a highly public, widely used function in google-api-core. Adding a brand new explicit argument changes the signature. While adding it at the very end (before **kwargs) is usually safe, popping it from kwargs feels even "safer" because it doesn't change the explicit parameter list at all.

• Convenience during Prototyping: If we decide to revisit this at a later time (i.e. consider expanding the args list), that would be a doable thing but it did not seem worth it to go through all the effort to revise all the tests, the call signatures, the docstrings, type hints, etc unless we feel changing the create_channel signature is the right thing to do.

• Pass-Through Concerns: configuration is not just another option to pass down ... create_channel was largely a thin wrapper around grpc.secure_channel. At this time, grpc.secure_channel does NOT accept a configuration argument hence the step to remove it from kwargs before passing kwargs on.

If your question were more general: can't we encapsulate some of this bloat... we could do that (and i now believe we should)

Keep create_channel as lean as possible

def create_channel(
    target,
    credentials=None,
    scopes=None,
    ssl_credentials=None,
    credentials_file=None,
    quota_project_id=None,
    default_scopes=None,
    default_host=None,
    compression=None,
    attempt_direct_path: Optional[bool] = False,
    **kwargs,
):
    # ... setup credentials and target ...
    
    configuration = kwargs.pop("configuration", None)

    channel = grpc.secure_channel(
        target, composite_credentials, compression=compression, **kwargs
    )

    # All the OpenTelemetry mechanics are encapsulated in this helper
    channel = _intercept_channel_if_tracing_enabled(channel, configuration)

    return channel

Create an extracted helper

def _intercept_channel_if_tracing_enabled(channel, configuration):
    """If enabled, wraps the channel with OpenTelemetry tracing."""
    is_tracing_enabled = _feature_gating_helpers.resolve_feature_flags(
        env_var="GOOGLE_CLOUD_PYTHON_TRACING_ENABLED",
        feature_key="tracer_provider",
        configuration=configuration,
    )
    
    if not is_tracing_enabled:
        return channel

    try:
        import opentelemetry.instrumentation.grpc as otel_grpc  # type: ignore[import-not-found]

        tracer_provider = None
        if configuration is not None:
            if isinstance(configuration, dict):
                tracer_provider = configuration.get("tracer_provider")
            else:
                tracer_provider = getattr(configuration, "tracer_provider", None)

        interceptor = otel_grpc.client_interceptor(tracer_provider=tracer_provider)
        return otel_grpc.intercept_channel(channel, interceptor)
    except ImportError:
        # Fail open if instrumentation is missing
        return channel

I think we should keep api_core.grpc_helpers.create_channel as a light-weight wrapper over grpc channel creation, and leave this logic out of it. Attaching an interceptor like this feels like a decision we'd want to make at in the client layer, not baked into the api_core layer. It breaks some of the abstractions we have in place

We currently already set up a logging interceptor within the gapic transport class, and that feels like a more natural place for this kind of thing.

Although I'm not really a fan of how the LoggingInterceptor is set up currently, so don't follow that pattern exactly. LoggingInterceptor is hard-coded into the grpc transport class, so any veneers/users that use custom transport implementations lose the interceptor. Now that we're starting to scale up the number of interceptors in place, I think it would be better if we could either:

• A. pass in a list of interceptors to the transport at init time, like the rest client supports.
• B. add an attach_interceptor() method to the transport, and then call that to set-up our interceptors inside Client.__init__

TL;DR: I think the interceptor should live on the Transport, and then we can read the ClientOptions and optionally attach the transport when setting up a client. Would that work?

I think we should keep api_core.grpc_helpers.create_channel as a light-weight wrapper over grpc channel creation, and leave this logic out of it. Attaching an interceptor like this feels like a decision we'd want to make at in the client layer, not baked into the api_core layer. It breaks some of the abstractions we have in place

We currently already set up a logging interceptor within the gapic transport class, and that feels like a more natural place for this kind of thing.

Although I'm not really a fan of how the LoggingInterceptor is set up currently, so don't follow that pattern exactly. LoggingInterceptor is hard-coded into the grpc transport class, so any veneers/users that use custom transport implementations lose the interceptor. Now that we're starting to scale up the number of interceptors in place, I think it would be better if we could either:

• A. pass in a list of interceptors to the transport at init time, like the rest client supports.
• B. add an attach_interceptor() method to the transport, and then call that to set-up our interceptors inside Client.__init__

TL;DR: I think the interceptor should live on the Transport, and then we can read the ClientOptions and optionally attach the transport when setting up a client. Would that work?

So this means we'd be forced to bump up the minimum api_core version across all libraries going forward, right?

If at all possible, we should aim to fail gracefully, even if observability features are locked behind a certain api_core version

Can we be more specific about the types here? (You can use string annotations if you can't import the types yet)