experiment(secretmanager): Otel tracing prototype in google-cloud-secretmanager - #18086
experiment(secretmanager): Otel tracing prototype in google-cloud-secretmanager#18086chalmerlowe wants to merge 8 commits into
Conversation
|
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.
Instead of popping configuration from kwargs here, consider adding configuration: Optional[object] = None as an explicit parameter to the create_channel function signature. This makes the parameter explicit, improves discoverability, and allows static analysis tools to type-check it properly.
| 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.
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).