Skip to content

fix(gapic-generator): [Approach 2] attach _host via internal _TransportClientOptions subclass #18934

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.

Sign up for GitHub

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

Draft
chalmerlowe wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

fix(gapic-generator): [Approach 2] attach _host via internal _TransportClientOptions subclass #18934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad65d4c
fix(gapic-generator): default client_options.api_endpoint to transpor…
chalmerlowe Oct 27, 2026
81e79e2
fix(gapic-generator): shallow-copy client_options and handle None whe…
chalmerlowe Oct 30, 2026
8ac1e81
test(gapic-generator): assert transport._client_options in otel chann…
chalmerlowe Oct 30, 2026
10a0662
refactor(observability): attach _host via internal _TransportClientOp…
chalmerlowe Oct 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions ...or/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/base.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ from typing import {% if service.any_extended_operations_methods %}Any, {% endif
{% set package_path = api.naming.module_namespace|join('.') + "." + api.naming.versioned_module_name %}
from {{package_path}} import gapic_version as package_version
from {{package_path}}._compat import (
_observability,
ASYNC_WRAP_METHOD_SUPPORTS_KIND as _ASYNC_WRAP_METHOD_SUPPORTS_KIND,
WRAP_METHOD_SUPPORTS_TRACING as _WRAP_METHOD_SUPPORTS_TRACING,
)
Expand Down Expand Up @@ -64,6 +65,22 @@ DEFAULT_CLIENT_INFO = gapic_v1.client_info.ClientInfo(gapic_version=package_vers
DEFAULT_CLIENT_INFO.protobuf_runtime_version = google.protobuf.__version__


class _TransportClientOptions(client_options_lib.ClientOptions):
"""Internal ClientOptions subclass that attaches the resolved transport host
as a separate ``_host`` attribute without mutating ``api_endpoint``."""

def __init__(
self,
base_options: Optional[client_options_lib.ClientOptions] = None,
*,
_host: Optional[str] = None,
) -> None:
super().__init__()
if base_options is not None:
self.__dict__.update(base_options.__dict__)
self._host = _host


class {{ service.name }}Transport(abc.ABC):
"""Abstract transport class for {{ service.name }}."""

Expand Down Expand Up @@ -162,6 +179,23 @@ class {{ service.name }}Transport(abc.ABC):
host += ':443'
self._host = host

if isinstance(client_options, dict):
client_options = client_options_lib.from_dict(client_options)

# Attach the resolved transport host on an internal ClientOptions subclass
# so OpenTelemetry spans can fall back to _host without mutating api_endpoint.
# Skip when _ignore_credentials is True (e.g., when a pre-constructed gRPC
# channel is supplied and self._host does not reflect the channel target).
if not self._ignore_credentials and (
client_options is not None
or (
_observability is not None
and _observability.is_otel_capabilities_enabled(None)
)
):
Comment on lines +189 to +195

@gemini-code-assist gemini-code-assist Bot Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current condition only wraps client_options when it is not None OR when OpenTelemetry is already enabled at startup.

This introduces an inconsistency:

  1. If a user passes client_options=ClientOptions(), it is always wrapped and _host is attached, even if OTEL is disabled at startup. If OTEL is enabled later dynamically, the host fallback works.
  2. If a user passes client_options=None and OTEL is disabled at startup, it is NOT wrapped. If OTEL is enabled later dynamically, the host fallback fails because self._client_options remains None.

To ensure consistent behavior and support dynamic OpenTelemetry enablement robustly, we should always wrap client_options in _TransportClientOptions when not self._ignore_credentials.

        if not self._ignore_credentials:

client_options = _TransportClientOptions(
client_options, _host=self._host
)
self._client_options = client_options
self._wrapped_methods: Dict[Callable, Callable] = {}

Expand Down
98 changes: 96 additions & 2 deletions .../gapic-generator/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,7 +996,7 @@ def test_{{ service.name|snake_case }}_grpc_transport_otel_channel_interceptor()
client_options=options,
)

mock_get_interceptor.assert_called_once_with(options)
mock_get_interceptor.assert_called_once_with(transport._client_options)
mock_apply_interceptors.assert_called_once_with(
mock_channel, [mock_otel_interceptor]
)
Expand Down Expand Up @@ -1070,7 +1070,9 @@ def test_{{ service.name|snake_case }}_grpc_asyncio_transport_otel_channel_inter
client_options=options,
)

mock_apply_async_interceptors.assert_called_once_with(mock_channel, options)
mock_apply_async_interceptors.assert_called_once_with(
mock_channel, transport._client_options
)
assert mock_create_channel.call_count == 1
assert transport.grpc_channel == mock_channel

Expand Down Expand Up @@ -2498,6 +2500,98 @@ def test_{{ service.name|snake_case }}_base_transport_wrap_method_modern_api_cor
assert "client_options" in mock_wrap.call_args.kwargs


