fix: firstResponseLatencies should only be collected for readRows calls (#1658)

danieljbruce · web-flow · commit 99cf5a601024 · 2025-08-05T14:58:31.000Z
## Description This fix ensures that first response latencies are only collected for ReadRows calls and not for the other grpc methods. This matches the requirements described in https://cloud.google.com/bigtable/docs/client-side-metrics-descriptions#first-response-latencies. ## Impact Ensures that the right client side metrics are getting collected for the right methods. ## Testing - More tolerance has been added to application latencies - We change the test to expect first response latencies for readRows calls and not expect them otherwise
diff --git a/src/client-side-metrics/gcp-metrics-handler.ts b/src/client-side-metrics/gcp-metrics-handler.ts
@@ -31,6 +31,7 @@ const {
 } = require('@opentelemetry/sdk-metrics');
 import * as os from 'os';
 import * as crypto from 'crypto';
+import {MethodName} from './client-side-metrics-attributes';
 
 /**
  * Generates a unique client identifier string.
@@ -251,10 +252,16 @@ export class GCPMetricsHandler implements IMetricsHandler {
       status: data.status,
       ...commonAttributes,
     });
-    otelInstruments.firstResponseLatencies.record(data.firstResponseLatency, {
-      status: data.status,
-      ...commonAttributes,
-    });
+    if (
+      data.metricsCollectorData.method === MethodName.READ_ROWS ||
+      data.metricsCollectorData.method === MethodName.READ_ROW
+    ) {
+      otelInstruments.firstResponseLatencies.record(data.firstResponseLatency, {
+        status: data.status,
+        ...commonAttributes,
+      });
+    }
+
     if (data.applicationLatency) {
       otelInstruments.applicationBlockingLatencies.record(
         data.applicationLatency,
diff --git a/src/client-side-metrics/operation-metrics-collector.ts b/src/client-side-metrics/operation-metrics-collector.ts
@@ -181,10 +181,7 @@ export class OperationMetricsCollector {
         }) => {
           this.onStatusMetadataReceived(status);
         },
-      )
-      .on('data', () => {
-        this.onResponse();
-      });
+      );
   }
 
   /**
@@ -310,7 +307,7 @@ export class OperationMetricsCollector {
                 client_name: `nodejs-bigtable/${version}`,
                 operationLatency: totalMilliseconds,
                 retryCount: this.attemptCount - 1,
-                firstResponseLatency: this.firstResponseLatency ?? undefined,
+                firstResponseLatency: this.firstResponseLatency ?? 0,
                 applicationLatency: applicationLatency ?? 0,
               });
             }
diff --git a/src/utils/createReadStreamInternal.ts b/src/utils/createReadStreamInternal.ts
@@ -320,6 +320,10 @@ export function createReadStreamInternal(
       gaxOpts,
       retryOpts,
     });
+    requestStream.on('data', () => {
+      // This handler is necessary for recording firstResponseLatencies.
+      metricsCollector.onResponse();
+    });
 
     activeRequestStream = requestStream!;
 
diff --git a/system-test/client-side-metrics-all-methods.ts b/system-test/client-side-metrics-all-methods.ts
@@ -37,6 +37,7 @@ import {
 import {ClientOptions} from 'google-gax';
 import {ClientSideMetricsConfigManager} from '../src/client-side-metrics/metrics-config-manager';
 import {MetricServiceClient} from '@google-cloud/monitoring';
+import {MethodName} from '../src/client-side-metrics/client-side-metrics-attributes';
 
 const SECOND_PROJECT_ID = 'cfdb-sdk-node-tests';
 
@@ -66,6 +67,25 @@ function getHandlerFromExporter(Exporter: typeof CloudMonitoringExporter) {
   }).GCPMetricsHandler;
 }
 
+function checkFirstResponseLatency(requestHandled: OnOperationCompleteData) {
+  assert(
+    Object.prototype.hasOwnProperty.call(
+      requestHandled,
+      'firstResponseLatency',
+    ),
+  );
+  if (
+    requestHandled.metricsCollectorData.method === MethodName.READ_ROWS ||
+    requestHandled.metricsCollectorData.method === MethodName.READ_ROW
+  ) {
+    assert(requestHandled.firstResponseLatency);
+    assert(requestHandled.firstResponseLatency > 0);
+  } else {
+    assert.strictEqual(requestHandled.firstResponseLatency, 0);
+  }
+  delete requestHandled.firstResponseLatency;
+}
+
 function readRowsAssertionCheck(
   projectId: string,
   requestsHandled: (OnOperationCompleteData | OnAttemptCompleteData)[] = [],
@@ -98,11 +118,10 @@ function readRowsAssertionCheck(
   const secondRequest = requestsHandled[1] as any;
   // We would expect these parameters to be different every time so delete
   // them from the comparison after checking they exist.
+  checkFirstResponseLatency(secondRequest);
   assert(secondRequest.operationLatency);
-  assert(secondRequest.firstResponseLatency);
-  assert.strictEqual(secondRequest.applicationLatency, 0);
   delete secondRequest.operationLatency;
-  delete secondRequest.firstResponseLatency;
+  assert(secondRequest.applicationLatency < 10);
   delete secondRequest.applicationLatency;
   delete secondRequest.metricsCollectorData.appProfileId;
   assert.deepStrictEqual(secondRequest, {
@@ -144,11 +163,10 @@ function readRowsAssertionCheck(
   const fourthRequest = requestsHandled[3] as any;
   // We would expect these parameters to be different every time so delete
   // them from the comparison after checking they exist.
+  checkFirstResponseLatency(fourthRequest);
   assert(fourthRequest.operationLatency);
-  assert(fourthRequest.firstResponseLatency);
-  assert.strictEqual(fourthRequest.applicationLatency, 0);
+  assert(fourthRequest.applicationLatency < 10);
   delete fourthRequest.operationLatency;
-  delete fourthRequest.firstResponseLatency;
   delete fourthRequest.applicationLatency;
   delete fourthRequest.metricsCollectorData.appProfileId;
   assert.deepStrictEqual(fourthRequest, {
