Repository navigation
perf(spanner): optimize built-in metrics hot path and harden concurrency - #18329
Conversation
Optimize hot-path execution in the Spanner client's built-in metrics
subsystem and resolve subtle regex boundary and concurrency issues:
- Correctness & Regex Parsing:
- Add negative lookbehind `(?<![a-zA-Z0-9_-])` to GFE and AFE timing
regexes to prevent false-positive matches (e.g. `safe; dur=55`).
- Guard regex evaluations in `extract_front_end_latencies` with fast string
containment checks (`"gfet4t7" in text`, `"afe" in text`).
- Extract helper `_extract_metric_latency` and flatten header detection.
- Caching & Allocations:
- Add bounded LRU cache (`maxsize=128`) for resource path parsing,
yielding a ~9x speedup matching the Java client's caching strategy.
- Add bounded LRU cache (`maxsize=64`) for RPC method name formatting.
- Introduce `_ObservableDict` to achieve zero-allocation steady-state
OpenTelemetry attribute caching with O(1) invalidation on mutation.
- Consolidate RPC attempt preparation in `MetricsInterceptor._prepare_attempt`.
- Concurrency & Resource Safety:
- Extract `_BaseAsyncResponseWrapper` and synchronize `_metrics_recorded`
via `threading.Lock` across `cancel()`, `__del__()`, and `_record_metrics()`.
- Safely close unawaited initial metadata coroutines on cancellation.
- Guarantee ContextVar token reset in `MetricsCapture.__exit__` via `finally`.
- Harden `_safe_decode_utf8` to return `""` when passed `None`.
![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 optimizes and hardens the Spanner metrics collection system. Key changes include caching resource path parsing and method name formatting using lru_cache, introducing thread-safe locking and cancellation handling in the response wrappers, caching OpenTelemetry attributes with an _ObservableDict to invalidate the cache on modifications, and refactoring metadata parsing to safely handle various formats. Extensive unit tests have been added to cover these optimizations and edge cases. There are no review comments to address, and I have no feedback to provide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request optimizes metrics collection in the Google Cloud Spanner client by introducing LRU caching for resource path parsing and method name formatting, adding thread-safe locking to response wrappers, and implementing an attribute caching mechanism in MetricsTracer using a custom _ObservableDict. The feedback highlights two important improvements: catching OverflowError alongside ValueError when parsing latency values to prevent potential crashes on extremely large numbers, and overriding the in-place union operator (__ior__) in _ObservableDict to ensure the cache invalidation callback is consistently triggered.
|
_async_intercept detects streams with hasattr(response, "anext"), but grpc.aio UnaryStreamCall/StreamStreamCall only define aiter. Real ExecuteStreamingSql/StreamingRead calls are wrapped in _AsyncUnaryResponseWrapper, so no metrics are recorded when the stream ends. With this PR's del change, every successful async stream is now recorded as CANCELLED when it is garbage-collected (it was OK before). The tests miss this because their mocks define anext |
@sinhasubham This is not a problem that is introduced in this PR, but an existing bug on Setting the status to |
Combines all non-draft, non-'do not merge' Spanner micro-optimization PRs: - #18329: perf(spanner): optimize built-in metrics hot path and harden concurrency - #18359: perf(spanner): optimize query parameter encoding with direct type dispatch - #18379: perf(spanner): build ExecuteSqlRequest on the raw protobuf message - #18408: perf(spanner): optimize request ID header generation and retry closures - #18420: perf(spanner): prune lock and begin event allocations on single-use snapshots - #18422: perf(spanner): avoid allocating empty RequestOptions on read and query paths - #18602: perf(spanner): optimize single-chunk results and single-row lookups - #18604: test(spanner): add tests for consuming PartialResultSet streams
🤖 I have created a release *beep* *boop* --- ## [3.72.0](google-cloud-spanner-v3.71.0...google-cloud-spanner-v3.72.0) (2026-11-12) ### Features * **dbapi:** wire timeout parameter through Connection to execute_sql ([#18484](#18484)) ([98fc304](98fc304)) * declare Python3.15 support ([8775794](8775794)) * **spanner:** auth login support for Spanner Omni endpoints ([#18273](#18273)) ([9e0548d](9e0548d)) * update API sources and regenerate ([#18797](#18797)) ([be4d954](be4d954)) ### Bug Fixes * **google-cloud-spanner:** clear buffered DDL statements on rollback and flush on autocommit ([#18636](#18636)) ([15d6a2c](15d6a2c)) * **google-cloud-spanner:** do not replay DDL statements on transaction retry ([#18625](#18625)) ([78b9da2](78b9da2)) * **spanner:** correctly wrap async streaming calls in metrics interceptor ([#18581](#18581)) ([2d19b0a](2d19b0a)) * **spanner:** eliminate duplicate metrics capture and measure full streaming latency ([#18605](#18605)) ([a7678fd](a7678fd)) * **spanner:** record streaming operation completion when stream finishes ([#18704](#18704)) ([ddeae47](ddeae47)) * **spanner:** release transaction lock if inline begin fails ([#18409](#18409)) ([dd24029](dd24029)) * **spanner:** stop deleting multiplexed sessions on rotation and close ([#18332](#18332)) ([c38e960](c38e960)) ### Performance Improvements * **spanner:** add fast-path for multiplexed session acquisition ([#18317](#18317)) ([9569b39](9569b39)) * **spanner:** avoid allocating empty RequestOptions on read and query paths ([#18422](#18422)) ([3743ff0](3743ff0)) * **spanner:** build ExecuteSqlRequest on the raw protobuf message ([#18379](#18379)) ([6d91fec](6d91fec)) * **spanner:** eliminate redundant tracing span in run_in_transaction ([#18357](#18357)) ([181e26a](181e26a)) * **spanner:** optimize built-in metrics hot path and harden concurrency ([#18329](#18329)) ([f2a791f](f2a791f)) * **spanner:** optimize query option merging and prevent in-place mutation ([#18358](#18358)) ([8cb3c6a](8cb3c6a)) * **spanner:** optimize query parameter encoding with direct type dispatch ([#18359](#18359)) ([f06497c](f06497c)) * **spanner:** optimize query result decoding and type resolution ([#18330](#18330)) ([ce72986](ce72986)) * **spanner:** optimize request ID header generation and retry closures ([#18408](#18408)) ([ad03b95](ad03b95)) * **spanner:** optimize row popping in StreamedResultSet ([#18316](#18316)) ([e66103d](e66103d)) * **spanner:** optimize single-chunk results and single-row lookups ([#18602](#18602)) ([05b3775](05b3775)) * **spanner:** prune lock and begin event allocations on single-use snapshots ([#18420](#18420)) ([ab56473](ab56473)) * **spanner:** short-circuit trace_call when tracing is inactive ([#18356](#18356)) ([58031be](58031be)) * **spanner:** support PartialResultSet.last with background stream draining ([#18320](#18320)) ([80efd63](80efd63)) ### Documentation * fix typos ([#18979](#18979)) ([5c8baeb](5c8baeb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Optimize hot-path execution in the Spanner client's built-in metrics subsystem and resolve subtle regex boundary and concurrency issues:
Correctness & Regex Parsing:
(?<![a-zA-Z0-9_-])to GFE and AFE timing regexes to prevent false-positive matches (e.g.safe; dur=55).extract_front_end_latencieswith fast string containment checks ("gfet4t7" in text,"afe" in text)._extract_metric_latencyand flatten header detection.Caching & Allocations:
maxsize=128) for resource path parsing, yielding a ~9x speedup matching the Java client's caching strategy.maxsize=64) for RPC method name formatting._ObservableDictto achieve zero-allocation steady-state OpenTelemetry attribute caching with O(1) invalidation on mutation.MetricsInterceptor._prepare_attempt.Concurrency & Resource Safety:
_BaseAsyncResponseWrapperand synchronize_metrics_recordedviathreading.Lockacrosscancel(),__del__(), and_record_metrics().MetricsCapture.__exit__viafinally._safe_decode_utf8to return""when passedNone.