feat(Spanner.V1): Add attempt metrics for streams - #15806
robertvoinescu-work wants to merge 1 commit into
Conversation
0af58c5 to
8178bef
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
0f81d4d to
57ee54c
Compare
c9920b4 to
0db1a35
Compare
robertvoinescu-work
changed the title
c341414 to
ac638b6
Compare
robertvoinescu-work
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements built-in metrics instrumentation for server streaming calls in the Spanner client, introducing InstrumentedAsyncStreamReader to track stream duration and completion status, along with corresponding unit tests. Feedback on the changes highlights a critical resource leak where the original AsyncServerStreamingCall is not disposed, thread-safety concerns in InstrumentedAsyncStreamReader that should be resolved using Interlocked operations, and a minor typo in the test comments.
ac638b6 to
d5d12d6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements built-in metrics instrumentation for Spanner server streaming calls by introducing InstrumentedAsyncStreamReader and updating MetricsInterceptor to intercept AsyncServerStreamingCall. It also consolidates test doubles and adds comprehensive streaming tests. A critical issue was identified where disposing of an early-terminated AsyncServerStreamingCall fails to dispose of the InstrumentedAsyncStreamReader, resulting in lost attempt metrics; wrapping the dispose action to dispose of the instrumented reader first was suggested.
d5d12d6 to
11af836
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds built-in metrics instrumentation for gRPC server streaming calls in the Google Cloud Spanner V1 client, introducing InstrumentedAsyncStreamReader to track stream duration and completion status, and updating MetricsInterceptor to intercept streaming calls. It also includes comprehensive unit tests. The feedback suggests ensuring robust resource cleanup during disposal by using a try-finally block, mapping OperationCanceledException to StatusCode.Cancelled instead of StatusCode.Unknown for more accurate cancellation metrics (along with updating the tests), and fixing a minor typo in a comment.
11af836 to
3bed32f
Compare
|
|
||
| var instrumentedStreamReader = new InstrumentedAsyncStreamReader<TResponse>( | ||
| call.ResponseStream, | ||
| stopwatch, |
There was a problem hiding this comment.
So the amount of time we measure is how long it takes to read the whole stream? Are you sure that's what we are meant to do here?
| var labels = Labeler.GetLabels(context.Method.Name, dbNameProvider, status, _clientIdentity); | ||
| RecordAttemptMetrics(elapsedMs, labels); | ||
|
|
||
| // Fire and forget, unlike the unary path: this callback runs inline on the reader's MoveNext |
There was a problem hiding this comment.
MoveNext here is async. Why can't this be awaited there?
| /// <summary> | ||
| /// Invokes the close callback, unless the stream has already closed. | ||
| /// </summary> | ||
| private void TryRecordClose(StatusCode status) |
There was a problem hiding this comment.
nit: Or something of the sort? Instead of closed?
| private void TryRecordClose(StatusCode status) | |
| private void Done(StatusCode status) |
| /// <summary> | ||
| /// Records the close of the stream when the consumer abandons it before completion. | ||
| /// </summary> | ||
| public void NotifyClosed() |
There was a problem hiding this comment.
Shouldn't this be Dispose?
But also, from when a user abandons the stream to when it's Disposed of, there might be a long time. Are we sure we want to record that?
2 participants
b/404948213