First response latencies for readRows only

Author: danieljbruce

src/client-side-metrics/operation-metrics-collector.ts

@@ -181,10 +181,7 @@ export class OperationMetricsCollector {
         }) => {
           this.onStatusMetadataReceived(status);
         },
-      )
-      .on('data', () => {
-        this.onResponse();
-      });
+      );
   }
 
   /**
@@ -303,14 +300,23 @@ export class OperationMetricsCollector {
         {
           this.handlers.forEach(metricsHandler => {
             if (metricsHandler.onOperationComplete) {
+              const firstResponseLatencyExpression =
+                this.methodName === MethodName.READ_ROWS ||
+                this.methodName === MethodName.READ_ROW
+                  ? {
+                      firstResponseLatency:
+                        this.firstResponseLatency ?? undefined,
+                    }
+                  : {};
               metricsHandler.onOperationComplete({
                 status: finalOperationStatus.toString(),
                 streaming: this.streamingOperation,
                 metricsCollectorData: this.getMetricsCollectorData(),
                 client_name: `nodejs-bigtable/${version}`,
                 operationLatency: totalMilliseconds,
                 retryCount: this.attemptCount - 1,
-                firstResponseLatency: this.firstResponseLatency ?? undefined,
+                // Conditionally add the firstResponseLatency property
+                ...firstResponseLatencyExpression,
                 applicationLatency: applicationLatency ?? 0,
               });
             }

src/utils/createReadStreamInternal.ts

@@ -324,6 +324,9 @@ export function createReadStreamInternal(
       gaxOpts,
       retryOpts,
     });
+    requestStream.on('data', () => {
+      metricsCollector.onResponse();
+    });
 
     activeRequestStream = requestStream!;
 

system-test/client-side-metrics-all-methods.ts

@@ -98,11 +98,15 @@ 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.
+  if (method === 'Bigtable.ReadRows' || method === 'Bigtable.ReadRow') {
+    assert(secondRequest.firstResponseLatency);
+    delete secondRequest.firstResponseLatency;
+  } else {
+    assert(!secondRequest.firstResponseLatency);
+  }
   assert(secondRequest.operationLatency);
-  assert(secondRequest.firstResponseLatency);
-  assert.strictEqual(secondRequest.applicationLatency, 0);
+  assert(secondRequest.applicationLatency < 10);
   delete secondRequest.operationLatency;
-  delete secondRequest.firstResponseLatency;
   delete secondRequest.applicationLatency;
   delete secondRequest.metricsCollectorData.appProfileId;
   assert.deepStrictEqual(secondRequest, {
@@ -144,11 +148,15 @@ 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.
+  if (method === 'Bigtable.ReadRows' || method === 'Bigtable.ReadRow') {
+    assert(fourthRequest.firstResponseLatency);
+    delete fourthRequest.firstResponseLatency;
+  } else {
+    assert(!fourthRequest.firstResponseLatency);
+  }
   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, {
@@ -260,7 +268,7 @@ async function checkForPublishedMetrics(projectId: string) {
   }
 }
 
-describe('Bigtable/ClientSideMetrics', () => {
+describe.only('Bigtable/ClientSideMetrics', () => {
   const instanceId1 = 'emulator-test-instance';
   const instanceId2 = 'emulator-test-instance2';
   const tableId1 = 'my-table';