feat: Add plumbing to support unary calls for client side metric collection #1631

danieljbruce · 2025-07-03T18:24:29Z

Description

This PR adds source code changes with plumbing so that client side metric collection can be done for unary calls. No client side metrics are actually collected yet for readModifyWriteRow or checkAndMutate calls, but future PRs will use this plumbing to collect metrics for those calls. A test is provided that makes a fake method on the table to demonstrate how a method would collect these metrics to serve as an example.

Impact

Doesn't change the way the client library works. Just provides support for when we open a PR that collects client side metrics for unary calls.

Testing

A unit test is created that adds a fake method to a table that does a basic unary readModifyWriteRow call. It ensures that the metrics make it to the test metrics handler correctly.

Additional Information

Changes:

src/interceptor.ts: This file contains code for attaching interceptors to a call that will call the correct metrics collector methods when data arrives

src/client-side-metrics/operation-metrics-collector.ts: The types are cleaned up in this file. None of the functionality has changed in this file

system-test/read-modify-write-row-interceptors.ts: This adds a test that ensures the plumbing for collecting metadata trailers and headers works as intended for unary calls.

This commit introduces a new system test for client-side metrics related to the ReadModifyWriteRow operation. The test (`system-test/isolated-read-modify-write-row-metrics.ts`) is designed to be self-contained: - It uses a `TestMetricsHandler` to capture metrics locally. - It simulates the gRPC call for ReadModifyWriteRow to avoid reliance on a running emulator for its core assertions regarding metrics collection. - It verifies that zone, cluster, and server timing are correctly extracted from simulated metadata and reported to the metrics handler. The previous non-isolated test file for this functionality has been removed. A minor modification was also made to `system-test/bigtable.ts` to ensure the global Bigtable client (used in suite-level before/after hooks) is initialized with a dummy project ID and emulator endpoint. This addresses potential 'project ID not found' errors when an emulator is used, although current test environment issues indicate the emulator is not connected.

Refactors the system test for ReadModifyWriteRow client-side metrics to use a mock gRPC server and custom interceptors. Key changes in `system-test/read-modify-write-row-interceptors.ts`: - A mock Bigtable gRPC service is implemented, providing a controllable `ReadModifyWriteRow` endpoint that can return specific responses, initial metadata (for server-timing), and trailing metadata (for zone/cluster via x-cbt-op-details). - A custom gRPC interceptor provider, `createMetricsInterceptorProvider`, is introduced. This interceptor is attached to the `bigtable.request` call and invokes the lifecycle methods of an `OperationMetricsCollector`. - The test now creates a `fakeReadModifyWriteRow` method on a Table instance. This method makes a `bigtable.request` call (configured with the custom interceptor) to the mock server. - Assertions verify that the `TestMetricsHandler` correctly receives metrics populated by the `OperationMetricsCollector` via the interceptor pipeline, including data derived from the mock server's metadata (zone, cluster, server timing). This approach allows for more thorough testing of the metrics collection mechanism, including the interceptor integration, in a controlled environment.

This commit refactors the ReadModifyWriteRow client metrics system test (`system-test/read-modify-write-row-interceptors.ts`) to improve how the `OperationMetricsCollector` lifecycle is managed in conjunction with the mock gRPC server and custom interceptor. Changes: - The `createMetricsInterceptorProvider` function has been simplified. The interceptor it creates is now solely responsible for relaying in-flight gRPC data (initial metadata, response messages, final status with trailing metadata) to the `OperationMetricsCollector` by calling `onMetadataReceived`, `onResponse`, and `onStatusMetadataReceived`. - The `fakeReadModifyWriteRow` method, which orchestrates the test call, now explicitly manages the `OperationMetricsCollector`'s broader lifecycle. It calls `onOperationStart` and `onAttemptStart` *before* the `bigtable.request()` call to the mock server. *After* the request promise settles (either resolves or rejects), it calls `onAttemptComplete` and `onOperationComplete` with the final status of the RPC attempt. This revised structure more accurately reflects the division of responsibilities: - The calling code (simulated by `fakeReadModifyWriteRow`) manages the overall operation and attempt lifecycle with the collector. - The gRPC interceptor observes and passes along the data specific to the RPC attempt as it flows through. This should lead to more accurate testing of the metrics collection pipeline.

