Skip to content

fix(gapic-generator): default client_options.api_endpoint to transport host for OpenTelemetry spans #18869

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

Open
chalmerlowe wants to merge 3 commits into
base: main
Choose a base branch
from
+1,881 −57
Open

fix(gapic-generator): default client_options.api_endpoint to transport host for OpenTelemetry spans #18869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 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
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
20 changes: 20 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 @@ -3,11 +3,13 @@
{% block content %}

import abc
import copy
from typing import {% if service.any_extended_operations_methods %}Any, {% endif %}Awaitable, Callable, Dict, Optional, Sequence, Union

{% 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 @@ -162,6 +164,24 @@ class {{ service.name }}Transport(abc.ABC):
host += ':443'
self._host = host

# If the transport was instantiated directly with client_options=None
# while OpenTelemetry tracing is enabled via environment variable,
# create a ClientOptions instance with the resolved transport host so
# method and wire spans can record server.address and server.port.
if (
client_options is None
and _observability is not None
and _observability.is_otel_capabilities_enabled(None)
):
client_options = client_options_lib.ClientOptions(api_endpoint=self._host)
# And here we check for the presence of an api_endpoint attr OR key,
# shallow-copying before populating to avoid mutating caller-owned state.
elif isinstance(client_options, client_options_lib.ClientOptions) and not client_options.api_endpoint:
client_options = copy.copy(client_options)
client_options.api_endpoint = self._host

@daniel-sanche daniel-sanche 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.

There's a an edge case here: the host argument is only used to build new channels. But grpc transports can be passed in custom channels, which may point at a different host than the one here. So we may be reporting different data than the channel actually sends.

Is it possible to add some logic to the grpc transport, to disable this override when a channel is passed in directly? (_ignore_credentials seems to already be doing something similar)

@daniel-sanche daniel-sanche 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.

But depending on how this data is consumed later, maybe this isn't a big problem

@daniel-sanche daniel-sanche 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.

It feels a little weird to be writing to client_options, which is usually user-provided configuration. I could see that confusing things in the future, if we don't know the source of certain data. We usually prefer user-provided configuration over defaults

It's probably ok since this is an internal copy. But this does give me pause, and maybe there's a better way to handle this

@chalmerlowe chalmerlowe Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

QUESTION

I agree with your instinct here — overwriting api_endpoint on the transport's copy of client_options blurs the line between "user-configured endpoint override" and "resolved transport default", which could easily cause confusion down the road if anything inspects client_options.api_endpoint expecting it to only reflect explicit user input.

The only reason we routed self._host through client_options.api_endpoint here is that in google-api-core 2.30.0, _observability._extract_endpoint_attributes(client_options) (called by wrap_method, get_otel_interceptor, and apply_otel_async_channel_interceptors) currently only accepts client_options and looks up client_options.api_endpoint.

What do you think of this approach instead?

  1. In google-api-core, we add an optional default_host: Optional[str] = None parameter to:
  • wrap_method
  • get_otel_interceptor
  • get_otel_async_interceptor
  • apply_otel_async_channel_interceptors
  • and _extract_endpoint_attributes (which will resolve client_options.api_endpoint or default_host).

Those functions have lean signatures right now, so adding default_host is still fairly clean.

  1. Since we can cut a stand-alone google-api-core release early (and validate immediately via showcase-canary with INSTALL_LOCAL_CORE=true), when the next release of gapic-generator goes out, it can then pass default_host=self._host directly from Point A to Point B without touching or copying client_options at all.

Thoughts?

elif isinstance(client_options, dict) and not client_options.get("api_endpoint"):
client_options = copy.copy(client_options)
client_options["api_endpoint"] = self._host
self._client_options = client_options

@daniel-sanche daniel-sanche 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.

suggestion: It's a little surprising that we can store this field as either a dict or a ClientOptions instance. I'd recommend turning the dict representation into ClientOptions before this, which would simplify the conditional above, since you'd know the exact type

@chalmerlowe chalmerlowe Oct 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

RESOLVED

Good call — normalizing dict via client_options_lib.from_dict(client_options) upfront matches Client.__init__, guarantees self._client_options is always Optional[ClientOptions], and collapses the two elif branches into one. Updated!

self._wrapped_methods: Dict[Callable, Callable] = {}

Expand Down
80 changes: 78 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,80 @@ 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_endpoint",
[
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_endpoint
):
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) == expected_endpoint


