-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Experiment(secret-manager): add tracing transport logic to google-cloud-secret-manager (C) #18188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/otel-tracing-centralized-interceptor
Are you sure you want to change the base?
Changes from all commits
c686ba3
16910c1
c6031b3
2e658a0
64f8df6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,12 +27,12 @@ | |
| import grpc # type: ignore | ||
| import proto # type: ignore | ||
| from google.api_core import gapic_v1, grpc_helpers | ||
| from google.api_core.grpc_helpers import ClientInterceptor | ||
| from google.auth import credentials as ga_credentials # type: ignore | ||
| from google.auth.transport.grpc import SslCredentials # type: ignore | ||
| from google.cloud.location import locations_pb2 # type: ignore | ||
| from google.protobuf.json_format import MessageToJson | ||
|
|
||
| from google.cloud.secretmanager_v1.types import resources, service | ||
| from google.protobuf.json_format import MessageToJson | ||
|
|
||
| from .base import DEFAULT_CLIENT_INFO, SecretManagerServiceTransport | ||
|
|
||
|
|
@@ -148,6 +148,14 @@ def __init__( | |
| client_info: gapic_v1.client_info.ClientInfo = DEFAULT_CLIENT_INFO, | ||
| always_use_jwt_access: Optional[bool] = False, | ||
| api_audience: Optional[str] = None, | ||
| interceptors: Optional[ | ||
| Sequence[ | ||
| Union[ | ||
| ClientInterceptor, | ||
| Callable[[grpc.Channel], grpc.Channel], | ||
| ] | ||
| ] | ||
| ] = None, | ||
| ) -> None: | ||
| """Instantiate the transport. | ||
|
|
||
|
|
@@ -198,6 +206,9 @@ def __init__( | |
| to the service that will be set when using certain 3rd party | ||
| authentication flows. Audience is typically a resource identifier. | ||
| If not set, the host value will be used as a default. | ||
| interceptors (Optional[Sequence[Union[ClientInterceptor, Callable[[grpc.Channel], grpc.Channel]]]]): | ||
| Additional interceptors (or callables that apply interceptors) to apply to the | ||
| gRPC channel. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The underlying implementation is looking good Regarding the naming: I think we should still stick with the standard
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the docstring, could we do something like this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daniel-sanche Done. |
||
|
|
||
| Raises: | ||
| google.auth.exceptions.MutualTLSChannelError: If mutual TLS transport | ||
|
|
@@ -274,6 +285,10 @@ def __init__( | |
| ], | ||
| ) | ||
|
|
||
| self._grpc_channel = grpc_helpers.apply_channel_interceptors( | ||
| self._grpc_channel, interceptors | ||
| ) | ||
|
|
||
| self._interceptor = _LoggingClientInterceptor() | ||
| self._logged_channel = grpc.intercept_channel( | ||
| self._grpc_channel, self._interceptor | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| # limitations under the License. | ||
| # | ||
| import asyncio | ||
| import functools | ||
| import json | ||
| import math | ||
| import os | ||
|
|
@@ -61,15 +62,14 @@ | |
| from google.auth import credentials as ga_credentials | ||
| from google.auth.exceptions import MutualTLSChannelError | ||
| from google.cloud.location import locations_pb2 | ||
| from google.oauth2 import service_account | ||
|
|
||
| from google.cloud.secretmanager_v1.services.secret_manager_service import ( | ||
| SecretManagerServiceAsyncClient, | ||
| SecretManagerServiceClient, | ||
| pagers, | ||
| transports, | ||
| ) | ||
| from google.cloud.secretmanager_v1.types import resources, service | ||
| from google.oauth2 import service_account | ||
|
|
||
| CRED_INFO_JSON = { | ||
| "credential_source": "/path/to/file", | ||
|
|
@@ -770,6 +770,103 @@ def test_secret_manager_service_client_client_options( | |
| ) | ||
|
|
||
|
|
||
| def test_secret_manager_service_client_otel_channel_injection_enabled(): | ||
| """Proves that when OpenTelemetry tracing is enabled: | ||
|
|
||
| 1. SecretManagerServiceClient obtains the channel interceptor via | ||
| _observability.get_otel_interceptor passing client_options. | ||
| 2. The interceptor is passed into transport kwargs under 'interceptors', | ||
| allowing the Transport to apply it via apply_channel_interceptors. | ||
| """ | ||
| mock_interceptor = mock.Mock() | ||
| with ( | ||
| mock.patch( | ||
| "google.cloud.secretmanager_v1.services.secret_manager_service.client._observability.get_otel_interceptor", | ||
| return_value=mock_interceptor, | ||
| ) as mock_get_interceptor, | ||
| mock.patch.object( | ||
| transports.SecretManagerServiceGrpcTransport, "__init__", return_value=None | ||
| ) as patched_transport_init, | ||
| ): | ||
| client = SecretManagerServiceClient(transport="grpc") | ||
|
|
||
| mock_get_interceptor.assert_called_once_with(client._client_options) | ||
| called_kwargs = patched_transport_init.call_args.kwargs | ||
| assert "interceptors" in called_kwargs | ||
| assert called_kwargs["interceptors"] == [mock_interceptor] | ||
|
|
||
|
|
||
| def test_secret_manager_service_client_otel_channel_injection_disabled(): | ||
| """Proves that when OpenTelemetry tracing is disabled: | ||
|
|
||
| 1. SecretManagerServiceClient checks for an OTel interceptor and receives None. | ||
| 2. No 'interceptors' argument is passed to the transport constructor. | ||
| """ | ||
| with ( | ||
| mock.patch( | ||
| "google.cloud.secretmanager_v1.services.secret_manager_service.client._observability.get_otel_interceptor", | ||
| return_value=None, | ||
| ) as mock_get_interceptor, | ||
| mock.patch.object( | ||
| transports.SecretManagerServiceGrpcTransport, "__init__", return_value=None | ||
| ) as patched_transport_init, | ||
| ): | ||
| client = SecretManagerServiceClient(transport="grpc") | ||
|
|
||
| mock_get_interceptor.assert_called_once_with(client._client_options) | ||
| called_kwargs = patched_transport_init.call_args.kwargs | ||
| assert "interceptors" not in called_kwargs | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we should pass in an empty list, and assert that the otel inteceptor isn't present? This test will soon be out of date, since there will be other default interceptors |
||
|
|
||
|
|
||
| def test_secret_manager_service_grpc_transport_interceptors(): | ||
| """Proves that SecretManagerServiceGrpcTransport accepts channel interceptors | ||
| and invokes grpc_helpers.apply_channel_interceptors to apply them to the underlying | ||
| gRPC channel. | ||
| """ | ||
| mock_interceptor = mock.Mock() | ||
| mock_channel = mock.Mock() | ||
|
|
||
| with ( | ||
| mock.patch.object( | ||
| transports.SecretManagerServiceGrpcTransport, | ||
| "create_channel", | ||
| return_value=mock_channel, | ||
| ), | ||
| mock.patch( | ||
| "google.api_core.grpc_helpers.apply_channel_interceptors", | ||
| return_value=mock_channel, | ||
| ) as mock_apply_interceptors, | ||
| ): | ||
| transports.SecretManagerServiceGrpcTransport( | ||
| interceptors=[mock_interceptor], | ||
| ) | ||
|
|
||
| mock_apply_interceptors.assert_called_once_with( | ||
| mock_channel, [mock_interceptor] | ||
| ) | ||
|
|
||
|
|
||
| def test_secret_manager_service_grpc_transport_custom_channel_interceptors(): | ||
| """Proves that SecretManagerServiceGrpcTransport wraps explicitly passed custom channels | ||
| using grpc_helpers.apply_channel_interceptors. | ||
| """ | ||
| mock_interceptor = mock.Mock() | ||
| mock_custom_channel = mock.Mock(spec=grpc.Channel) | ||
|
|
||
| with mock.patch( | ||
| "google.api_core.grpc_helpers.apply_channel_interceptors", | ||
| return_value=mock_custom_channel, | ||
| ) as mock_apply_interceptors: | ||
| transports.SecretManagerServiceGrpcTransport( | ||
| channel=mock_custom_channel, | ||
| interceptors=[mock_interceptor], | ||
| ) | ||
|
|
||
| mock_apply_interceptors.assert_called_once_with( | ||
| mock_custom_channel, [mock_interceptor] | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "client_class,transport_class,transport_name,use_client_cert_env", | ||
| [ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When actually implementing this, we shoudl probably guard this, so we can fail gracefully, right? (IIRC, the plan wasn't to force an upgrade for this feature?)