This commit adds detailed console.log statements throughout the `system-test/read-modify-write-row-interceptors.ts` test file. Logging has been added to: - The `fakeReadModifyWriteRow` method to trace its execution flow, particularly around the `await bigtable.request()` call. - The `createMetricsInterceptorProvider` function and the methods of the gRPC `InterceptingCall` object it returns (e.g., `start`, `onReceiveMetadata`, `onReceiveMessage`, `onReceiveStatus`). - The `MockBigtableService.ReadModifyWriteRow` method to track when the mock server is hit and what it's attempting to send. These logs are intended to help diagnose potential issues with the execution order, such as the `await bigtable.request()` call completing prematurely or interceptor hooks not being triggered as expected. This will be useful once the test suite's environment allows the test file to be reached (i.e., global hooks requiring emulator connection can pass).

…tps://github.com/googleapis/nodejs-bigtable into feat/read-modify-write-row-metrics-isolated-test # Conflicts: # system-test/read-modify-write-row-interceptors.ts

daniel-sanche · 2025-07-10T22:54:51Z

src/interceptor.ts

+  };
+}
+
+export function withInterceptors(


Can you show me how you expect to use this? Does something like this need to be called at the start of each request?

In a method call we write something like this:

bigtable.request( { client: 'BigtableClient', method: 'readModifyWriteRow', reqOpts: { tableName: table.name, rowKey: Buffer.from(ROW_KEY), rules: [ { familyName: COLUMN_FAMILY, columnQualifier: Buffer.from(COLUMN), appendValue: Buffer.from('-wood'), }, ], appProfileId: undefined, }, gaxOpts: withInterceptors({}, metricsCollector), }, (err: ServiceError | null, resp?: any) => { if (err) { reject(err); } else { resolve(resp); } }, );

See the tests below. The interceptors are added onto gax options.

…tps://github.com/googleapis/nodejs-bigtable into feat/read-modify-write-row-metrics-isolated-test

danieljbruce · 2025-07-14T15:22:02Z

system-test/read-modify-write-row-interceptors.ts

+    );
+    assert.ok(attemptCompleteData.attemptLatency >= 0);
+    assert(attemptCompleteData.serverLatency);
+    assert.ok(attemptCompleteData.serverLatency >= 0);


This is where we prove we are getting the server latency from the metadata headers.

danieljbruce · 2025-07-14T15:22:31Z

system-test/read-modify-write-row-interceptors.ts

+    assert.strictEqual(
+      operationCompleteData.metricsCollectorData.cluster,
+      CLUSTER,
+    );


This is where we prove we are getting the Zone/Cluster from the trailer.

…test

daniel-sanche

Looking good for the most part, but I have a few small comments

daniel-sanche · 2025-07-14T20:52:46Z

src/interceptor.ts

+    }
+    if (!gaxOptions.otherArgs.options.interceptors) {
+      gaxOptions.otherArgs.options.interceptors = [interceptor];
+    }


don't you need to add the interceptor, even if other interceptors were added?

That really depends on how we want to design the system. Here are the pros and cons of adding the interceptor.

Pros:

Existing users that are already using interceptors, likely a small minority of our users, will automatically be using the new interceptor now so they don't have to add the CSM interceptor to their interceptors

Cons:

It means the user doesn't have full control of the interceptors which reduces flexibility

Yeah, I guess the pros outweigh the cons so we can add this change.

Actually, I'm not sure I like this change anymore. If the interceptors are not an array (although they should be) then trying to push another interceptor onto interceptors will throw an error. I don't want users to experience that just in case they are using interceptors incorrectly. As you can see in the screenshot, the compiler doesn't guarantee any particular structure for the interceptors property.

Just to make sure I'm understanding this right: with the current code, if the user passes in their own interceptors, they would effectively be disabling csm, right?

What else could they be passing other than an array? There must be some docstring somewhere telling us what we need to support

if the user passes in their own interceptors, they would effectively be disabling csm, right?

Yes, unless they explicitly passed in the CSM interceptor.

this says "Additional arguments to be passed to the API calls." That's not helpful enough so I'll keep looking.

All the code examples pass interceptors in as an array. If you feel strongly about adding it to the existing interceptors then we can make that change, but I just don't like how the compiler doesn't guarantee that it is an array.

