Skip to content
This repository was archived by the owner on Mar 26, 2026. It is now read-only.

feat: Client side metrics support for mutateRows #1638

Merged
danieljbruce merged 37 commits into from
Jul 28, 2025
Merged

feat: Client side metrics support for mutateRows #1638

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
fe5b621
Add metrics collector hooks in the right places
danieljbruce Jul 9, 2025
f74cee8
Move readrows tests over
danieljbruce Jul 9, 2025
7b45ceb
Group ReadRows under separate describe block
danieljbruce Jul 9, 2025
bf9a52f
Add mutateRows tests
danieljbruce Jul 9, 2025
20167af
Add onResponse to mutateRows collection
danieljbruce Jul 9, 2025
21f96e9
Eliminate the extra mutateRows calls
danieljbruce Jul 10, 2025
689efad
Change the test frame to work without inserting
danieljbruce Jul 10, 2025
9d40bf7
Remove console traces
danieljbruce Jul 10, 2025
2bb1a59
Remove the error console log
danieljbruce Jul 10, 2025
079a607
Inserts will conflate results for readRows too
danieljbruce Jul 10, 2025
168e132
Remove only
danieljbruce Jul 10, 2025
cdbdc20
Remove only
danieljbruce Jul 10, 2025
ee12cea
Merge branch 'main' into 359913994-mutate-rows
danieljbruce Jul 10, 2025
d56ddf3
Remove the extra onResponse call
danieljbruce Jul 10, 2025
9b6ed02
Include onOperationComplete in the callback
danieljbruce Jul 14, 2025
bc70978
Remove the onOperationComplete call
danieljbruce Jul 14, 2025
76e50e7
Merge branch 'main' into 359913994-mutate-rows
danieljbruce Jul 15, 2025
5022a7a
Get rid of error code fragment
danieljbruce Jul 18, 2025
9aa6ce1
onResponse handler moved into metrics collector
danieljbruce Jul 18, 2025
04e4e77
Rename handleStatusAndMetadata
danieljbruce Jul 21, 2025
524eb47
Add comments, shorten snippet
danieljbruce Jul 21, 2025
285651f
Add the wrapRequest method to the mock
danieljbruce Jul 21, 2025
42366c9
Pass null along instead
danieljbruce Jul 21, 2025
1514d0c
Add retries comment
danieljbruce Jul 21, 2025
c1f273f
Merge branch 'main' into 359913994-mutate-rows
danieljbruce Jul 28, 2025
3c05e26
Use the same setup table code as before
danieljbruce Jul 28, 2025
e4d2c28
Rename method to setupBigtableWithInsert
danieljbruce Jul 28, 2025
33f449a
Keep setupBigtable name as setupBigtable
danieljbruce Jul 28, 2025
926c480
Eliminate unused import
danieljbruce Jul 28, 2025
a82647b
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Jul 28, 2025
4d74ab4
Merge branch '359913994-mutate-rows' of https://github.com/googleapis…
gcf-owl-bot[bot] Jul 28, 2025
a0de09b
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] Jul 28, 2025
bbea829
Merge branch '359913994-mutate-rows' of https://github.com/googleapis…
gcf-owl-bot[bot] Jul 28, 2025
9b13fba
Run the linter
danieljbruce Jul 28, 2025
f83727b
Merge branch '359913994-mutate-rows' of https://github.com/googleapis…
danieljbruce Jul 28, 2025
1594239
Make sure the table is set up properly, but don’t
danieljbruce Jul 28, 2025
eb3fefe
Remove only
danieljbruce Jul 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/tabular-api-surface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,12 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
[],
);

const metricsCollector =
this.bigtable._metricsConfigManager.createOperation(
MethodName.MUTATE_ROWS,
StreamingState.STREAMING,
this,
);
/*
The following line of code sets the timeout if it was provided while
creating the client. This will be used to determine if the client should
Expand Down Expand Up @@ -388,6 +394,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
// Return if the error happened before a request was made
if (numRequestsMade === 0) {
callback(err);
metricsCollector.onOperationComplete(err ? err.code : 0);
return;
}

Expand All @@ -399,9 +406,11 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
options.gaxOptions?.retry?.backoffSettings ||
DEFAULT_BACKOFF_SETTINGS;
const nextDelay = getNextDelay(numRequestsMade, backOffSettings);
metricsCollector.onAttemptComplete(err ? err.code : 0);
Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should know that err exists at this point, right? It seems like error code 0 should never happen

If we need a null check, I'd say we should put it beside isRetryable(err). But I assume that's already part of isRetryable

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should know that err exists at this point, right?

No. In fact if the error doesn't exist then most of the time this if block will be entered because isRetryable will be true. See the comment:

// If the error is empty but there are still outstanding mutations,
// it means that there are retryable errors in the mutate response
// even when the RPC succeeded
return !err || RETRYABLE_STATUS_CODES.has(err.code);
image

Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I know this is out of scope of this PR, but some comments here would be very useful. A docstring for isRetryable, and/or a comment at the start of this block describing what makes it retryable would help a lot

Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more important question though is: Do we record the attempt as successful if some mutations failed? or do we pass through the error code of a failed mutation?

In Python, it looks like I was attaching the status code of the exception at the top of the error list. I can't remember if that came from the product team or not though. Have you discussed this with Mattie? Or looked at Java? We should verify this before moving forward

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gemini says we record OK for PartialFailureError results in java. I'll verify that in the code next and ask Bigtable, but I am not too familiar with that codebase so may take time.

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we send back the OK code here so for the time being I'll change this to pass in code 0.

@mutianf In case of a PartialFailureError, what error code do we want to record for client side metrics on a completed attempt?

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And just to provide more background, I verified that stream ending (no error) corresponds to the case where we would expect a PartialFailureError this test. So in this case, if we are inside this if block and the err is null then we can conclude that we are retrying because some of the indicies are pending.

Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussing with Mattie, she said Java records a status of success when a mutation fails, but the rpc succeeds. So we can do the same here

setTimeout(makeNextBatchRequest, nextDelay);
return;
}
metricsCollector.onOperationComplete(err ? err.code : 0);
Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the operation complete here?

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in this case the error is not retryable so we expect the operation to end. ie. the setTimeout call does the request again.


// If there's no more pending mutations, set the error
// to null
Expand Down Expand Up @@ -431,7 +440,9 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
callback(err);
};

metricsCollector.onOperationStart();
const makeNextBatchRequest = () => {
metricsCollector.onAttemptStart();
Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some ideas:

  1. We could make the first call to onAttemptStart automatically call onOperationStart right away.
  2. I like the idea of wrapping the callbacks so that it calls onOperationComplete just before we call the callback. I think that would clean things up.
  3. 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.
  4. All calls flow through the request method. We could consider allowing request to 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.

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only ended up adopting change #2. I think #1 has the downside that we lose flexibility and we only save 1 line of code.

Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, those changes look a lot better. I think there are still improvements to be had, but this is a great start

const entryBatch = entries.filter((entry: Entry, index: number) => {
return pendingEntryIndices.has(index);
});
Expand Down Expand Up @@ -469,14 +480,16 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
options.gaxOptions,
);

this.bigtable
.request<google.bigtable.v2.MutateRowsResponse>({
const requestStream =
this.bigtable.request<google.bigtable.v2.MutateRowsResponse>({
client: 'BigtableClient',
method: 'mutateRows',
reqOpts,
gaxOpts: options.gaxOptions,
retryOpts,
})
});
metricsCollector.handleStatusAndMetadata(requestStream);
Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice if this returned the stream, so we could use it as more of a wrapper that could be applied at stream creation time:

const requestStream = metricsCollector.handleStatusAndMetadata<google.bigtable.v2.MutateRowsResponse>(
    this.bigtable.request({
          client: 'BigtableClient',
          method: 'mutateRows',
          reqOpts,
          gaxOpts: options.gaxOptions,
          retryOpts,
    })
)

Then we could maybe rename handleStatusAndMetadata to requestWrapper or something?

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I don't see much of a downside for doing that.

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I tried this and it blows up all the tests. It's probably due to bad mocks or something. I'm worried this change is going to blow up the size of this PR. Can I take this as a backlog item?

image

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backlogged here

Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really do want to make sure we are using wrappers as much as we can here, because a bunch of one-off lines for metric collection can get ugly fast

I'm not going to push too hard on this specific instance, since the one extra line doesn't add too much. But I really don't want brittle tests to stop us from merging code improvements, so keep that in mind so we can avoid this kind of problem in the future

Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we backlog: can you share the diff that was giving you trouble? That error message seems surprising, so I just want to make sure we're on the same page

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try 359913994-mutate-rows-with-diff branch. f4c7162 is the commit that contains the change for mutateRows. You will be able to see the errors if you run this branch

Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should rename handleStatusAndMetadata to something like wrapRequest, since it's also doing onResponse now?

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is wrapRequest now.

requestStream
.on('error', (err: ServiceError) => {
onBatchResponse(err);
})
Expand All @@ -499,6 +512,7 @@ Please use the format 'prezzy' or '${instance.name}/tables/prezzy'.`);
(errorDetails as any).entry = originalEntry;
mutationErrorsByEntryIndex.set(originalEntriesIndex, errorDetails);
});
metricsCollector.onResponse();
Copy link
Copy Markdown

@daniel-sanche daniel-sanche Jul 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't this be handled by handleStatusAndMetadata? Attach a separate data handler for onResponse?

Copy link
Copy Markdown
Contributor Author

@danieljbruce danieljbruce Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Note that this change has to be applied to readRows as well so you'll see the change there too.

})
.on('end', onBatchResponse);
numRequestsMade++;
Expand Down
Loading
Loading