Repository navigation
perf(spanner): eliminate redundant tracing span in run_in_transaction - #18357
Conversation
![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 refactors transaction execution and tracing in Google Cloud Spanner sessions (both sync and async). It extracts transaction creation and abort handling into helper methods (_create_transaction_for_attempt and _handle_aborted), removes the CloudSpanner.Session.run_in_transaction span in favor of using the current span, and updates add_span_event to internally check if the span is recording. The reviewer points out that since add_span_event now internally guards against None or non-recording spans, the explicit if span and span.is_recording(): checks at several call sites in session.py are redundant and should be removed to simplify the code.
634e058 to
cf47962
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the run_in_transaction method in both the synchronous and asynchronous session implementations by extracting transaction creation and aborted error handling into helper methods (_create_transaction_for_attempt and _handle_aborted). This change simplifies the tracing logic by removing the CloudSpanner.Session.run_in_transaction span and attaching retry events directly to the current active span. Additionally, safe checks were added to OpenTelemetry tracing, and corresponding system and unit tests were updated. Feedback on the changes suggests addressing a potential IndexError in _handle_aborted when accessing exception.errors[0] directly, as exception.errors can sometimes be empty.
cf47962 to
3d6358f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors transaction retry and tracing logic in Google Cloud Spanner sessions. It introduces helper methods _create_transaction_for_attempt and _handle_aborted to simplify run_in_transaction in both synchronous and asynchronous session implementations. It also updates tracing to use get_current_span() instead of creating a new nested span, and updates the corresponding unit and system tests. Feedback on the changes highlights a missing import for get_current_span in _async/session.py which would cause a NameError at runtime.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the run_in_transaction method in both synchronous and asynchronous Session classes by extracting transaction creation into _create_transaction_for_attempt and aborted error handling into _handle_aborted. It also removes the redundant CloudSpanner.Session.run_in_transaction span, updates helper functions to safely handle exceptions without an errors attribute, and updates the test suite accordingly. The review feedback suggests simplifying the ternary expressions used to extract the exception cause in session.py and _helpers.py by assigning the result of getattr to a local variable first to avoid redundant attribute lookups.
Eliminate the redundant inner 'CloudSpanner.Session.run_in_transaction' span and duplicate MetricsCapture context, leaving 'CloudSpanner.Database.run_in_transaction' as the single transaction lifecycle span (matching Java and Go client behavior). - Attach transaction retry, abort, and rollback events directly to the active span. - Guard span event creation, retry delay computation, and exception stringification behind span.is_recording(). - Refactor the run_in_transaction retry loop by extracting _create_transaction_for_attempt and _handle_aborted helpers.
3d6358f to
dc23e97
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors transaction execution and tracing in Google Cloud Spanner. It extracts transaction creation and aborted error handling into helper methods (_create_transaction_for_attempt and _handle_aborted) in both synchronous and asynchronous session implementations. It also simplifies the OpenTelemetry tracing hierarchy by removing the CloudSpanner.Session.run_in_transaction span and instead attaching events directly to the active parent span. Additionally, it updates and adds unit and system tests to verify the new tracing behavior, and makes minor robustness improvements to generate.py and exception attribute lookups. There are no review comments, so I have no feedback to provide.
olavloite
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
🤖 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>
Eliminate the redundant inner 'CloudSpanner.Session.run_in_transaction' span and duplicate MetricsCapture context, leaving 'CloudSpanner.Database.run_in_transaction' as the single transaction lifecycle span (matching Java and Go client behavior).