feat(spanner): log client configuration at startup#17040
Conversation
727fa09 to
cbe95d4
Compare
There was a problem hiding this comment.
Code Review
This pull request replaces pickle with json in partition_helper.py to improve security and introduces a feature to log Spanner client options via an environment variable. Review feedback recommends internalizing the host attribute to _host to avoid naming conflicts and suggests using public properties instead of private attributes for logging to improve consistency.
| self.host = "spanner.googleapis.com" | ||
| if self._emulator_host: | ||
| self.host = self._emulator_host | ||
| elif self._experimental_host: | ||
| self.host = self._experimental_host | ||
| elif self._client_options and self._client_options.api_endpoint: | ||
| self.host = self._client_options.api_endpoint |
There was a problem hiding this comment.
The attribute host is quite generic and might conflict with future changes in base classes or subclasses. Since it is primarily used for logging client options here, it should be prefixed with an underscore to indicate it is internal.
| self.host = "spanner.googleapis.com" | |
| if self._emulator_host: | |
| self.host = self._emulator_host | |
| elif self._experimental_host: | |
| self.host = self._experimental_host | |
| elif self._client_options and self._client_options.api_endpoint: | |
| self.host = self._client_options.api_endpoint | |
| self._host = "spanner.googleapis.com" | |
| if self._emulator_host: | |
| self._host = self._emulator_host | |
| elif self._experimental_host: | |
| self._host = self._experimental_host | |
| elif self._client_options and self._client_options.api_endpoint: | |
| self._host = self._client_options.api_endpoint |
| "Spanner options: \n Project ID: %s\n Host: %s\n Route to leader enabled: %s\n Directed read options: %s\n Default transaction options: %s\n Observability options: %s\n Built-in metrics enabled: %s", | ||
| self.project, | ||
| self.host, | ||
| self.route_to_leader_enabled, | ||
| self._directed_read_options, | ||
| self._default_transaction_options, | ||
| self._observability_options, | ||
| _get_spanner_enable_builtin_metrics_env(), | ||
| ) |
There was a problem hiding this comment.
For better readability and consistency with the asynchronous client implementation, consider breaking the log format string into multiple lines. Additionally, prefer using public properties instead of private attributes (e.g., directed_read_options instead of _directed_read_options) where available.
log.info(
"Spanner options: \n"
" Project ID: %s\n"
" Host: %s\n"
" Route to leader enabled: %s\n"
" Directed read options: %s\n"
" Default transaction options: %s\n"
" Observability options: %s\n"
" Built-in metrics enabled: %s",
self.project,
self._host,
self.route_to_leader_enabled,
self.directed_read_options,
self.default_transaction_options,
self.observability_options,
_get_spanner_enable_builtin_metrics_env(),
)| self.host = "spanner.googleapis.com" | ||
| if self._emulator_host: | ||
| self.host = self._emulator_host | ||
| elif self._experimental_host: | ||
| self.host = self._experimental_host | ||
| elif self._client_options and self._client_options.api_endpoint: | ||
| self.host = self._client_options.api_endpoint |
There was a problem hiding this comment.
The attribute host is quite generic and might conflict with future changes in base classes or subclasses. Since it is primarily used for logging client options here, it should be prefixed with an underscore to indicate it is internal.
| self.host = "spanner.googleapis.com" | |
| if self._emulator_host: | |
| self.host = self._emulator_host | |
| elif self._experimental_host: | |
| self.host = self._experimental_host | |
| elif self._client_options and self._client_options.api_endpoint: | |
| self.host = self._client_options.api_endpoint | |
| self._host = "spanner.googleapis.com" | |
| if self._emulator_host: | |
| self._host = self._emulator_host | |
| elif self._experimental_host: | |
| self._host = self._experimental_host | |
| elif self._client_options and self._client_options.api_endpoint: | |
| self._host = self._client_options.api_endpoint |
| self.host, | ||
| self.route_to_leader_enabled, | ||
| self._directed_read_options, | ||
| self._default_transaction_options, | ||
| self._observability_options, |
There was a problem hiding this comment.
Prefer using public properties instead of private attributes (e.g., directed_read_options instead of _directed_read_options) where available for logging.
| self.host, | |
| self.route_to_leader_enabled, | |
| self._directed_read_options, | |
| self._default_transaction_options, | |
| self._observability_options, | |
| self._host, | |
| self.route_to_leader_enabled, | |
| self.directed_read_options, | |
| self.default_transaction_options, | |
| self.observability_options, |
This is an already reviewed change as part of this PR in old repo.
This change introduces logging for Spanner client options upon initialization.
This allows customers to easily capture and inspect the configuration being used, which is valuable for debugging and verification.
This feature mirrors the existing logSpannerOptions functionality in the Java client, improving consistency across client libraries. googleapis/java-spanner#4141