Skip to content

feat(storage): add OpenTelemetry standard metrics for Client, Bucket, and resource APIs - #18941

Draft
shradhakatyal wants to merge 8 commits into
googleapis:mainfrom
shradhakatyal:feat/gcs-otel-veneer-bucket-metrics
Draft

shradhakatyal wants to merge 8 commits into
googleapis:mainfrom
shradhakatyal:feat/gcs-otel-veneer-bucket-metrics

Conversation

@shradhakatyal

@shradhakatyal shradhakatyal commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds logical OpenTelemetry standard metrics instrumentation across Client, Bucket, Batch, ACL, HMACKeyMetadata, and BucketNotification JSON APIs in google-cloud-storage.

Stack Overview (review isolated commit 47160ae48f6 for this PR):

Key Changes

  • Client & Bucket Operations (client.py, bucket.py): Instruments logical operations across bucket creation, lookup, metadata updates, IAM policies, lifecycle/retention helpers, and lazy HTTPIterator list APIs (ListBuckets, ListObjects, ListHmacKeys, ListNotifications).
  • Batch Execution (batch.py): Records logical operation metrics on Batch.finish() while deferring queued sub-requests.
  • Resource Classes (acl.py, hmac_key.py, notification.py): Instruments BucketACL, DefaultObjectACL, ObjectACL, HMACKeyMetadata, and BucketNotification CRUD operations.

…d helpers

Implement core OpenTelemetry standard metrics infrastructure for GCS:
- Add _MetricsRecorder with the 9 standard metric instruments
- Add custom latency (165 buckets) and byte-size (45 buckets) histogram views
- Add classify_error_type for low-cardinality error.type mapping
- Add normalize_url_template for bounded HTTP url.template routing
- Add start_operation ContextVar for outermost logical operation tracking and nested call suppression

[Generated-by: AI]
- Add gcp.storage.client.retries counter and rename TTFB metric to gcp.storage.client.ttfb
- Remove gcp.client.repo from _COMMON_ATTRIBUTES
- Update _build_size_buckets to generate 48 buckets matching Go and Java SDKs
- Align per-metric attributes across logical operation, RPC/HTTP attempt, error, retry, and TTFB recorders

[Generated-by: AI]
…fy CONNECTION_ERROR

- Scope histogram View registrations to meter_name="google.cloud.storage"
- Guard normalize_url_template and _extract_server_address against malformed URIs and gRPC authority schemes
- Consolidate CONNECTION_REFUSED, CONNECTION_RESET, and CONNECTIVITY error labels into CONNECTION_ERROR

[Generated-by: AI]
…port

- Wire _MetricsRecorder lifecycle, enable_metrics, and meter_provider into Client
- Instrument Connection.api_request for per-attempt HTTP duration, TTFB, retries, and errors
- Add _start_storage_operation, _PropertyMixin, and lazy HTTPIterator page metrics helpers
- Skip per-attempt HTTP metrics while queuing sub-requests into Batch
- Suppress internal background BucketMetadataCache lookups from emitting user-facing metrics
… and resource APIs

- Instrument logical operations on Client and Bucket metadata, IAM, lifecycle, and list APIs
- Record Batch.finish() logical operation metrics while deferring queued sub-requests
- Instrument BucketACL, DefaultObjectACL, ObjectACL, HMACKeyMetadata, and BucketNotification

@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 comprehensive OpenTelemetry standard metrics instrumentation across the Google Cloud Storage Python client, including route normalization, canonical error classification, and custom histogram views. It integrates metric recording into HTTP requests, bucket operations, ACLs, HMAC keys, and notifications, while adding extensive unit tests to verify transport and veneer metrics. The reviewer recommended wrapping attribute accesses in the exception chain traversal helper (_iter_exception_chain) with try...except blocks and logging any failures to prevent unexpected exceptions from propagating and masking original user exceptions.

Comment on lines +367 to +385
for attr_name in (
"__cause__",
"__context__",
"cause",
"reason",
"original_exception",
):
nested = getattr(current, attr_name, None)
if isinstance(nested, BaseException) and id(nested) not in visited:
visited.add(id(nested))
queue.append(nested)

for seq_attr in ("args", "errors"):
seq = getattr(current, seq_attr, ())
if isinstance(seq, (tuple, list)):
for item in seq:
if isinstance(item, BaseException) and id(item) not in visited:
visited.add(id(item))
queue.append(item)

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.

medium

To prevent any unexpected exceptions during exception chain traversal from propagating and masking the original user exception, it is recommended to wrap the attribute accesses in try...except Exception blocks. However, avoid silently ignoring the exception with a broad except Exception: pass block. Instead, log the exception (e.g., using logger.warning) to aid in debugging and prevent masking underlying issues.

        for attr_name in (
            "__cause__",
            "__context__",
            "cause",
            "reason",
            "original_exception",
        ):
            try:
                nested = getattr(current, attr_name, None)
                if isinstance(nested, BaseException) and id(nested) not in visited:
                    visited.add(id(nested))
                    queue.append(nested)
            except Exception as e:
                logger.warning("Failed to traverse exception attribute %s: %s", attr_name, e)

        for seq_attr in ("args", "errors"):
            try:
                seq = getattr(current, seq_attr, ())
                if isinstance(seq, (tuple, list)):
                    for item in seq:
                        if isinstance(item, BaseException) and id(item) not in visited:
                            visited.add(id(item))
                            queue.append(item)
            except Exception as e:
                logger.warning("Failed to traverse exception sequence attribute %s: %s", seq_attr, e)
References
  1. Avoid broad except Exception: blocks that silently return None or pass. Instead, log the exception (e.g., using logger.warning) to aid in debugging and prevent masking underlying issues.

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