feat: Add Feature Gating configuration helpers.#17524
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces observability environment variable and client options resolution helpers under a new observability module, including the is_signal_enabled function and corresponding unit tests. Feedback is provided to improve the resolution of client options by avoiding direct __dict__ access (which fails for classes using __slots__ or properties) in favor of getattr, and replacing rstrip('s') with a safer suffix removal method to prevent unintended character stripping.
53085c1 to
9aa54fb
Compare
|
While it is likely that this PR is complete, we want to wait until the design doc (being handled internally) is complete and approved before merging. |
… follow precedence
db771c1 to
0f92c35
Compare
| def resolve_feature_flags( | ||
| env_var: str, | ||
| provider_key: str, | ||
| client_options: Optional[Union[Dict[str, Any], Any]] = None, |
There was a problem hiding this comment.
I assume Any is for ClientOptions? Why not use the class directly?
There was a problem hiding this comment.
Redundant
This hint is a bit redundant. Since Any covers everything (including dictionaries), putting Dict[str, Any] inside the Union with Any doesn't restrict anything for a type checker.
Optional[Union[Dict[str, Any], Any]]Clarity
If we change it to:
client_options: Optional[Union[Dict[str, Any], ClientOptions]] = NoneWe do get some clarity. An engineer looking at the function signature knows exactly what is expected in one scenario.
Duck Typing
However, there is a reason to prefer Any (or object) here. It gives our customers flexibility to use their own custom options classes, as long as they implement the interface our code expects (i.e they can use duck typing).
In our implementation of _has_provider:
if isinstance(client_options, dict):
return client_options.get(provider_key) is not None
return getattr(client_options, provider_key, None) is not NoneWe use getattr(). This means our helper doesn't care if the object is strictly a ClientOptions. It can be any object that happens to have the attribute we are looking for. We can see this intent in our tests in test_feature_gating_helpers.py:
class _MockOptions:
def __init__(self):
self.other_option = "value"We pass this _MockOptions object to resolve_feature_flags. It is NOT a subclass of ClientOptions.
If we change the type hint to ClientOptions, strict type checkers (like mypy) will claim that _MockOptions is not a valid argument.
If we keep it as Any (or object), the type checker stays quiet because it understands we are using duck typing.
There was a problem hiding this comment.
How common do you expect that use case to be? Will customers even have reason to call this code? And if they did, couldn't they also achieve this by either:
- subclassing
ClientOptionswith their own implementation - passing in a dict
- using
typing.cast
Personally, I think allowing arbitrary types here feels overkill, and potentially dangerous. Dict[str, Any] | ClientOptions makes more sense to me. Or really, ideally just one or the other
But it sounds like you have plans to extend this in the future, so maybe I'm missing context. If you want to keep it fully permissive, I'd suggest:
- rename
client_optionsto something more general - use
objectinstead ofAny(since only objects havegetattr) - include
ClientOptionsin the type annotation, for the same reason you includeDict
There was a problem hiding this comment.
We included ClientOptions in the type annotation.
| """ | ||
|
|
||
| # Check for programmatic feature provider | ||
| has_provider = _has_provider(client_options, provider_key) |
There was a problem hiding this comment.
nit: this helper wouldn't be needed if you can simplify the client_options type (either by disallowing dicts, or doing a one-liner to convert them)
options = ClientOptions.from_dict(client_options) if isinstance(client_options, dict) else client_options
has_provider = getattr(options, provider_key, None) is not None
There was a problem hiding this comment.
I prefer to keep the code in the resolver lean and extensible thus the choice to use helper functions like _has_provider
Especially considering that at a future time I would like to add the ability for the resolver to resolve:
- legacy environment variables (such as multiple env vars that we already have in Spanner and Storage)
In addition, there is also some benefit to being able to set env vars on a language level (turn on o11y in Python by not Node) OR on a per-service level (turn on o11y in Bigtable but not Firestore).
Being able to slip tiny helper functions into the resolver helps keep the resolution logic and precedence order the main priority AND not "how should we determine if something is set OR not set and how is that different from how we parsed providers 12 lines up OR parsed this customer env var"
- Language-wide env vars (e.g.,
GOOGLE_CLOUD_<LANGUAGE>_TRACING_ENABLED) - Service-Specific env vars (e.g.,
GOOGLE_CLOUD_<SERVICE>_TRACING_ENABLED - etc
If we are running this function multiple times in a row (e.g., during client initialization to check GOOGLE_SDK_PYTHON_TRACING_ENABLED, then GOOGLE_SDK_PYTHON_METRICS_ENABLED, etc.), we want resolve_feature_flags to be straight forward for troubleshooting and maintenance.
There was a problem hiding this comment.
It sounds like the function signature for resolve_feature_flags could be changing in the future then? Do you know what it would look like? We have to be careful about breaking changes, so if there's anything we can do now to make it more flexible, that would be great
I don't completely understand the plan yet, but a couple things to consider:
client_optionscould be given a more general name (like configuration), if we expect other config types in the future- We could add
*to make client_options keyword only, so we can insert others in any order later env_varandclient_optionscould accept lists instead of single items (although I think we could add this without a breaking change later)
There was a problem hiding this comment.
These are good ideas for future-proofing. We agree on making arguments keyword-only and using more general names. We updated the signature to make all arguments keyword-only (placing * at the start) to prevent accidental parameter swapping and allow easy expansion later. We have also renamed provider_key to feature_key and client_options to configuration to reflect that this is a generic internal helper.
Regarding lists: the resolver is currently intended to be a simple one-specific-input, one-specific-output (e.g. a boolean) gate, so the idea of passing in lists of configs is not in line with the expected use. If we need to resolve several gates, we expect to call it multiple times, keeping the logic simple:
is_tracing_enabled = resolve_feature_flags(env_var="GOOGLE_CLOUD_PYTHON_TRACING_ENABLED")
is_metrics_enabled = resolve_feature_flags(env_var="GOOGLE_CLOUD_EXPERIMENTAL_PYTHON_METRICS_ENABLED", feature_key="metrics_provider", configuration=client_options)
is_creds_feature_enabled = resolve_feature_flags(env_var="GOOGLE_CLOUD_CREDENTIALS_FEATURE_ENABLED", feature_key="credential_type", configuration=credentials)
Removing the do not merge label based on progress in the LLD. |
daniel-sanche
left a comment
There was a problem hiding this comment.
Nothing blocking, but I left a few comments to consider, for things that could be hard to change after merging
If none of them seem concerning, I'm good to approve as-is
|
|
||
| def resolve_feature_flags( | ||
| env_var: str, | ||
| provider_key: str, |
There was a problem hiding this comment.
nit: I wonder if the provider naming is general enough? "Provider" is the term used for Otel, but does it apply equally to all features? Maybe this should be called feature_key? Or flag_id?
There was a problem hiding this comment.
We updated the arg name.
| if "EXPERIMENTAL" in env_var: | ||
| # Fail Fast if provider present but experimental environment variable is not enabled | ||
| if env_var_setting is not True and has_provider: | ||
| raise ValueError( |
There was a problem hiding this comment.
Do you think a custom error would be helpful here? ValueError seems a like an imperfect fit for this kind of thing. And, assuming this is being called as part of client init, we may be rasing other ValueErrors for other reasons, so it might be good to differentiate them some more
There was a problem hiding this comment.
We updated to a custom error.
| def resolve_feature_flags( | ||
| env_var: str, | ||
| provider_key: str, | ||
| client_options: Optional[Union[Dict[str, Any], Any]] = None, |
There was a problem hiding this comment.
How common do you expect that use case to be? Will customers even have reason to call this code? And if they did, couldn't they also achieve this by either:
- subclassing
ClientOptionswith their own implementation - passing in a dict
- using
typing.cast
Personally, I think allowing arbitrary types here feels overkill, and potentially dangerous. Dict[str, Any] | ClientOptions makes more sense to me. Or really, ideally just one or the other
But it sounds like you have plans to extend this in the future, so maybe I'm missing context. If you want to keep it fully permissive, I'd suggest:
- rename
client_optionsto something more general - use
objectinstead ofAny(since only objects havegetattr) - include
ClientOptionsin the type annotation, for the same reason you includeDict
| """ | ||
|
|
||
| # Check for programmatic feature provider | ||
| has_provider = _has_provider(client_options, provider_key) |
There was a problem hiding this comment.
It sounds like the function signature for resolve_feature_flags could be changing in the future then? Do you know what it would look like? We have to be careful about breaking changes, so if there's anything we can do now to make it more flexible, that would be great
I don't completely understand the plan yet, but a couple things to consider:
client_optionscould be given a more general name (like configuration), if we expect other config types in the future- We could add
*to make client_options keyword only, so we can insert others in any order later env_varandclient_optionscould accept lists instead of single items (although I think we could add this without a breaking change later)
| if isinstance(client_options, dict): | ||
| return client_options.get(provider_key) is not None | ||
|
|
||
| return getattr(client_options, provider_key, None) is not None |
There was a problem hiding this comment.
Maybe we should have some limitations provider_key? Like returning False for __?
Something like __class__ or __dict__ would return True, but probably not what we're looking for
There was a problem hiding this comment.
We added protections against reading dunder_methods.
chalmerlowe
left a comment
There was a problem hiding this comment.
Removed refs to otel in constraints.txt files.
This PR introduces a foundational configuration layer for integrating General Feature gating into Google Cloud Python client libraries.
Intent & Motivation
We are about be embark on adding observability (o11y) to entire suite of Cloud SDK libraries. To enable this functionality on an opt-in basis we are prepping a general feature gating capability. While the initial push is towards enabling o11y, we recognize that this can and should be more general than that.
Changes Included
resolve_feature_flagfunction in the new_feature_gating_helperspackage to resolve feature gates:Resolves settings in the following order of precedence:
1. Programmatic overrides in configurations (checks for feature_key values)
2. Language-wide Environment Variable:
GOOGLE_CLOUD_PYTHON_<FEATURE>_ENABLED(natively checks for an
EXPERIMENTALtoken first e.g.GOOGLE_CLOUD_[EXPERIMENTAL]_PYTHON_<FEATURE>_ENABLED)3. Default fallback
Experimental Fallbacks: Implemented fallback logic that automatically checks for
EXPERIMENTALprefixes within the environment variable definitions to support early-stage adoption and rollout phases safely.Test Coverage: Provided test coverage using
pytestfor precedence rules and environmental scenarios.