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`.
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.
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.