fix: firstResponseLatencies should only be collected for readRows calls#1658
Conversation
| firstResponseLatency: | ||
| this.firstResponseLatency ?? undefined, | ||
| } | ||
| : {}; |
There was a problem hiding this comment.
I think this check would be better here, in the handler.
If we put the validation here, we now have three meaningful states to keep track of for firstRepsonseLatency: float, undefined, or unset. And that opens us up to a bunch of weird edge cases if the type doesn't match what we expect when it comes time to record the metric.
All that type complexity goes away if we only record the metric for readRows, and ignore the field altogether for other rpcs
There was a problem hiding this comment.
This is a bit of an aside, but why do we use undefined here instead of null? It seems a bit confusing to me that this.firstResponseLatency is float or null, but data.firstResponseLatency is float or undefined. We wouldn't need that extra ?? operator if they used the same representation for an unset value
There was a problem hiding this comment.
I adopted your suggestion. I think it simplifies the logic quite a bit and I didn't like the complex expression we had before.
why do we use undefined here instead of null?
undefined is more idiomatic in typescript interfaces because you can just add ? in front of the property. In this case I used null in the metrics collector for unset firstResponseLatencies just to be deliberate about setting its value. If you don't like this undefined/null inconsistency (which I think is fixed now by the this.firstResponseLatency ?? 0 expression) then I would suggest changing the null setting in the metrics collector to undefined though that has the drawback of being less explicit.
There was a problem hiding this comment.
It might be worth considering if using consistent null/undefined representations through would make the code cleaner, but I think it's not such a big deal after this change
There was a problem hiding this comment.
Backlogged in https://b.corp.google.com/issues/435657679
| retryOpts, | ||
| }); | ||
| requestStream.on('data', () => { | ||
| metricsCollector.onResponse(); |
There was a problem hiding this comment.
maybe add a comment that this is for firstResponseLatency?
(This does make me wonder if we can remove this handler after it is called once for performance reasons, to avoid all those extra method calls when we only want the first one. ReadRow's on('data') is the most performance-sensitive part of the library. But maybe that's out of scope here)
There was a problem hiding this comment.
I backlogged the suggestion of detaching the handler at https://b.corp.google.com/issues/435417385. I don't want it to be blocking readModifyWriteRow.
There was a problem hiding this comment.
Added a comment
|
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
🤖 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 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