Repository navigation
feat(storage): integrate OpenTelemetry metrics gating across clients and transfer_manager - #18577
shradhakatyal wants to merge 6 commits into
Conversation
- Add _opentelemetry_metrics.py with internal dev gate and environment variable parsing - Implement is_metrics_enabled and is_advanced_metrics_enabled evaluation helpers - Provide get_common_attributes and get_meter helpers for OpenTelemetry metrics - Add comprehensive unit tests in test__opentelemetry_metrics.py
- Set _ENABLE_METRICS_DEV_GATE to True in test_opentelemetry_import_error_on_load - Wrap importlib.reload(_opentelemetry_metrics) cleanup in a try...finally block
…th OTEL convention - Rename GCP_STORAGE_PYTHON_ENABLE_DEBUG_METRICS to GCP_STORAGE_PYTHON_ENABLE_OTEL_DEBUG_METRICS - Rename ENABLE_DEBUG_METRICS_ENV_VAR constant to ENABLE_OTEL_DEBUG_METRICS_ENV_VAR - Update corresponding unit tests in test__opentelemetry_metrics.py
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry metrics support to the Google Cloud Storage (GCS) Python client. It adds a new _opentelemetry_metrics module to manage metrics gating, environment variable configuration, and dependency checks. The Client, GrpcClient, and AsyncGrpcClient classes are updated to accept enable_metrics and enable_advanced_metrics parameters, and transfer_manager.py is updated to support pickling these new properties. Feedback on the pull request suggests improving the robustness of client unpickling in transfer_manager.py by dynamically mapping extra positional arguments to keyword arguments when len(args) > 6, preventing potential TypeError exceptions during version mismatches.
| if len(args) >= 8: | ||
| kwargs.setdefault("enable_metrics", args[6]) | ||
| kwargs.setdefault("enable_advanced_metrics", args[7]) | ||
| args = args[:6] |
There was a problem hiding this comment.
To ensure robust backward and forward compatibility (for example, if a client is unpickled from an older or intermediate version where len(args) is 7), we should avoid strict length checks like len(args) >= 8 which could result in passing too many positional arguments to Client.__init__ and raising a TypeError. Instead, check if len(args) > 6 and dynamically map any extra positional arguments to their respective keyword arguments, slicing args to the maximum of 6 positional arguments accepted by Client.__init__.
| if len(args) >= 8: | |
| kwargs.setdefault("enable_metrics", args[6]) | |
| kwargs.setdefault("enable_advanced_metrics", args[7]) | |
| args = args[:6] | |
| if len(args) > 6: | |
| kwargs.setdefault("enable_metrics", args[6]) | |
| if len(args) > 7: | |
| kwargs.setdefault("enable_advanced_metrics", args[7]) | |
| args = args[:6] |
- Decouple is_advanced_metrics_enabled from is_metrics_enabled - Gate get_meter on active standard or advanced metrics enablement - Handle empty/whitespace and unrecognized values in _parse_bool_env with a warning - Use client_setting parameter name in TypeError messages - Expand unit tests for independent advanced metrics, env var parsing, and get_meter
7ec58e8 to
7fd2242
Compare
… and missing OTel tests - Rename client_setting to enable_metrics and enable_advanced_metrics in gating functions - Expand get_meter docstring with Args and Returns sections - Verify explicit enablement flags return False/None when opentelemetry is missing
…and transfer_manager - Add enable_metrics and enable_advanced_metrics to Client, GrpcClient, and AsyncGrpcClient - Add metrics_enabled and advanced_metrics_enabled properties across all client classes - Preserve metrics configuration across process boundaries in transfer_manager - Add unit tests in test_client.py, test_grpc_client.py, test_async_grpc_client.py, and test_transfer_manager.py
7fd2242 to
93b878c
Compare
This branch has not been deployed
1 participant
Integrate the OpenTelemetry metrics gating configuration across Google Cloud Storage client classes and
transfer_manager.enable_metricsandenable_advanced_metricsparameters toClient,GrpcClient, andAsyncGrpcClient.metrics_enabledandadvanced_metrics_enabledproperties across all three client classes.enable_metricsandenable_advanced_metricsacross multi-process boundaries intransfer_manager(_reduce_clientand_LazyClient).test_client.py,test_grpc_client.py,test_async_grpc_client.py, andtest_transfer_manager.py.Note
Stacked on top of #18407. To review only the changes introduced in this PR before #18407 merges, view commit
7ec58e87d3e.