Can we check the type of gaxOptions.otherArgs.options.interceptors, and only append if it is a list? And ignore unexpected input types?

I really don't think we should silently disable CSM if the user passes in an interceptor. That would present as a bug, and it could be difficult to track down

Sounds good. I made this change.

daniel-sanche · 2025-07-14T20:58:00Z

src/interceptor.ts

+          },
+          onReceiveStatus: (status, nextStat) => {
+            if (status.code === GrpcStatus.OK && savedReceiveMessage) {
+              collector.onResponse(); // Call onResponse for successful unary calls with a message


is this the right place for onResponse? Why not put this in onReceiveMessage?

Also, isn't onResponse only used by read rows? should createMetricsInterceptorProvider have a bool argument indicating whether to call onResponse?

Also, isn't onResponse only used by read rows?

This is a good point. I don't think we need the call to onResponse at all because this interceptor should only be used for unary calls. For streaming calls like readRows, if we really did want to call onResponse earlier like right when the client receives the message rather than after it passes through gax then we should have a different interceptor that doesn't do the status and metadata parse. Unless we plan on making changes to the streaming calls to change the way they do status and metadata parsing?

My suggestion is to remove the onResponse call for now since the intent is to only use these interceptors for unary calls.

Then as a backlog item we can decide:

If streaming calls should use interceptors as well. If not no changes are required

I did an experiment and confirmed that onReceiveMessage seems to be the right place to do the onResponseCall

Make the withInterceptors method accept a parameter that picks the right interceptor. ie. if ReadRows then call onResponse

Ok, yeah IIRC onResponse is only for read_rows, so probably don't need it here

I think interceptors are optional for streams. If it would make the code more clean, and let us strip out some extra logic, we should go for it. But if it's easier to grab the metadata using wrappers, that's fine too

I think this is the code which appears to always send back an OK code when there are partial failure errors?

Was that last comment meant for the other thread, where we were discussing Partial Errors?

I don't think that section of code tells us much on its own though. Where is the error being handled?

Was that last comment meant for the other thread, where we were discussing Partial Errors?

Yes I think so. I see it contains the link to the same code.

I think interceptors are optional for streams

Yes. I think so too. And even if not then this isn't a blocker. I think no further action is required here.

…test

daniel-sanche · 2025-07-18T22:21:11Z

src/interceptor.ts

+            nextMd(metadata);
+          },
+          onReceiveMessage: (message, nextMsg) => {
+            savedReceiveMessage = message; // Still need to know if a message was received for onResponse


is this variable still needed? If not, is this event onReceiveMessage handler still needed?

Good catch. This variable has been removed now.

variable

…test

daniel-sanche

I think my only remaining concern is that users passing in interceptors would disable CSM.

We need to either document that and explain how to manually pass the CSM interceptor, or make sure we append the interceptor to whatever the user passes in

daniel-sanche · 2025-07-22T23:40:00Z

src/interceptor.ts

+          },
+          onReceiveMessage: (message, nextMsg) => {
+            nextMsg(message);
+          },


is this block actually needed? If we left this off, doesn't it just call nextMesg(message) automatically?

Yes. I tested it and we can ignore this block.

daniel-sanche · 2025-07-22T23:40:47Z

src/interceptor.ts

+      },
+      sendMessage: (message, next) => next(message),
+      halfClose: next => next(),
+      cancel: next => next(),


are these unimplemented callbacks needed?

No. These aren't necessary either. If you remove them then messages still get sent etc. They were just copy/paste from the logging interceptor code.

daniel-sanche

LGTM

daniel-sanche · 2025-07-23T16:02:11Z

src/interceptor.ts

