Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the built-in metrics initialization and tracing lifecycle in the Spanner client, particularly addressing contextvar reset issues during early termination of resumable streams in sync and async snapshots. It updates _initialize_metrics to support an emulator_host parameter, handles AnonymousCredentials, and adds fallback handling for OpenTelemetry's AlwaysOffExemplarFilter. Additionally, _restart_on_unavailable is updated to manage the metrics_tracer lifecycle directly, and ping operations are wrapped with MetricsCapture. Feedback on the changes suggests simplifying MetricsCapture.__exit__ by removing defensive getattr checks for attributes that are already guaranteed to be initialized in __init__.
dbbf425 to
5a5a43d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the metrics collection and tracing lifecycle in the Google Cloud Spanner Python client. Key changes include updating _initialize_metrics to prevent initialization when using an emulator host or anonymous credentials, integrating MetricsCapture into session ping operations, and managing the metrics_tracer lifecycle within _restart_on_unavailable for both synchronous and asynchronous snapshots. Additionally, MetricsCapture was updated to safely handle context resets and suppress telemetry export errors, and SpannerMetricsTracerFactory was adjusted to preserve its enabled state across singleton instantiations. Comprehensive unit and integration tests were added to verify these changes. I have no feedback to provide as there are no review comments.
5a5a43d to
ed3575e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and enhances the metrics collection and tracing mechanisms in the Google Cloud Spanner Python client for both synchronous and asynchronous operations. It introduces a helper function _resource_info_from_database to safely extract and cache resource information (project, instance, database) and integrates SpannerMetricsTracerFactory and MetricsCapture to handle operation lifecycles without contaminating ambient context variables. Additionally, extensive unit and integration tests have been added to verify metrics behavior, including error resilience and the prevention of duplicate metrics. Feedback on the changes suggests optimizing the retrieval of _resource_info in _helpers.py by using getattr to avoid double evaluation of the property.
ed3575e to
3ec1b95
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and improves the built-in metrics collection and tracing lifecycle across both synchronous and asynchronous paths in the Google Cloud Spanner client. Key updates include caching and centralizing resource information retrieval, integrating MetricsCapture directly into operations like ping and _restart_on_unavailable to handle retries and early termination cleanly, and skipping metrics initialization when using an emulator or anonymous credentials. Comprehensive tests have also been added to verify these behaviors. The code review feedback suggests simplifying the resource information retrieval in both database.py and _async/database.py by accessing guaranteed attributes directly instead of using defensive getattr checks.
3ec1b95 to
c75b8d6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the built-in metrics and tracing implementation in the Google Cloud Spanner Python client. It consolidates resource information extraction into a helper function _resource_info_from_database to eliminate duplicate properties across Batch, Pool, Session, and Snapshot classes. It also updates MetricsCapture and the _restart_on_unavailable generator to safely handle cases where OpenTelemetry is not installed, prevent context variable pollution, and ensure robust cleanup during early termination. The feedback identifies a potential context leak in MetricsCapture.__enter__ if tracer.record_operation_start() raises an exception, suggesting a try...except block to reset the tracer token upon failure.
…ming latency, and cache resource attributes Eliminate redundant operation metrics capture across unary RPCs and streaming queries, measure streaming query latency accurately across full stream lifetimes, and optimize per-RPC metrics overhead through cached resource attributes. Problems Fixed: - Unary RPCs were capturing operation metrics twice: trace_call unconditionally entered MetricsCapture, while caller methods across Database, Session, Snapshot, Transaction, and Batch also wrapped their calls in MetricsCapture. This doubled operation counts and latencies per RPC, creating one legitimate record and one phantom record without resource labels. - Streaming operations (execute_sql and read) exited MetricsCapture immediately after receiving the response iterator instead of waiting for rows to finish streaming, resulting in near-zero operation latencies and duplicate operation records on stream retries. - Every RPC traversed the object hierarchy (session -> database -> instance -> client) to extract project, instance, and database strings, followed by dictionary allocations and repeated setter calls on every tracer instance. Key Changes: - Decoupled tracing from metrics capture: Removed MetricsCapture from trace_call so tracing strictly manages spans, while callers handle operation metrics once with full resource attributes. - Measured full streaming duration: Restructured _restart_on_unavailable so that an operation spans the full lifetime of the stream, recording completion in a finally block when the stream finishes, fails, or is cancelled. Stream retries are recorded as attempts of the same logical operation. - Cached resource attributes on Database: Database._resource_info pre-merges project, instance, and database labels with client-level attributes once into a cached dictionary. create_metrics_tracer simply clones this dictionary, eliminating per-RPC object traversal and setter calls. - Consolidated session resource resolution: Added _resource_info to _SessionWrapper in _helpers.py and delegated to _resource_info_from_database, eliminating duplicate property definitions across Snapshot, Batch, MutationGroups, Session, and SessionPool. - Short-circuited metrics capture when OpenTelemetry is not installed to bypass factory instantiation and ContextVar operations. - Added comprehensive unit and in-memory mock server tests verifying metrics accuracy, caching behavior, fallback paths, and stream resumption.
c75b8d6 to
5d7e099
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors and enhances the built-in metrics collection and OpenTelemetry tracing implementation in the Google Cloud Spanner Python client. Key changes include centralizing resource information extraction via a new helper function _resource_info_from_database, optimizing metric initialization to skip setup when using an emulator or anonymous credentials, and improving the robustness of the MetricsCapture context manager to handle start/completion exceptions and environment configurations where OpenTelemetry is missing. Additionally, the snapshot query retry loop (_restart_on_unavailable) has been updated to correctly manage the lifecycle of metrics tracers across retries and early terminations. Comprehensive unit and integration tests have been added to validate these behaviors. I have no further feedback to provide as no review comments were submitted.
Eliminate redundant operation metrics capture across unary RPCs and streaming
queries, measure streaming query latency accurately across full stream lifetimes,
and optimize per-RPC metrics overhead through cached resource attributes.
Problems Fixed:
entered MetricsCapture, while caller methods across Database, Session,
Snapshot, Transaction, and Batch also wrapped their calls in MetricsCapture.
This doubled operation counts and latencies per RPC, creating one legitimate
record and one phantom record without resource labels.
after receiving the response iterator instead of waiting for rows to finish
streaming, resulting in near-zero operation latencies and duplicate operation
records on stream retries.
client) to extract project, instance, and database strings, followed by
dictionary allocations and repeated setter calls on every tracer instance.
Key Changes:
so tracing strictly manages spans, while callers handle operation metrics once
with full resource attributes.
an operation spans the full lifetime of the stream, recording completion in a
finally block when the stream finishes, fails, or is cancelled. Stream retries
are recorded as attempts of the same logical operation.
project, instance, and database labels with client-level attributes once into a
cached dictionary. create_metrics_tracer simply clones this dictionary,
eliminating per-RPC object traversal and setter calls.
_SessionWrapper in _helpers.py and delegated to _resource_info_from_database,
eliminating duplicate property definitions across Snapshot, Batch,
MutationGroups, Session, and SessionPool.
factory instantiation and ContextVar operations.
Closes #16495
Fixes #16173