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]>

olavloite · 2024-12-19T15:01:10Z

google/cloud/spanner_v1/database.py

+                channel_id = len(self.__transports_to_channel_id) + 1
+                self.__transports_to_channel_id[api._transport] = channel_id
+
+            return channel_id


I think that we can safely cache this value once it has been calculated once. self.spanner_api is never going to change after it has been created, and the same is also true for api._transport.

It is indeed cached as it is saved in the Database static __transports_to_channel_id map or would you prefer I name it with capital letters so that it is much easier to spot as __TRANSPORTS_TO_CHANNEL_ID?

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.

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

google/cloud/spanner_v1/instance.py

@@ -501,6 +501,7 @@ def database(
                proto_descriptors=proto_descriptors,
            )
        else:
+            print("enabled interceptors")


nit: remove?

olavloite · 2024-12-24T11:30:43Z

google/cloud/spanner_v1/pool.py

@@ -243,6 +243,7 @@ def bind(self, database):
            "CloudSpanner.FixedPool.BatchCreateSessions",
            observability_options=observability_options,
        ) as span:
+            attempt = 1


nit: I don't think we need this. We can just pass in 1 as a constant in the function below just as for the other functions.

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

tests/unit/test_request_id_header.py

+            if n_finished == len(threads):
+                break
+
+            time.sleep(1)


nit: can we remove this? The call to thread.join() should ensure that the threads have finished.

olavloite · 2024-12-24T11:50:41Z

tests/unit/test_request_id_header.py

+                (1, REQ_RAND_PROCESS_ID, client_id, channel_id, 1, 1),
+            ),
+            (
+                "/google.spanner.v1.Spanner/GetSession",


Note: This PR should get rid of all of these GetSession calls, so rebasing on main my cause this test to fail.

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

…propagation Generates a request_id that is then injected inside metadata that's sent over to the Cloud Spanner backend. Officially inject the first set of x-goog-spanner-request-id values into header metadata Add request-id interceptor to use in asserting tests Wrap Snapshot methods with x-goog-request-id metadata injector Setup scaffolding for XGoogRequestIdHeader checks Wire up XGoogSpannerRequestIdInterceptor for TestDatabase checks Inject header in more Session using spots plus more tests Base for tests with retries on abort More plumbing for Transaction and Database Update unit tests for Transaction Wrap more in Transaction + update tests Update tests Plumb in more tests Update TestDatabase Fixes googleapis#1261

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 added 4 commits December 24, 2024 11:04

More plumbing for Database DDL methods

0478a40

Update test_spannner.test_transaction tests

d36647c

Update test_session tests

7fc3f3f

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

odeke-em added 2 commits December 24, 2024 17:55

Update tests

8c7a8bb

Update propagation of changes

760d6cd

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 added 3 commits December 25, 2024 04:15

Plumb test_context_manager

27e8fc7

Fix more tests

3e47760

More test plumbing

70eeaef

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

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 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 Dec 24, 2024

olavloite Dec 24, 2024

olavloite Dec 24, 2024

olavloite Dec 24, 2024

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

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

edited

Loading