Skip to content

chore(spanner): attach Request ID headers to Spanner RPCs - #6248

Open
olavloite wants to merge 1 commit into
googleapis:mainfrom
olavloite:spanner-wire-request-id
Open

chore(spanner): attach Request ID headers to Spanner RPCs#6248
olavloite wants to merge 1 commit into
googleapis:mainfrom
olavloite:spanner-wire-request-id

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

Automatically attach unique x-goog-spanner-request-id headers to Spanner queries and reads, tracking attempt numbers across unary and streaming RPC retries.

@olavloite
olavloite requested review from a team as code owners July 31, 2026 19:16
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Jul 31, 2026
@olavloite
olavloite requested a review from sakthivelmanii July 31, 2026 19:16

@gemini-code-assist 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.

Code Review

This pull request implements automatic generation and propagation of Spanner request IDs across unary and streaming RPCs, including proper handling of retries and aborted transactions. The review feedback suggests optimizing the retry logic in ResultSet by using std::mem::take to avoid an unnecessary clone of GaxRequestOptions when updating the request ID header.

Comment thread src/spanner/src/result_set.rs Outdated
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.11972% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.08%. Comparing base (93d9a0f) to head (ad968fd).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/spanner/src/client.rs 97.64% 2 Missing ⚠️
src/spanner/src/request_id.rs 99.53% 2 Missing ⚠️
src/spanner/src/result_set.rs 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6248      +/-   ##
==========================================
+ Coverage   96.07%   96.08%   +0.01%     
==========================================
  Files         273      273              
  Lines       68742    69267     +525     
==========================================
+ Hits        66042    66557     +515     
- Misses       2700     2710      +10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@olavloite
olavloite force-pushed the spanner-wire-request-id branch from 4f67749 to 3e957d6 Compare July 31, 2026 19:35
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist 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.

Code Review

This pull request implements request ID propagation and tracking for Spanner RPCs (both unary and streaming), ensuring that retries are correctly tracked by incrementing the attempt suffix in the x-goog-spanner-request-id header. It introduces a SpannerRequestIdInterceptor to handle the headers, updates ResultSet to manually increment the attempt suffix on streaming retries, and adds comprehensive tests. The review feedback points out that applying #[allow(dead_code)] to the entire impl RequestIdCreator block silences compiler warnings for active production code, and suggests moving test-only methods to a separate block or marking them individually.

Comment thread src/spanner/src/request_id.rs Outdated
@olavloite
olavloite force-pushed the spanner-wire-request-id branch from 3e957d6 to 74f39a9 Compare August 1, 2026 10:10
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist 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.

Code Review

This pull request implements Spanner request ID generation and propagation across unary, streaming, and retried RPCs. It introduces a SpannerRequestIdInterceptor to append attempt numbers to the x-goog-spanner-request-id header, and updates ResultSet to manually increment the attempt suffix during streaming retries. A review comment identifies a potential integer overflow panic and incorrect channel ID mapping when using channel_hint + 1 directly, suggesting a safer modulo-based channel ID calculation instead.

Comment thread src/spanner/src/client.rs Outdated
Automatically attach unique `x-goog-spanner-request-id` headers to Spanner queries and reads, tracking attempt numbers across unary and streaming RPC retries.
@olavloite
olavloite force-pushed the spanner-wire-request-id branch from 74f39a9 to ad968fd Compare August 1, 2026 10:45
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist 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.

Code Review

This pull request implements robust propagation and retry handling for the x-goog-spanner-request-id header across unary and streaming Spanner RPCs. It introduces request ID generation, integrates it with various client operations, and updates the attempt interceptor and result set retry logic to correctly track and increment attempt numbers. Feedback is provided to optimize the stream retry logic in result_set.rs by avoiding unnecessary cloning of the HeaderMap unless a modification is actually required.

Comment thread src/spanner/src/result_set.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants