FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

perf(spanner): optimize built-in metrics hot path and harden concurrency by olavloite · Pull Request #18329 · googleapis/google-cloud-python · GitHub

  1. googleapis
  2. google-cloud-python

Repository navigation

perf(spanner): optimize built-in metrics hot path and harden concurrency - #18329

Open
olavloite wants to merge 5 commits into
mainfrom
spanner-optimize-built-in-metrics-hot-path
ConversationCommits (5)ChecksFiles changed
Open

perf(spanner): optimize built-in metrics hot path and harden concurrency#18329
olavloite wants to merge 5 commits into
mainfrom
spanner-optimize-built-in-metrics-hot-path

Conversation

olavloite commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

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.

perf(spanner): optimize built-in metrics hot path and harden concurrency
71c0fdc 
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`.
olavloite requested a review from a team as a code owner September 10, 2026 08:30
gemini-code-assist Bot reviewed Sep 10, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

sinhasubham commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

/gemini review

gemini-code-assist Bot reviewed Sep 29, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread packages/google-cloud-spanner/google/cloud/spanner_v1/metrics/metrics_tracer.py
Comment thread packages/google-cloud-spanner/google/cloud/spanner_v1/metrics/metrics_tracer.py

sinhasubham commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

_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
I understand that the existing code was already quietly failing to record completion metrics for async streams (which needs to be fixed anyway), but because of this new del change, those silent drop-offs are now actively being recorded as CANCELLED.

olavloite commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

_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 I understand that the existing code was already quietly failing to record completion metrics for async streams (which needs to be fixed anyway), but because of this new del change, those silent drop-offs are now actively being recorded as CANCELLED.

@sinhasubham This is not a problem that is introduced in this PR, but an existing bug on main that should not be fixed here, but rather in a separate PR. I've opened #18581 for that.

Setting the status to Cancelled when __del__ is called for a stream that has not finished is correct.

This branch has not been deployed

No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL