chore(storage): support DISABLE_GCS_PYTHON_CLIENT_OTEL_BUCKET_METADATA environment variable flag#17248
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the DISABLE_BUCKET_MD_IN_OTEL environment variable to allow users to explicitly disable GCS destination annotations in OpenTelemetry traces. While the implementation successfully achieves this, the current approach evaluates the environment variable on every single API request and trace span helper call, which introduces unnecessary overhead. To optimize performance, these checks should be inlined into the conditional blocks to leverage Python's short-circuit evaluation. Additionally, the system test should be refactored to use pytest's monkeypatch fixture instead of manual environment variable manipulation to ensure robust cleanup.
…ting and refactor system tests to use pytest monkeypatch
7dadd37 to
0866ac4
Compare
| and client._bucket_metadata_cache | ||
| and not ( | ||
| os.environ.get("DISABLE_BUCKET_MD_IN_OTEL", "").lower() | ||
| in ("1", "true", "yes", "on") |
There was a problem hiding this comment.
Instead of checking for all values, can't we just document the usage of this environment variable instead?
There was a problem hiding this comment.
I kept consistent with what already existing for Otel in python -
the way it was checked lacked readablity. I've now made it a bit more readable.
| and hasattr(client, "_bucket_metadata_cache") | ||
| and client._bucket_metadata_cache | ||
| and not ( | ||
| os.environ.get("DISABLE_BUCKET_MD_IN_OTEL", "").lower() |
There was a problem hiding this comment.
Is this change related to the feature guarded by ENABLE_GCS_PYTHON_CLIENT_OTEL_TRACES? If so, should we use consistent naming?
…N_CLIENT_OTEL_BUCKET_METADATA
This PR introduces the
DISABLE_GCS_PYTHON_CLIENT_OTEL_BUCKET_METADATAenvironment flag to dynamically disable injecting Cloud Storage bucket metadata destination attributes (gcp.resource.destination.idandgcp.resource.destination.location) inside OTel spans.