experiment(secretmanager): Otel tracing prototype in google-cloud-secretmanager - #18086
experiment(secretmanager): Otel tracing prototype in google-cloud-secretmanager#18086chalmerlowe wants to merge 16 commits into
Conversation
…c.intercept_channel
|
DO NOT MERGE label added because this is an experiment to help define what changes should go into the GAPIC generator. |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry tracing support for gRPC channels in google-api-core and google-cloud-secret-manager by adding a tracer_provider option to ClientOptions and intercepting gRPC channels when tracing is enabled. The review feedback suggests defining configuration as an explicit parameter in the create_channel signatures instead of popping it from kwargs, addressing the unused configuration parameter in the REST transport, and adding test coverage to verify custom tracer provider injection rather than relying solely on the global provider.
| # NOTE: 'configuration' is popped to prevent a TypeError. | ||
| # Generated async transports (like those in secretmanager) pass 'configuration' | ||
| # down to this helper via **kwargs to support tracing in sync transports. | ||
| # However, 'aio.secure_channel' does not recognize this parameter and will | ||
| # crash if it is passed through. | ||
| # Async gRPC tracing is deferred to a future phase/PR, so we simply discard | ||
| # this parameter for now to ensure generated async code doesn't fail at runtime. | ||
| kwargs.pop("configuration", None) |
There was a problem hiding this comment.
Instead of manually popping configuration from kwargs inside the function body, consider adding configuration: Optional[object] = None as an explicit parameter to the create_channel function signature. Since named parameters are not captured in **kwargs, this will automatically prevent configuration from being passed down to aio.secure_channel, eliminating the need for this manual pop and the associated comment while improving type safety and discoverability.
| target = _modify_target_for_direct_path(target) | ||
|
|
||
| return grpc.secure_channel( | ||
| configuration = kwargs.pop("configuration", None) |
There was a problem hiding this comment.
| url_scheme: str = "https", | ||
| interceptor: Optional[SecretManagerServiceRestInterceptor] = None, | ||
| api_audience: Optional[str] = None, | ||
| configuration=None, |
There was a problem hiding this comment.
The configuration parameter is accepted in __init__ but is never stored (e.g., self._configuration = configuration) or used within the REST transport. If tracing is to be supported or if this parameter is intended for future use, it should be stored or passed to the base class/helpers. Otherwise, it is currently a dead parameter.
| def test_access_secret_version_custom_span(setup_otel, monkeypatch): | ||
| """Verify that calling access_secret_version produces a custom T3 span with attributes.""" | ||
|
|
||
| # Enable tracing via env var (assuming this is how we gate it for clients too) | ||
| monkeypatch.setenv("GOOGLE_CLOUD_PYTHON_TRACING_ENABLED", "true") | ||
|
|
||
| client = secretmanager_v1.SecretManagerServiceClient( | ||
| credentials=ga_credentials.AnonymousCredentials(), | ||
| ) |
There was a problem hiding this comment.
The observability tests currently only verify tracing using the global tracer provider (via the setup_otel fixture). Since the primary goal of this PR is to support custom tracer provider injection via ClientOptions, it would be highly beneficial to add a test case that explicitly passes a custom tracer_provider to the client's client_options and asserts that spans are recorded on that custom provider. This also avoids having to mock or override the private trace._TRACER_PROVIDER global variable.
Captures exceptions in access_secret_version on the T3 span and sets error attributes. Refines test fixtures to prevent ProxyTracer leakage.
0b40f3f to
fc56170
Compare
westarle
left a comment
There was a problem hiding this comment.
Thanks for sharing this PR, it helps put the other in context. I like the plan, I'll be interested to see how you update the generator to get the gcp.* values.
| @@ -39,13 +39,24 @@ | |||
| from google.api_core import exceptions as core_exceptions | |||
| from google.api_core import gapic_v1 | |||
| from google.api_core import retry as retries | |||
|
|
|||
| try: | |||
| from google.api_core import _feature_gating_helpers | |||
There was a problem hiding this comment.
would it be simpler to just bump the minimum version of google-api-core that supports this feature?
| @@ -756,6 +771,7 @@ def __init__( | |||
| client_info=client_info, | |||
| always_use_jwt_access=True, | |||
| api_audience=self._client_options.api_audience, | |||
| configuration=self._client_options, | |||
There was a problem hiding this comment.
this helps explain where the configuration is coming from!
if we are unpacking _client_options here, I think we should unpack the .tracer_provider as well.
| timeout=timeout, | ||
| metadata=metadata, | ||
| ) | ||
| if is_tracing_enabled and HAS_OTEL and tracer: |
There was a problem hiding this comment.
I think you would want to use the tracer_provider to get the tracer, instead of using the global one you fetched above.
I'd suggest resolving the global tracer provider here only if you don't have a tracer_provider passed via ClientOptions.
Problem
Need to demonstrate and verify how generated Google Cloud Python clients should interact with the new custom tracer provider plumbing in
google-api-core, specifically for gRPC.Note
This Pull Request is Prototype Only and is illustrative of what the GAPIC (Google API Client) generator should produce. The changes in
google-cloud-secret-managerare intended to inform generator template updates and/or base classes, not necessarily to be merged as handwritten code.Solution
This Pull Request updates the
google-cloud-secret-managerpackage to demonstrate end-to-end custom tracer provider injection and custom span attributes.SecretManagerServiceClientto passself._client_optionsas theconfigurationparameter when initializing transports.configurationparameter.access_secret_version.test_observability.pyto verify that custom spans are created with the correct attributes when tracing is enabled.Notes to Reviewers
google-api-coreintroduced in the companion PR feat(api-core): Opentelemetry tracing support for gRPC transports in google-api-core #18069 (Core Tracing Infrastructure).