@pytest.mark.parametrize(
"otel_enabled,expected_host",
[
pytest.param(False, None, id="otel_disabled"),
pytest.param(True, "default.example.com:443", id="otel_enabled"),
],
)
def test_{{ service.name|snake_case }}_base_transport_client_options_none(
monkeypatch, otel_enabled, expected_host
):
fake_observability = mock.MagicMock()
fake_observability.is_otel_capabilities_enabled.return_value = otel_enabled
monkeypatch.setattr(transports.base, "_observability", fake_observability)

transport = transports.{{ service.name }}Transport(
host="default.example.com:443",
credentials=ga_credentials.AnonymousCredentials(),
client_options=None,
)
assert getattr(transport._client_options, "api_endpoint", None) is None
assert getattr(transport._client_options, "_host", None) == expected_host


@pytest.mark.parametrize(
"initial_endpoint",
[
pytest.param(None, id="unset_api_endpoint"),
pytest.param("custom.example.com:443", id="explicit_api_endpoint"),
],
)
def test_{{ service.name|snake_case }}_base_transport_client_options_object(
initial_endpoint,
):
options = client_options.ClientOptions(api_endpoint=initial_endpoint)
transport = transports.{{ service.name }}Transport(
host="default.example.com:443",
credentials=ga_credentials.AnonymousCredentials(),
client_options=options,
)
assert isinstance(transport._client_options, client_options.ClientOptions)
assert transport._client_options.api_endpoint == initial_endpoint
assert transport._client_options._host == "default.example.com:443"
assert options.api_endpoint == initial_endpoint
assert not hasattr(options, "_host")


@pytest.mark.parametrize(
"initial_options,expected_endpoint",
[
pytest.param(
{},
None,
id="unset_api_endpoint",
),
pytest.param(
{"api_endpoint": "custom.example.com:443"},
"custom.example.com:443",
id="explicit_api_endpoint",
),
],
)
def test_{{ service.name|snake_case }}_base_transport_client_options_dict(
initial_options, expected_endpoint
):
options = dict(initial_options)
transport = transports.{{ service.name }}Transport(
host="default.example.com:443",
credentials=ga_credentials.AnonymousCredentials(),
client_options=options,
)
assert isinstance(transport._client_options, client_options.ClientOptions)
assert transport._client_options.api_endpoint == expected_endpoint
assert transport._client_options._host == "default.example.com:443"
assert options == initial_options


def test_{{ service.name|snake_case }}_base_transport_client_options_ignored_when_ignore_credentials():
class _CustomChannelTransport(transports.{{ service.name }}Transport):
def __init__(self, **kwargs):
self._ignore_credentials = True
super().__init__(**kwargs)

options = client_options.ClientOptions()
transport = _CustomChannelTransport(
host="default.example.com:443",
client_options=options,
)
assert transport._client_options is options
assert transport._client_options.api_endpoint is None
assert not hasattr(transport._client_options, "_host")


def test_{{ service.name|snake_case }}_base_transport_wrap_method_older_api_core_fallbacks(monkeypatch):
monkeypatch.setattr(transports.base, "_WRAP_METHOD_SUPPORTS_TRACING", False)
with mock.patch.object(transports.base.gapic_v1.method, "wrap_method"):
Expand Down
34 changes: 34 additions & 0 deletions ...integration/goldens/asset/google/cloud/asset_v1/services/asset_service/transports/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

from google.cloud.asset_v1 import gapic_version as package_version
from google.cloud.asset_v1._compat import (
_observability,
ASYNC_WRAP_METHOD_SUPPORTS_KIND as _ASYNC_WRAP_METHOD_SUPPORTS_KIND,
WRAP_METHOD_SUPPORTS_TRACING as _WRAP_METHOD_SUPPORTS_TRACING,
)
Expand All @@ -43,6 +44,22 @@
DEFAULT_CLIENT_INFO.protobuf_runtime_version = google.protobuf.__version__


class _TransportClientOptions(client_options_lib.ClientOptions):
"""Internal ClientOptions subclass that attaches the resolved transport host
as a separate ``_host`` attribute without mutating ``api_endpoint``."""

def __init__(
self,
base_options: Optional[client_options_lib.ClientOptions] = None,
*,
_host: Optional[str] = None,
) -> None:
super().__init__()
if base_options is not None:
self.__dict__.update(base_options.__dict__)
self._host = _host


class AssetServiceTransport(abc.ABC):
"""Abstract transport class for AssetService."""

