feat: Record ReadRows application latencies for client side metrics#1647
Conversation
be customized
Removes the onRowReachesUser method and instead uses the userStream to record application latency. This simplifies the API and makes the metric more accurate. The following files were modified: - src/client-side-metrics/gcp-metrics-handler.ts - src/client-side-metrics/metrics-handler.ts - src/client-side-metrics/operation-metrics-collector.ts - test/metrics-collector/metrics-collector.ts - test/metrics-collector/typical-method-call.txt - test-common/expected-otel-export-input.ts - test-common/metrics-handler-fixture.ts
the analytics
Property name changed
for readrows
|
|
||
| start() { | ||
| this.startTime = process.hrtime.bigint(); | ||
| this.startTime = hrtime.bigint(); |
There was a problem hiding this comment.
This more closely matches the way hrtime is used elsewhere and is easier to mock in tests.
| if (options?.transformHook) { | ||
| options?.transformHook(event, _encoding, callback); | ||
| } | ||
| callback(null, event); |
There was a problem hiding this comment.
Calling the callback is now the responsibility of the transformHook. This is because in practice the transformHook uses the callback in different ways.
| callback(null, row); | ||
| }, | ||
| }); | ||
| metricsCollector.attachUserStream(userStream); |
There was a problem hiding this comment.
This attachUserStream method is added to the metrics collector so that the metrics collector can read application latencies from the user stream. There are alternatives though:
- One alternative is to pass the userStream into the constructor of the metricsCollector which may be cleaner and eliminate a line of code. The only thing is that the metrics collector is created before createReadStreamInternal and passed in so this requires slight architecture change. We could pass the arguments for the metrics collector into this method instead of passing in the metrics collector itself and create the metrics collector in this method passing in the user stream to its constructor.
- We could pass application latencies directly into the onOperationComplete method of the metrics collector.
…github.com/googleapis/nodejs-bigtable into 359913994-application-latencies-readrows-3
Document function
daniel-sanche
left a comment
There was a problem hiding this comment.
Looks good, just a few small comments
| private applicationLatencies: number[]; | ||
| private lastRowReceivedTime: bigint | null; | ||
| private handlers: IMetricsHandler[]; | ||
| public userStream?: ITimeTrackable; |
There was a problem hiding this comment.
Is there a benefit to using an interface here, since there's only one implementer?
There was a problem hiding this comment.
It makes test mocking easier, but I could probably make this work with TimedStream. Will do this tomorrow.
There was a problem hiding this comment.
Actually, I replaced this type with TimedStream so everything is fine now.
| onOperationComplete(finalOperationStatus: grpc.status) { | ||
| onOperationComplete( | ||
| finalOperationStatus: grpc.status, | ||
| applicationLatency: number, |
There was a problem hiding this comment.
application_latency is only needed for read_rows. It seems strange to make this a required argument for all onOperationComplete calls. Can this be optional, and default to null or something? Or make a separate method that requires both? (Whatever feels more node-idiomatic)
There was a problem hiding this comment.
Are you sure? I know First Response Latencies was just ReadRows. Were you thinking of FirstResponseLatencies?
I see from https://cloud.google.com/bigtable/docs/client-side-metrics-descriptions that it's most relevant for readrows, but we need to support all operations for this one?
There was a problem hiding this comment.
Ok. I made this argument optional.
There was a problem hiding this comment.
I guess it should be applicable for all methods that return streams, but it's not too clear to me how this would be defined for unary operations. I discussed this with Mattie, and she mentioned the backoff part, but that's out of scope for now.
Yeah, I think keeping it optional is a good idea, since there will be at least some cases where it doesn't apply
🤖 I have created a release *beep* *boop* --- ## [6.3.0](https://togithub.com/googleapis/nodejs-bigtable/compare/v6.2.0...v6.3.0) (2025-08-11) ### Features * Add client side metrics for checkAndMutateRow calls ([#1661](https://togithub.com/googleapis/nodejs-bigtable/issues/1661)) ([c258ea1](https://togithub.com/googleapis/nodejs-bigtable/commit/c258ea1b29203aad3eaaf9cfe64ddabb8c1018bf)) * Add client side metrics for readModifyWriteRow calls ([#1656](https://togithub.com/googleapis/nodejs-bigtable/issues/1656)) ([2129312](https://togithub.com/googleapis/nodejs-bigtable/commit/2129312401bf9f5b8e51b13ac576cb765de401df)) * Client side metrics support for mutateRows ([#1638](https://togithub.com/googleapis/nodejs-bigtable/issues/1638)) ([7601e4d](https://togithub.com/googleapis/nodejs-bigtable/commit/7601e4da115ff6a5da411cc857917b579c70ced7)) * Collect client side metrics for sampleRowKeys calls ([#1660](https://togithub.com/googleapis/nodejs-bigtable/issues/1660)) ([6ed98fa](https://togithub.com/googleapis/nodejs-bigtable/commit/6ed98faefe446e67f83fd5394aae30374fd3ec3a)) * For client side metrics, record metrics as MUTATE_ROW for single row mutates ([#1650](https://togithub.com/googleapis/nodejs-bigtable/issues/1650)) ([f190a8c](https://togithub.com/googleapis/nodejs-bigtable/commit/f190a8c322498ddfbe73406759a43a268c16bdc4)) * Record ReadRows application latencies for client side metrics ([#1647](https://togithub.com/googleapis/nodejs-bigtable/issues/1647)) ([8af801b](https://togithub.com/googleapis/nodejs-bigtable/commit/8af801b3ecd7ff5e30e6c8cc67bd4123bdf34ee9)) ### Bug Fixes * FirstResponseLatencies should only be collected for readRows calls ([#1658](https://togithub.com/googleapis/nodejs-bigtable/issues/1658)) ([99cf5a6](https://togithub.com/googleapis/nodejs-bigtable/commit/99cf5a6010249ed0eedd88f23b2d32cacb106c07)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Description
This PR makes it so that all readRows calls will start recording application latencies. With this change, data for application latencies will now be available on the Google Cloud dashboard.
Impact
Data should now be available for application latencies on ReadRows calls in the client side metrics dashboard.
Testing
Tests are added that mock out the hrtime module and make sure the right application latencies values are recorded.