feat(debugging): implement x-goog-spanner-request-id propagation per request #2205

odeke-em · 2024-12-21T14:07:48Z

Implements propagation of the x-goog-spanner-request-id that'll be propagated for every call. Once an error has been encountered, that error will have .requestId set.

Fixes #2200

odeke-em · 2024-12-27T16:15:40Z

@olavloite @surbhigarg92 kindly help me take a look at this PR, it is ready for review. Thank you!

This change brings in the bases for x-goog-spanner-request-id by commit AtomicCounter and the various utilities plus some unit tests. Ripped out of PR googleapis#2205 Updates googleapis#2200

* feat(x-goog-spanner-request-id): add bases This change brings in the bases for x-goog-spanner-request-id by commit AtomicCounter and the various utilities plus some unit tests. Ripped out of PR #2205 Updates #2200 * Fix lint * Address review comments * Address more review feedback * fix: format process-id as a 32-bit hex number * chore: fix formatting issue * fix: left-pad process-id * fix: no of elements after process-id should be 4 --------- Co-authored-by: Knut Olav Løite <[email protected]>

…request Implements propagation of the x-goog-spanner-request-id that'll be propagated for every call. Once an error has been encountered, that error will have `.requestId` set. Fixes googleapis#2200

olavloite · 2025-02-10T18:26:53Z

test/spanner.ts

@@ -170,6 +313,7 @@ describe('Spanner with mock server', () => {
      servicePath: 'localhost',
      port,
      sslCreds: grpc.credentials.createInsecure(),
+      // interceptors: [xGoogReqIDInterceptor.generateLoggingClientInterceptor()],


nit: remove?

olavloite · 2025-02-10T18:27:22Z

test/spanner.ts

@@ -2690,6 +2877,7 @@ describe('Spanner with mock server', () => {
            (e as ServiceError).message,
            'No resources available.'
          );
+          // assert.deepStrictEqual((e as RequestIDError).requestID,`1.${randIdForProcess}.1.1.1.1`);


Why is this commented?

It is because that error isn't sent via RPC but really just thrown when fail: true is set.

test/spanner.ts

olavloite · 2025-02-10T18:27:39Z

test/spanner.ts

@@ -2825,6 +3014,7 @@ describe('Spanner with mock server', () => {
          (err as ServiceError).code,
          Status.PERMISSION_DENIED
        );
+        // assert.deepStrictEqual((err as RequestIDError).requestID,`1.${randIdForProcess}.1.1.1.1`);


olavloite · 2025-02-10T18:28:13Z

src/request_id_header.ts

@@ -88,6 +88,8 @@ function newAtomicCounter(n?: number): AtomicCounter {
  return new AtomicCounter(n);
 }

+const X_GOOG_REQ_ID_REGEX = /(\d+\.){5}\d+/;


Is this used anywhere? (If it is, then that is probably an indication of a small bug, as it assumes that the process id consists of only digits)

olavloite · 2025-02-10T18:29:38Z

src/index.ts

+        // TODO: Negotiate with the Google team to plumb gRPC
+        // settings such as interceptors to the gRPC client.
+        // 'grpc.interceptors': interceptors,


What is the status of this?

No action at all sadly, I reported to the maintainers of this library but out of scope currently and not a priority for them.

olavloite · 2025-02-10T18:31:38Z

src/session.ts

+          this.commonHeaders_,
+          this,
+          nextNthRequest(database),
+          1


I don't think attempt is ever set to any value higher than 1. There are also tests for retried RPCs.

odeke-em · 2025-02-11T16:23:33Z

src/database.ts

+    };
+    withReqId[X_GOOG_SPANNER_REQUEST_ID_HEADER] = craftRequestId(
+      this._clientId || 1,
+      1, // TODO: Properly infer the channelId


@olavloite I filed a TODO for retrieving the channelId which I believe can be done much later in a phased series of PRs instead of at once.

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

product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/nodejs-spanner API. labels Dec 21, 2024

odeke-em force-pushed the x-goog-spanner-request-id branch 3 times, most recently from 1f5c031 to 85f6d7a Compare December 21, 2024 16:13

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

odeke-em force-pushed the x-goog-spanner-request-id branch 2 times, most recently from 323edc3 to fc4a2de Compare December 22, 2024 12:40

odeke-em mentioned this pull request Dec 22, 2024

SpannerOptions: document how to pass in client gRPC options like client interceptors #2206

Open

6 tasks

odeke-em force-pushed the x-goog-spanner-request-id branch 2 times, most recently from 210bbc1 to b6e7308 Compare December 24, 2024 19:05

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

odeke-em force-pushed the x-goog-spanner-request-id branch 2 times, most recently from c27ba41 to 12efa68 Compare December 24, 2024 19:08

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

odeke-em force-pushed the x-goog-spanner-request-id branch from 12efa68 to e823c42 Compare December 27, 2024 15:34

odeke-em force-pushed the x-goog-spanner-request-id branch from 54d1188 to 918f802 Compare January 6, 2025 01:18

odeke-em mentioned this pull request Jan 7, 2025

feat(x-goog-spanner-request-id): add bases #2211

Merged

odeke-em force-pushed the x-goog-spanner-request-id branch from b8630f5 to 253d879 Compare February 9, 2025 22:34

odeke-em requested a review from a team as a code owner February 9, 2025 22:34

feat(debugging): implement x-goog-spanner-request-id propagation per …

e683e25

…request Implements propagation of the x-goog-spanner-request-id that'll be propagated for every call. Once an error has been encountered, that error will have `.requestId` set. Fixes googleapis#2200

odeke-em force-pushed the x-goog-spanner-request-id branch from 253d879 to e683e25 Compare February 9, 2025 22:44

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

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

olavloite reviewed Feb 10, 2025

View reviewed changes

All: update tests

ebab047

odeke-em force-pushed the x-goog-spanner-request-id branch from 88c617d to ebab047 Compare February 11, 2025 16:20

odeke-em commented Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(debugging): implement x-goog-spanner-request-id propagation per request #2205

feat(debugging): implement x-goog-spanner-request-id propagation per request #2205

odeke-em commented Dec 21, 2024

odeke-em commented Dec 27, 2024

olavloite Feb 10, 2025

olavloite Feb 10, 2025

odeke-em Feb 11, 2025

olavloite Feb 10, 2025

olavloite Feb 10, 2025

olavloite Feb 10, 2025

odeke-em Feb 11, 2025

olavloite Feb 10, 2025

odeke-em Feb 11, 2025

feat(debugging): implement x-goog-spanner-request-id propagation per request #2205

Are you sure you want to change the base?

feat(debugging): implement x-goog-spanner-request-id propagation per request #2205

Conversation

odeke-em commented Dec 21, 2024

odeke-em commented Dec 27, 2024

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