Expand Down Expand Up @@ -133,6 +150,23 @@ def __init__(
host += ':443'
self._host = host

if isinstance(client_options, dict):
client_options = client_options_lib.from_dict(client_options)

# Attach the resolved transport host on an internal ClientOptions subclass
# so OpenTelemetry spans can fall back to _host without mutating api_endpoint.
# Skip when _ignore_credentials is True (e.g., when a pre-constructed gRPC
# channel is supplied and self._host does not reflect the channel target).
if not self._ignore_credentials and (
client_options is not None
or (
_observability is not None
and _observability.is_otel_capabilities_enabled(None)
)
):
client_options = _TransportClientOptions(
client_options, _host=self._host
)
self._client_options = client_options
self._wrapped_methods: Dict[Callable, Callable] = {}

Expand Down
98 changes: 96 additions & 2 deletions ...generator/tests/integration/goldens/asset/tests/unit/gapic/asset_v1/test_asset_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,7 +851,7 @@ def test_asset_service_grpc_transport_otel_channel_interceptor():
client_options=options,
)

mock_get_interceptor.assert_called_once_with(options)
mock_get_interceptor.assert_called_once_with(transport._client_options)
mock_apply_interceptors.assert_called_once_with(
mock_channel, [mock_otel_interceptor]
)
Expand Down Expand Up @@ -925,7 +925,9 @@ def test_asset_service_grpc_asyncio_transport_otel_channel_interceptor():
client_options=options,
)

mock_apply_async_interceptors.assert_called_once_with(mock_channel, options)
mock_apply_async_interceptors.assert_called_once_with(
mock_channel, transport._client_options
)
assert mock_create_channel.call_count == 1
assert transport.grpc_channel == mock_channel

Expand Down Expand Up @@ -18433,6 +18435,98 @@ def test_asset_service_base_transport_wrap_method_modern_api_core(monkeypatch):
assert "client_options" in mock_wrap.call_args.kwargs


@pytest.mark.parametrize(
"otel_enabled,expected_host",
[
pytest.param(False, None, id="otel_disabled"),
pytest.param(True, "default.example.com:443", id="otel_enabled"),
],
)
def test_asset_service_base_transport_client_options_none(
monkeypatch, otel_enabled, expected_host
):
fake_observability = mock.MagicMock()
fake_observability.is_otel_capabilities_enabled.return_value = otel_enabled
monkeypatch.setattr(transports.base, "_observability", fake_observability)

transport = transports.AssetServiceTransport(
host="default.example.com:443",
credentials=ga_credentials.AnonymousCredentials(),
client_options=None,
)
assert getattr(transport._client_options, "api_endpoint", None) is None
assert getattr(transport._client_options, "_host", None) == expected_host


@pytest.mark.parametrize(
"initial_endpoint",
[
pytest.param(None, id="unset_api_endpoint"),
pytest.param("custom.example.com:443", id="explicit_api_endpoint"),
],
)
def test_asset_service_base_transport_client_options_object(
initial_endpoint,
):
options = client_options.ClientOptions(api_endpoint=initial_endpoint)
transport = transports.AssetServiceTransport(
host="default.example.com:443",
credentials=ga_credentials.AnonymousCredentials(),
client_options=options,
)
assert isinstance(transport._client_options, client_options.ClientOptions)
assert transport._client_options.api_endpoint == initial_endpoint
assert transport._client_options._host == "default.example.com:443"
assert options.api_endpoint == initial_endpoint
assert not hasattr(options, "_host")


@pytest.mark.parametrize(
"initial_options,expected_endpoint",
[
pytest.param(
{},
None,
id="unset_api_endpoint",
),
pytest.param(
{"api_endpoint": "custom.example.com:443"},
"custom.example.com:443",
id="explicit_api_endpoint",
),
],
)
def test_asset_service_base_transport_client_options_dict(
initial_options, expected_endpoint
):
options = dict(initial_options)
transport = transports.AssetServiceTransport(
host="default.example.com:443",
credentials=ga_credentials.AnonymousCredentials(),
client_options=options,
)
assert isinstance(transport._client_options, client_options.ClientOptions)
assert transport._client_options.api_endpoint == expected_endpoint
assert transport._client_options._host == "default.example.com:443"
assert options == initial_options


def test_asset_service_base_transport_client_options_ignored_when_ignore_credentials():
class _CustomChannelTransport(transports.AssetServiceTransport):
def __init__(self, **kwargs):
self._ignore_credentials = True
super().__init__(**kwargs)

options = client_options.ClientOptions()
transport = _CustomChannelTransport(
host="default.example.com:443",
client_options=options,
)
assert transport._client_options is options
assert transport._client_options.api_endpoint is None
assert not hasattr(transport._client_options, "_host")


def test_asset_service_base_transport_wrap_method_older_api_core_fallbacks(monkeypatch):
monkeypatch.setattr(transports.base, "_WRAP_METHOD_SUPPORTS_TRACING", False)
with mock.patch.object(transports.base.gapic_v1.method, "wrap_method"):
Expand Down
Loading
Loading