[chalmerlowe]

Summary

Populates client_options.api_endpoint with the transport's resolved self._host in Transport.__init__ (base.py.j2) when client_options is provided without an explicit api_endpoint override.

Why

• Previously, _extract_endpoint_attributes in google-api-core only populated server.address and non-default server.port on Tier 3 (GAPIC client) and gRPC Tier 4 (wire attempt) spans when the caller explicitly passed ClientOptions(api_endpoint=...).
• When a client or transport connected using the service's DEFAULT_HOST (or a host passed directly to the transport constructor) without setting client_options.api_endpoint, server.address and server.port were omitted from Tier 3 and gRPC Tier 4 spans.
• Defaulting client_options.api_endpoint to self._host in the base transport constructor ensures Tier 3 and gRPC Tier 4 spans consistently record server.address and non-default server.port.

Files Modified

• packages/gapic-generator/gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/base.py.j2
• packages/gapic-generator/gapic/templates/tests/unit/gapic/%name_%version/%sub/test_%service.py.j2
• packages/gapic-generator/tests/system/telemetry_requirements_matrix.csv
• packages/gapic-generator/tests/integration/goldens/**

[gemini-code-assist]

Code Review

This pull request updates the base transport templates and generated golden files to automatically populate the api_endpoint in client_options with the transport's host if it is not already set, supporting both ClientOptions instances and dictionaries. It also updates the corresponding unit tests and telemetry requirements matrix. A review comment correctly points out that mutating client_options in-place can cause unexpected side effects if the caller reuses the options object, and suggests copying the options before modification.

[gemini-code-assist]

Mutating the client_options argument in-place (whether it is a ClientOptions instance or a dict) can lead to unexpected side effects if the caller reuses the same options object across multiple client or transport instances. To prevent this, we should create a copy of the options before modifying them.

        if isinstance(client_options, client_options_lib.ClientOptions):
            if not client_options.api_endpoint:
                import copy
                client_options = copy.copy(client_options)
                client_options.api_endpoint = self._host
        elif isinstance(client_options, dict):
            if not client_options.get("api_endpoint"):
                client_options = dict(client_options)
                client_options["api_endpoint"] = self._host

[daniel-sanche]

This seems like a fair point, we should probably copy the options (if we're not already)

[chalmerlowe]

Fixed in 81e79e2a791 and 8ac1e81836f. We now shallow-copy client_options (copy.copy(client_options)) for both ClientOptions instances and dict inputs before defaulting api_endpoint to self._host, and we added unit test assertions verifying that the caller's original ClientOptions or dict is not mutated in-place. We also added handling for client_options=None when OpenTelemetry tracing is enabled so direct transport instantiation populates api_endpoint as well.

[chalmerlowe]

UPDATED/RESOLVED

With the switch to passing default_host=self._default_host instead of reading/modifying client_options.api_endpoint in base.py.j2, base.py.j2 no longer needs any conditional inspection of client_options (it's back to a single self._client_options = client_options assignment), and _observability._extract_endpoint_attributes handles dict coercion via client_options_lib.from_dict if a dict is passed directly to a transport.

See this comment for additional info/clarification: we moved away from changing api_endpoint.
#18869 (comment)

[daniel-sanche]

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]

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

[chalmerlowe]

RESOLVED

I appreciate you pointing out this edge case. Because grpc and grpc_asyncio transports set self._grpc_channel = channel prior to calling super().__init__(...) whenever a pre-constructed grpc.Channel / grpc.aio.Channel instance is passed in, we now check getattr(self, "_grpc_channel", None) is not None in base.py.j2:

self._default_host = (
    None if getattr(self, "_grpc_channel", None) is not None else self._host
)

When a custom channel is injected directly, self._default_host is None, so we don't report a potentially mismatched default server.address / server.port on the spans (while still honoring client_options.api_endpoint if the user explicitly set it). We also added unit tests for sync, async, and base transports to verify this behavior.

[daniel-sanche]

Checking for _grpc_channel doesn't seem like a great approach to me. We expect it to be set by all grpc transports, whether passed in or not, it's just an implementation detail that it happens to be unset at this point in the execution. But that could easily change in the future, without warning

I think we should set an explicit user_provided_channel flag (like self._ignore_credentials), and use that for this conditional

[daniel-sanche]

Other than that, this approach LGTM