-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(api-core): Opentelemetry tracing support for gRPC transports in google-api-core #18069
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
Changes from all commits
e5664b1
913f7f8
9d97783
993a085
59ac07a
0f7215f
8acc668
a73811c
80e2027
2226cb4
58e199b
9a9e764
09d5335
66c5936
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 |
|---|---|---|
|
|
@@ -25,8 +25,7 @@ | |
| import google.auth.transport.requests | ||
| import google.protobuf | ||
| import grpc | ||
|
|
||
| from google.api_core import exceptions, general_helpers | ||
| from google.api_core import _feature_gating_helpers, exceptions, general_helpers | ||
|
|
||
| # The list of gRPC Callable interfaces that return iterators. | ||
| _STREAM_WRAP_CLASSES = (grpc.UnaryStreamMultiCallable, grpc.StreamStreamMultiCallable) | ||
|
|
@@ -384,10 +383,37 @@ def create_channel( | |
| if attempt_direct_path: | ||
| target = _modify_target_for_direct_path(target) | ||
|
|
||
| return grpc.secure_channel( | ||
| configuration = kwargs.pop("configuration", None) | ||
|
|
||
| channel = grpc.secure_channel( | ||
| target, composite_credentials, compression=compression, **kwargs | ||
| ) | ||
|
|
||
| is_tracing_enabled = _feature_gating_helpers.resolve_feature_flags( | ||
| env_var="GOOGLE_CLOUD_PYTHON_TRACING_ENABLED", | ||
| feature_key="tracer_provider", | ||
| configuration=configuration, | ||
| ) | ||
|
|
||
| if is_tracing_enabled: | ||
| 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) | ||
|
Comment on lines
+402
to
+407
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. 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.
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. 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:
The core reason for this structure is Separation of Concerns and Generalization:
If we want to pivot and tightly couple all feature gating exclusively to Regarding this question:
Why did we extract configuration from We needed a way to get the Several factors played into our decision:
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 Create an extracted helper
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. I think we should keep 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:
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? |
||
|
|
||
| interceptor = otel_grpc.client_interceptor(tracer_provider=tracer_provider) | ||
| channel = otel_grpc.intercept_channel(channel, interceptor) | ||
| except ImportError: | ||
| # If OpenTelemetry gRPC instrumentation is missing, this should simply NOOP and fail open rather than failing import. | ||
| pass | ||
|
|
||
| return channel | ||
|
|
||
|
|
||
| def _modify_target_for_direct_path(target: str) -> str: | ||
| """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,9 +24,8 @@ | |
| from typing import AsyncGenerator, Generic, Iterator, Optional, TypeVar | ||
|
|
||
| import grpc | ||
| from grpc import aio | ||
|
|
||
| from google.api_core import exceptions, general_helpers, grpc_helpers | ||
| from grpc import aio | ||
|
|
||
| # denotes the proto response type for grpc calls | ||
| P = TypeVar("P") | ||
|
|
@@ -303,6 +302,15 @@ def create_channel( | |
| if attempt_direct_path: | ||
| target = grpc_helpers._modify_target_for_direct_path(target) | ||
|
|
||
| # NOTE: 'configuration' is popped to prevent a TypeError. | ||
| # Generated async transports (like those in google-cloud-* libs) pass 'configuration' | ||
| # down to this helper via **kwargs to support tracing in sync transports. | ||
| # However, 'aio.secure_channel' does not recognize this parameter yet and will | ||
| # crash if it is passed through. | ||
|
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. 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 |
||
| # 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) | ||
|
|
||
| return aio.secure_channel( | ||
| target, composite_credentials, compression=compression, **kwargs | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ dependencies = [ | |
| "proto-plus >= 1.26.1, < 2.0.0", | ||
| "google-auth >= 2.14.1, < 3.0.0", | ||
| "requests >= 2.33.0, < 3.0.0", | ||
| "opentelemetry-api >= 1.27.0, < 2.0.0", | ||
| ] | ||
| dynamic = ["version"] | ||
|
|
||
|
|
@@ -64,6 +65,10 @@ grpc = [ | |
| "grpcio-status >= 1.59.0, < 2.0.0", | ||
| "grpcio-status >= 1.75.1, < 2.0.0; python_version >= '3.14'", | ||
| ] | ||
| tracing = [ | ||
| "opentelemetry-instrumentation-grpc >= 0.46b0, < 1.0.0", | ||
|
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. Is this tracing extra required for customers to use tracing? Could we move the otel-api dependency into this extra?
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. I reviewed the decisions we made in the draft HLD, the HLD, and the LLD where we decided to make If we move Also, it means we'll need to inject more Thoughts?
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 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 If there's a strong technical reason to put We should test the interaction of our implementation in case customers have 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.
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.
We can create a [tracing] extra that carries both:
As context: 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.
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. Let's put everything in the 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).
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. 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
I think we should avoid adding temporary experimental |
||
| ] | ||
|
|
||
|
|
||
|
|
||
| [tool.setuptools.dynamic] | ||
|
|
@@ -93,4 +98,6 @@ filterwarnings = [ | |
| "ignore:.*custom tp_new.*in Python 3.14:DeprecationWarning", | ||
| # Remove once https://github.com/grpc/grpc/issues/35086 is fixed (and version newer than 1.60.0 is published) | ||
| "ignore:There is no current event loop:DeprecationWarning", | ||
| # Ignore external OpenTelemetry/importlib.metadata SelectableGroups warning | ||
| "ignore:.*SelectableGroups dict interface is deprecated:DeprecationWarning", | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,3 +12,4 @@ requests==2.33.0 | |
| grpcio==1.59.0 | ||
| grpcio-status==1.59.0 | ||
| proto-plus==1.26.1 | ||
| opentelemetry-api==1.27.0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,3 +13,4 @@ grpcio==1.59.0 | |
| grpcio-status==1.59.0 | ||
| proto-plus==1.26.1 | ||
| aiohttp==3.13.4 | ||
| opentelemetry-api==1.27.0 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,6 @@ | |
|
|
||
|
|
||
| import google.auth.credentials | ||
|
|
||
| from google.api_core import exceptions, grpc_helpers_async | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| # Copyright 2026 Google LLC | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Tests for OpenTelemetry gRPC interceptor integration in google-api-core.""" | ||
|
|
||
| import sys | ||
| import types | ||
| from unittest import mock | ||
|
|
||
| import pytest | ||
|
|
||
| try: | ||
| from google.api_core import grpc_helpers | ||
|
|
||
| HAS_GRPC_HELPERS = True | ||
| except ImportError: | ||
| HAS_GRPC_HELPERS = False | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_otel_grpc(monkeypatch): | ||
| """Fixture to mock OpenTelemetry gRPC hierarchy.""" | ||
| mock_otel = mock.Mock() | ||
| mock_otel_grpc = mock_otel.instrumentation.grpc | ||
| mock_interceptor = mock.Mock() | ||
| mock_otel_grpc.client_interceptor.return_value = mock_interceptor | ||
|
|
||
| modules = { | ||
| "opentelemetry": mock_otel, | ||
| "opentelemetry.instrumentation": mock_otel.instrumentation, | ||
| "opentelemetry.instrumentation.grpc": mock_otel_grpc, | ||
| } | ||
|
|
||
| for name, mod in modules.items(): | ||
| monkeypatch.setitem(sys.modules, name, mod) | ||
|
|
||
| return mock_otel_grpc | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "is_otel_installed, tracing_env_var_value, expect_otel_interceptor", | ||
| [ | ||
| pytest.param(True, "true", True, id="installed_and_enabled"), | ||
| pytest.param(True, "false", False, id="installed_but_disabled"), | ||
| pytest.param(False, "true", False, id="not_installed_fails_open"), | ||
| ], | ||
| ) | ||
| @pytest.mark.skipif(not HAS_GRPC_HELPERS, reason="Requires google-api-core[grpc]") | ||
| def test_create_channel_otel_combos( | ||
| monkeypatch, | ||
| mock_otel_grpc, | ||
| is_otel_installed, | ||
| tracing_env_var_value, | ||
| expect_otel_interceptor, | ||
| ): | ||
| """Verify create_channel behavior with various OTel installation and enablement states.""" | ||
|
|
||
| monkeypatch.setenv("GOOGLE_CLOUD_PYTHON_TRACING_ENABLED", tracing_env_var_value) | ||
|
|
||
| if not is_otel_installed: | ||
| monkeypatch.setitem(sys.modules, "opentelemetry.instrumentation.grpc", None) | ||
|
|
||
| mock_channel = "raw_channel" | ||
| mock_otel_grpc.intercept_channel.side_effect = lambda ch, inc: f"wrapped_{ch}" | ||
|
|
||
| with ( | ||
| mock.patch( | ||
| "grpc.secure_channel", return_value=mock_channel | ||
| ) as mock_secure_channel, | ||
| ): | ||
| with mock.patch( | ||
| "google.api_core.grpc_helpers._create_composite_credentials", | ||
| return_value=mock.Mock(), | ||
| ): | ||
| channel = grpc_helpers.create_channel("localhost:1234") | ||
|
|
||
| # Always expect raw channel creation | ||
| mock_secure_channel.assert_called_once() | ||
|
|
||
| if expect_otel_interceptor: | ||
| mock_otel_grpc.client_interceptor.assert_called_once() | ||
| mock_otel_grpc.intercept_channel.assert_called_once_with( | ||
| mock_channel, mock_otel_grpc.client_interceptor.return_value | ||
| ) | ||
| assert channel == f"wrapped_{mock_channel}" | ||
| else: | ||
| # OTel should NOT have been called | ||
| mock_otel_grpc.intercept_channel.assert_not_called() | ||
| assert channel == mock_channel | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "config_factory", | ||
| [ | ||
| lambda tp: {"tracer_provider": tp}, | ||
| lambda tp: types.SimpleNamespace(tracer_provider=tp), | ||
| ], | ||
| ids=["dict", "object"], | ||
| ) | ||
| @pytest.mark.skipif(not HAS_GRPC_HELPERS, reason="Requires google-api-core[grpc]") | ||
| def test_create_channel_with_custom_tracer_provider( | ||
| monkeypatch, mock_otel_grpc, config_factory | ||
| ): | ||
| """Verify that create_channel passes custom tracer_provider to OTel interceptor.""" | ||
|
|
||
| mock_tracer_provider = mock.Mock() | ||
| config = config_factory(mock_tracer_provider) | ||
|
|
||
| mock_channel = "raw_channel" | ||
| with ( | ||
| mock.patch("grpc.secure_channel", return_value=mock_channel), | ||
| ): | ||
| with mock.patch( | ||
| "google.api_core.grpc_helpers._create_composite_credentials", | ||
| return_value=mock.Mock(), | ||
| ): | ||
| grpc_helpers.create_channel("localhost:1234", configuration=config) | ||
|
|
||
| mock_otel_grpc.client_interceptor.assert_called_once_with( | ||
| tracer_provider=mock_tracer_provider | ||
| ) |
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.
Can we be more specific about the types here? (You can use string annotations if you can't import the types yet)