feat(x-goog-spanner-request-id): implement Request-ID #1264

odeke-em · 2024-12-11T14:45:57Z

Implements x-goog-spanner-request-id propagation for end-to-end debugging.

conventional-commit-lint-gcf · 2024-12-11T14:46:05Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

This change introduces AtomicCounter, a concurrency/thread-safe counter do deal with the multi-threaded nature of variables. It permits operations: * atomic_counter += 1 * value = atomic_counter + 1 * atomic_counter.value that'll be paramount to bringing in the logic for x-goog-spanner-request-id in much reduced changelists. Updates googleapis#1261 Carved out from PR googleapis#1264

* feat(x-goog-spanner-request-id): introduce AtomicCounter This change introduces AtomicCounter, a concurrency/thread-safe counter do deal with the multi-threaded nature of variables. It permits operations: * atomic_counter += 1 * value = atomic_counter + 1 * atomic_counter.value that'll be paramount to bringing in the logic for x-goog-spanner-request-id in much reduced changelists. Updates #1261 Carved out from PR #1264 * Tests for with_request_id * chore: remove sleep * chore: remove unused import --------- Co-authored-by: Knut Olav Løite <[email protected]>

google/cloud/spanner_v1/database.py

olavloite · 2024-12-19T15:02:58Z

google/cloud/spanner_v1/database.py

@@ -698,10 +728,20 @@ def execute_partitioned_dml(
                _metadata_with_leader_aware_routing(self._route_to_leader_enabled)
            )

+        # Attempt will be incremented inside _restart_on_unavailable.


I assume that this comment applies to partial_attempt, no? If so, can we move it down to that variable.

It applies to begin_txn_attempt and I've updated the comment and moving the comment before the variable itself. Thank you.

olavloite · 2024-12-24T11:24:38Z

google/cloud/spanner_v1/database.py

                    trace_name="CloudSpanner.ExecuteStreamingSql",
                    request=request,
                    transaction_selector=txn_selector,
                    observability_options=self.observability_options,
+                    attempt=begin_txn_attempt,


I think that this should be partial_attempt. Retries of this RPC will not retry the BeginTransaction RPC, it will just continue to use the same transaction ID as the one that was returned by the original BeginTransaction RPC.

Oooh, okay, that's very interesting and thanks for letting me know, let me make that update.

I did however already pass in and increment partial_attempt inside wrapped_method. I'll instead remove that and increment them in the appropriate sites.

google/cloud/spanner_v1/instance.py

google/cloud/spanner_v1/pool.py

tests/unit/test_request_id_header.py

olavloite · 2024-12-24T11:52:24Z

tests/unit/test_request_id_header.py

+        ]
+        assert got_stream_segments == want_stream_segments
+
+    def test_database_run_in_transaction_retries_on_abort(self):


This test does not appear to actually verify anything (other than that it does not cause any errors). Can we either remove it, or add verifications for the scenario it should cover?

olavloite · 2024-12-24T11:57:13Z

tests/unit/test_session.py

