Skip to content

fix(gapic-generator): [Approach 1] thread default_host to OTel helpers instead of mutating client_options - #18933

Draft
chalmerlowe wants to merge 4 commits into
mainfrom
fix/otel-default-host-param
Draft

chalmerlowe wants to merge 4 commits into
mainfrom
fix/otel-default-host-param

Conversation

@chalmerlowe

Copy link
Copy Markdown
Contributor

Summary

Comparison prototype (Clone 1 of #18869) implementing Approach 1: threading default_host explicitly into google.api_core._observability and gapic_v1.method.wrap_method / method_async.wrap_method instead of mutating client_options.api_endpoint.

Changes on top of #18869

  1. Explicit default_host parameter (google-api-core & gapic-generator):
    • Adds default_host: Optional[str] = None to _extract_endpoint_attributes, get_otel_interceptor, get_otel_async_interceptor, apply_otel_async_channel_interceptors, and gapic_v1.method.wrap_method / method_async.wrap_method.
    • Leaves self._client_options on Transport completely untouched (from_dict conversion only; no copying or mutation of api_endpoint).
  2. Pre-constructed channel guard (_ignore_credentials):
    • Sets self._default_host = None if self._ignore_credentials else self._host so pre-constructed grpc.Channel / grpc.aio.Channel instances do not falsely report DEFAULT_HOST when api_endpoint is unset.
    • Updates construct_observability_client in test_span_compliance.py to pass a callable channel factory (_insecure_channel_factory) so local Showcase gRPC compliance tests exercise the primary host-managed path (self._ignore_credentials = False).
  3. Upfront dict conversion:
    • Converts dict client_options upfront via client_options_lib.from_dict(client_options).

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces support for passing a resolved 'default_host' fallback parameter to transport wrappers and OpenTelemetry interceptors in 'google-api-core' and generated GAPIC clients, ensuring correct endpoint attribute extraction when 'api_endpoint' is unset. Feedback on the changes highlights a critical issue in the generated compatibility check ('_compat.py.j2'), where checking 'kwdefaults' for 'default_host' will always evaluate to False because it is a positional-or-keyword argument; using 'inspect.signature' is recommended instead.

Comment on lines +106 to +109
SUPPORTS_DEFAULT_HOST = bool(
getattr(gapic_v1.method.wrap_method, "__kwdefaults__", None)
and "default_host" in gapic_v1.method.wrap_method.__kwdefaults__
)

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.

high

The current check for default_host support relies on __kwdefaults__. However, default_host is defined as a standard positional-or-keyword argument (not a keyword-only argument) in gapic_v1.method.wrap_method. Therefore, __kwdefaults__ will be None, and SUPPORTS_DEFAULT_HOST will always evaluate to False, silently disabling the feature in generated clients.\n\nUsing inspect.signature is a robust and standard way to check if the parameter is supported.

import inspect\n\nSUPPORTS_DEFAULT_HOST = \"default_host\" in inspect.signature(gapic_v1.method.wrap_method).parameters

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant