feat: Client side metrics support for mutateRows #1638
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
product-auto-label
bot
added
size: xl
product-auto-label
bot
added
size: l
src/tabular-api-surface.ts
Outdated
| setTimeout(makeNextBatchRequest, nextDelay); | ||
| return; | ||
| } | ||
| metricsCollector.onOperationComplete(err ? err.code : 0); |
There was a problem hiding this comment.
why is the operation complete here?
Contributor
Author
There was a problem hiding this comment.
Because in this case the error is not retryable so we expect the operation to end. ie. the setTimeout call does the request again.
|
|
||
| metricsCollector.onOperationStart(); | ||
| const makeNextBatchRequest = () => { | ||
| metricsCollector.onAttemptStart(); |
There was a problem hiding this comment.
these scattered onAttemptStart and onAttemptComplete lines make me a bit nervous. Especially since we have to repeat them across rpcs. Is there a way to encapsulate these better?
Maybe wrap the stream object itself? And wrap the callbacks?
Let me know if this makes sense to you. I can try to come up with some ideas if needed
Contributor
Author
There was a problem hiding this comment.
Some ideas:
- We could make the first call to
onAttemptStartautomatically callonOperationStartright away. - I like the idea of wrapping the callbacks so that it calls
onOperationCompletejust before we call the callback. I think that would clean things up. - We could consider implicitly calling onAttemptComplete at the start of onAttemptStart to implicitly end the last attempt, but then I think the attempt latency would be off because it would include the retry delay so maybe this is not such a great option.
- All calls flow through the
requestmethod. We could consider allowingrequestto accept hooks that get called when an attempt starts or when an attempt ends, but I'm not sure I like this idea. It makes this method even more complex and this is a public facing method. Even though users shouldn't really be using this method directly, it would still change a public API surface. At most I would consider taking this as a backlog item and doing it later.
Contributor
Author
There was a problem hiding this comment.
Thanks, those changes look a lot better. I think there are still improvements to be had, but this is a great start
product-auto-label
bot
added
size: xl
danieljbruce
added
the
kokoro:force-run
danieljbruce
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
daniel-sanche
left a comment
daniel-sanche
left a comment
There was a problem hiding this comment.
My only remaining concern is this thread, where we need to confirm which error code to log for successful rpcs with failed mutations
danieljbruce
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
…/nodejs-bigtable into 359913994-mutate-rows
…/nodejs-bigtable into 359913994-mutate-rows
…/nodejs-bigtable into 359913994-mutate-rows
add a mutateRow call for the handlers tests
danieljbruce
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
gcf-merge-on-green bot
pushed a commit
that referenced
this pull request
Aug 11, 2025
🤖 I have created a release *beep* *boop* --- ## [6.3.0](https://github.com/googleapis/nodejs-bigtable/compare/v6.2.0...v6.3.0) (2025-08-11) ### Features * Add client side metrics for checkAndMutateRow calls ([#1661](https://github.com/googleapis/nodejs-bigtable/issues/1661)) ([c258ea1](https://github.com/googleapis/nodejs-bigtable/commit/c258ea1b29203aad3eaaf9cfe64ddabb8c1018bf)) * Add client side metrics for readModifyWriteRow calls ([#1656](https://github.com/googleapis/nodejs-bigtable/issues/1656)) ([2129312](https://github.com/googleapis/nodejs-bigtable/commit/2129312401bf9f5b8e51b13ac576cb765de401df)) * Client side metrics support for mutateRows ([#1638](https://github.com/googleapis/nodejs-bigtable/issues/1638)) ([7601e4d](https://github.com/googleapis/nodejs-bigtable/commit/7601e4da115ff6a5da411cc857917b579c70ced7)) * Collect client side metrics for sampleRowKeys calls ([#1660](https://github.com/googleapis/nodejs-bigtable/issues/1660)) ([6ed98fa](https://github.com/googleapis/nodejs-bigtable/commit/6ed98faefe446e67f83fd5394aae30374fd3ec3a)) * For client side metrics, record metrics as MUTATE_ROW for single row mutates ([#1650](https://github.com/googleapis/nodejs-bigtable/issues/1650)) ([f190a8c](https://github.com/googleapis/nodejs-bigtable/commit/f190a8c322498ddfbe73406759a43a268c16bdc4)) * Record ReadRows application latencies for client side metrics ([#1647](https://github.com/googleapis/nodejs-bigtable/issues/1647)) ([8af801b](https://github.com/googleapis/nodejs-bigtable/commit/8af801b3ecd7ff5e30e6c8cc67bd4123bdf34ee9)) ### Bug Fixes * FirstResponseLatencies should only be collected for readRows calls ([#1658](https://github.com/googleapis/nodejs-bigtable/issues/1658)) ([99cf5a6](https://github.com/googleapis/nodejs-bigtable/commit/99cf5a6010249ed0eedd88f23b2d32cacb106c07)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Much like this change, this PR adds support for collecting client side metrics for mutateRows calls. This is a method that we want to collect client side metrics for.
Impact
This allows us to look at client side metrics for all mutateRows calls
Testing
All the tests that apply to readRows now also apply to mutateRows tests. This adds testing to ensure the metrics handlers get the right data, that the test doesn't time out and that the call to export the metrics succeeds.
Just a note that the readRows tests were copy pasted to a file that contains both the readrows tests and mutate rows tests because there have been small refactors and I didn't want to make a diff that was difficult to read. So we can delete the old file if necessary.