+    if (!gaxOptions.otherArgs.options.interceptors) {
+      gaxOptions.otherArgs.options.interceptors = [interceptor];
+    } else {
+      if (Array.isArray(gaxOptions.otherArgs.options.interceptors)) {


nit: this would scan better as an else if

danieljbruce and others added 5 commits July 2, 2025 17:26

Add the logging interceptor

1d648f0

Unary call interceptor experiment

ff27288

Add the method that gets the interceptor

84a006a

Fix types in the interceptor

4aa33df

product-auto-label bot added size: xl Pull request size is extra large. api: bigtable Issues related to the googleapis/nodejs-bigtable API. labels Jul 3, 2025

google-labs-jules bot and others added 23 commits July 3, 2025 19:07

More updates to the test code. Getting it working

80d1b2d

Fix the readModifyWriteRow service

b2e13ee

Mock service is being reached

1df9aa3

Delete service class

8b33974

Switch only

272ee4f

successful metadata retrieval

4905815

Fixed bug to fetch metrics

15be99e

Passing test

42b8639

Eliminate unncessary code

5550687

Eliminate lots of unecessary code

3d740c4

Move createMetricsInterceptorProvider

873c15e

Add some comments

8373ca0

Updated comment

d02d0c1

Don’t rely on another method to make this work

bb2cdcd

Add comments to themethd

a054d37

Delete unused code

59a3591

Merge branch 'feat/read-modify-write-row-metrics-isolated-test' of ht…

96ab36e

…tps://github.com/googleapis/nodejs-bigtable into feat/read-modify-write-row-metrics-isolated-test # Conflicts: # system-test/read-modify-write-row-interceptors.ts

Pull out function that attaches interceptors

e2e0f97

More documentation for the test helpers

2f513b0

Rename

eabbc0a

daniel-sanche reviewed Jul 10, 2025

View reviewed changes

danieljbruce added 2 commits July 14, 2025 09:55

Protos should match main

859b5c5

Merge branch 'feat/read-modify-write-row-metrics-isolated-test' of ht…

6a33923

…tps://github.com/googleapis/nodejs-bigtable into feat/read-modify-write-row-metrics-isolated-test

danieljbruce commented Jul 14, 2025

View reviewed changes

danieljbruce added 2 commits July 14, 2025 13:59

Revert MC changes to simplify types

5a92b25

Remove import statement

a23b5d7

danieljbruce mentioned this pull request Jul 14, 2025

Feat/read modify write row metrics isolated test style changes #1642

Draft

Merge branch 'main' into feat/read-modify-write-row-metrics-isolated-…

987d8a3

…test

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

danieljbruce requested a review from daniel-sanche July 14, 2025 19:19

daniel-sanche reviewed Jul 14, 2025

View reviewed changes

danieljbruce and others added 2 commits July 18, 2025 15:27

Merge branch 'main' into feat/read-modify-write-row-metrics-isolated-…

a3ff4f0

…test

onResponse call removed

d4a68b2

danieljbruce requested a review from daniel-sanche July 18, 2025 21:49

daniel-sanche reviewed Jul 18, 2025

View reviewed changes

danieljbruce and others added 2 commits July 22, 2025 15:48

Remove the saveReceived message for local instance

7662205

variable

Merge branch 'main' into feat/read-modify-write-row-metrics-isolated-…

a1012fc

…test

danieljbruce requested a review from daniel-sanche July 22, 2025 21:32

daniel-sanche reviewed Jul 22, 2025

View reviewed changes

danieljbruce added 2 commits July 23, 2025 10:30

Removed unnecessary code

cf08449

Always add the client side metric interceptor

2befabc

danieljbruce added the automerge Merge the pull request once unit tests and other checks pass. label Jul 23, 2025

danieljbruce requested a review from daniel-sanche July 23, 2025 15:06

daniel-sanche approved these changes Jul 23, 2025

View reviewed changes

gcf-merge-on-green bot merged commit c267ede into main Jul 23, 2025
20 of 23 checks passed

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jul 23, 2025

gcf-merge-on-green bot deleted the feat/read-modify-write-row-metrics-isolated-test branch July 23, 2025 16:04

release-please bot mentioned this pull request Jul 22, 2025

chore(main): release 6.2.0 #1627

Merged

feat: Add plumbing to support unary calls for client side metric collection #1631

feat: Add plumbing to support unary calls for client side metric collection #1631

Uh oh!

Conversation

danieljbruce commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Impact

Testing

Additional Information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieljbruce Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieljbruce Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieljbruce Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daniel-sanche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

danieljbruce commented Jul 3, 2025 •

edited

Loading

danieljbruce Jul 14, 2025 •

edited

Loading

danieljbruce Jul 18, 2025 •

edited

Loading

danieljbruce Jul 22, 2025 •

edited

Loading

daniel-sanche Jul 22, 2025 •

edited

Loading

daniel-sanche Jul 18, 2025 •

edited

Loading