+                        ("x-goog-spanner-route-to-leader", "true"),
+                        (
+                            "x-goog-spanner-request-id",
+                            f"1.{REQ_RAND_PROCESS_ID}.{database.NTH_CLIENT.value}.1.2.2",


Is attempt set to 2 here because the transaction is retried? If so, then that is not correct. The attempt number should only be increased if the RPC itself is being retried due to a retryable error for that (single) RPC. E.g. if a ServiceUnavailable error is returned.

olavloite · 2024-12-24T11:58:10Z

tests/unit/test_session.py

+                        ("x-goog-spanner-route-to-leader", "true"),
+                        (
+                            "x-goog-spanner-request-id",
+                            f"1.{REQ_RAND_PROCESS_ID}.{database.NTH_CLIENT.value}.1.3.2",


Same here as for the BeginTransaction RPC: Transaction retries should not increase the attempt count. Only retries of a single RPC due to UNAVAILABLE errors (and other retryable error codes).

olavloite · 2025-03-25T10:07:51Z

google/cloud/spanner_v1/_helpers.py

+            return orig_getattribute(obj, key, *args, **kwargs)
+
+        attr = orig_getattribute(obj, key, *args, **kwargs)
+        print("args", args, "attr.dir", dir(attr))


nit: remove

olavloite · 2025-03-25T10:08:41Z

google/cloud/spanner_v1/_helpers.py

+        if mangled_or_private:
+            return attr
+
+        print("\033[35mattr", attr, "hex_id", hex(id(attr)), "\033[00m")


nit: remove

here and everywhere: remove print statements

olavloite · 2025-03-25T10:24:40Z

google/cloud/spanner_v1/pool.py

@@ -246,6 +246,7 @@ def bind(self, database):
            observability_options=observability_options,
            metadata=metadata,
        ) as span, MetricsCapture():
+            attempt = 1


This variable can (still) be removed, as it is unused.

olavloite · 2025-03-25T10:25:45Z

google/cloud/spanner_v1/session.py

-        metadata = _metadata_with_prefix(self._database.name)
+        database = self._database
+        api = database.spanner_api
+        database = self._database


nit: remove, it is already assigned two lines above

olavloite · 2025-03-25T10:30:36Z

google/cloud/spanner_v1/snapshot.py

+        if not isinstance(nth_request, int):
+            raise Exception(f"failed to get an integer back: {nth_request}")


Why do we need this here, but not in the read function above?

olavloite · 2025-03-25T12:43:28Z

tests/mockserver_tests/test_request_id_header.py

+    def test_streaming_retryable_error(self):
+        add_select1_result()
+        add_error(SpannerServicer.ExecuteStreamingSql.__name__, unavailable_status())
+        add_error(SpannerServicer.ExecuteStreamingSql.__name__, unavailable_status())


nit: I think that adding one error is enough

This change commits the scaffolding for which testing will be used. This is a carve out of PRs googleapis#1264 and googleapis#1364, meant to make those changes lighter and much easier to review then merge. Updates googleapis#1261

* chore(x-goog-request-id): commit testing scaffold This change commits the scaffolding for which testing will be used. This is a carve out of PRs #1264 and #1364, meant to make those changes lighter and much easier to review then merge. Updates #1261 * Use guard to keep x-goog-request-id interceptor docile in tests until activation later * AtomicCounter update * Remove duplicate unavailable_status that had been already committed into main

product-auto-label bot added the size: m Pull request size is medium. label Dec 11, 2024

product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner API. label Dec 11, 2024

odeke-em force-pushed the x-goog-spanner-request-id branch from 2451afe to 4f1da67 Compare December 12, 2024 08:13

odeke-em changed the title ~~feat(x-goog-spanner-request-id): implement Requst-ID~~ feat(x-goog-spanner-request-id): implement Request-ID Dec 12, 2024

odeke-em force-pushed the x-goog-spanner-request-id branch from b021c9e to 47da5e4 Compare December 14, 2024 13:13

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Dec 14, 2024

odeke-em force-pushed the x-goog-spanner-request-id branch from 8616572 to caa60c2 Compare December 16, 2024 12:23

odeke-em mentioned this pull request Dec 18, 2024

feat(x-goog-spanner-request-id): introduce AtomicCounter #1275

Merged

odeke-em force-pushed the x-goog-spanner-request-id branch from 9903cac to 529333a Compare December 18, 2024 14:25

odeke-em marked this pull request as ready for review December 18, 2024 14:26

odeke-em requested review from a team as code owners December 18, 2024 14:26

blunderbuss-gcf bot assigned thiagotnunes Dec 18, 2024

product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Dec 19, 2024

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 19, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 19, 2024

odeke-em force-pushed the x-goog-spanner-request-id branch from 2d6a3ea to 65757b5 Compare December 20, 2024 05:37

olavloite reviewed Dec 24, 2024

View reviewed changes

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 24, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 24, 2024

odeke-em force-pushed the x-goog-spanner-request-id branch from f8e0717 to 7fc3f3f Compare December 24, 2024 21:54

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 25, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 25, 2024

odeke-em force-pushed the x-goog-spanner-request-id branch from 07a00f9 to 70eeaef Compare December 25, 2024 14:03

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 26, 2024

olavloite reviewed Mar 25, 2025

View reviewed changes

olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2025

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2025

odeke-em force-pushed the x-goog-spanner-request-id branch from 4484cfd to 3bda544 Compare March 28, 2025 11:23

odeke-em requested a review from olavloite March 28, 2025 11:24

Initial ExecuteStreamingSql request in snapshot should have the header

f8ad94f

odeke-em force-pushed the x-goog-spanner-request-id branch from 3bda544 to f8ad94f Compare March 28, 2025 11:27

sakthivelmanii added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 28, 2025

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 28, 2025

odeke-em added 4 commits March 28, 2025 16:05

Consolidate retries for _restart_on_unavailable

15984cb

Fix missing variable declaration

a5fdebd

Adjust with updates

80faca0

Update _execute_partitioned_dml_helper

06c12a2

sakthivelmanii added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 1, 2025

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 1, 2025

Monkey patch updates

9d4d942

odeke-em mentioned this pull request Apr 30, 2025

chore(x-goog-request-id): commit testing scaffold #1366

Merged

Merge branch 'main' into x-goog-spanner-request-id

6ed0b74

product-auto-label bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels May 17, 2025

odeke-em added 3 commits May 17, 2025 13:32

Reduce unnecessary changes

c31c03f

Experiment with wrapping for gapic retries

e67cc9e

Remove duplicate code

dab92dd

odeke-em force-pushed the x-goog-spanner-request-id branch from 6fe1810 to dab92dd Compare May 19, 2025 01:40

Complete mockserver tests

4334dd4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x-goog-spanner-request-id): implement Request-ID #1264

feat(x-goog-spanner-request-id): implement Request-ID #1264

odeke-em commented Dec 11, 2024

conventional-commit-lint-gcf bot commented Dec 11, 2024 •

edited

Loading

olavloite Dec 19, 2024

odeke-em Dec 24, 2024

olavloite Dec 24, 2024

odeke-em Dec 24, 2024

odeke-em Dec 24, 2024

olavloite Dec 24, 2024

olavloite Dec 24, 2024

olavloite Dec 24, 2024

olavloite Mar 25, 2025

olavloite Mar 25, 2025

olavloite Mar 25, 2025

olavloite Mar 25, 2025

olavloite Mar 25, 2025

olavloite Mar 25, 2025

olavloite Mar 25, 2025

		if not isinstance(nth_request, int):
		raise Exception(f"failed to get an integer back: {nth_request}")

feat(x-goog-spanner-request-id): implement Request-ID #1264

Are you sure you want to change the base?

feat(x-goog-spanner-request-id): implement Request-ID #1264

Conversation

odeke-em commented Dec 11, 2024

conventional-commit-lint-gcf bot commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conventional-commit-lint-gcf bot commented Dec 11, 2024 •

edited

Loading