@pytest.mark.parametrize(
"initial_endpoint,expected_endpoint",
[
pytest.param(None, "default.example.com:443", id="default_from_host"),
pytest.param(
"custom.example.com:443",
"custom.example.com:443",
id="preserve_explicit",
),
],
)
def test_{{ service.name|snake_case }}_base_transport_client_options_object(
initial_endpoint, expected_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 transport._client_options.api_endpoint == expected_endpoint
assert options.api_endpoint == initial_endpoint


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


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
20 changes: 20 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 @@ -14,10 +14,12 @@
# limitations under the License.
#
import abc
import copy
from typing import Awaitable, Callable, Dict, Optional, Sequence, Union

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 Down Expand Up @@ -133,6 +135,24 @@ def __init__(
host += ':443'
self._host = host

# If the transport was instantiated directly with client_options=None
# while OpenTelemetry tracing is enabled via environment variable,
# create a ClientOptions instance with the resolved transport host so
# method and wire spans can record server.address and server.port.
if (
client_options is None
and _observability is not None
and _observability.is_otel_capabilities_enabled(None)
):
client_options = client_options_lib.ClientOptions(api_endpoint=self._host)
# And here we check for the presence of an api_endpoint attr OR key,
# shallow-copying before populating to avoid mutating caller-owned state.
elif isinstance(client_options, client_options_lib.ClientOptions) and not client_options.api_endpoint:
client_options = copy.copy(client_options)
client_options.api_endpoint = self._host
elif isinstance(client_options, dict) and not client_options.get("api_endpoint"):
client_options = copy.copy(client_options)
client_options["api_endpoint"] = self._host
self._client_options = client_options
self._wrapped_methods: Dict[Callable, Callable] = {}

Expand Down
80 changes: 78 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,80 @@ 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_endpoint",
[
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_endpoint
):
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) == expected_endpoint


@pytest.mark.parametrize(
"initial_endpoint,expected_endpoint",
[
pytest.param(None, "default.example.com:443", id="default_from_host"),
pytest.param(
"custom.example.com:443",
"custom.example.com:443",
id="preserve_explicit",
),
],
)
def test_asset_service_base_transport_client_options_object(
initial_endpoint, expected_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 transport._client_options.api_endpoint == expected_endpoint
assert options.api_endpoint == initial_endpoint


@pytest.mark.parametrize(
"initial_options,expected_options",
[
pytest.param(
{},
{"api_endpoint": "default.example.com:443"},
id="default_from_host",
),
pytest.param(
{"api_endpoint": "custom.example.com:443"},
{"api_endpoint": "custom.example.com:443"},
id="preserve_explicit",
),
],
)
def test_asset_service_base_transport_client_options_dict(
initial_options, expected_options
):
options = dict(initial_options)
transport = transports.AssetServiceTransport(
host="default.example.com:443",
credentials=ga_credentials.AnonymousCredentials(),
client_options=options,
)
assert transport._client_options == expected_options
assert options == initial_options


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
20 changes: 20 additions & 0 deletions ...goldens/credentials/google/iam/credentials_v1/services/iam_credentials/transports/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
# limitations under the License.
#
import abc
import copy
from typing import Awaitable, Callable, Dict, Optional, Sequence, Union

from google.iam.credentials_v1 import gapic_version as package_version
from google.iam.credentials_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 Down Expand Up @@ -130,6 +132,24 @@ def __init__(
host += ':443'
self._host = host

# If the transport was instantiated directly with client_options=None
# while OpenTelemetry tracing is enabled via environment variable,
# create a ClientOptions instance with the resolved transport host so
# method and wire spans can record server.address and server.port.
if (
client_options is None
and _observability is not None
and _observability.is_otel_capabilities_enabled(None)
):
client_options = client_options_lib.ClientOptions(api_endpoint=self._host)
# And here we check for the presence of an api_endpoint attr OR key,
# shallow-copying before populating to avoid mutating caller-owned state.
elif isinstance(client_options, client_options_lib.ClientOptions) and not client_options.api_endpoint:
client_options = copy.copy(client_options)
client_options.api_endpoint = self._host
elif isinstance(client_options, dict) and not client_options.get("api_endpoint"):
client_options = copy.copy(client_options)
client_options["api_endpoint"] = self._host
self._client_options = client_options
self._wrapped_methods: Dict[Callable, Callable] = {}

Expand Down
Loading
Loading