feat(Spanner.V1): Add interceptor for Built-In Metrics by robertvoinescu-work · Pull Request #15766 · googleapis/google-cloud-dotnet

robertvoinescu-work · 2026-07-16T17:31:49Z

b/404948213

gemini-code-assist

Code Review

This pull request introduces a gRPC interceptor to record attempt-level metrics for Spanner, including attempt counts, latencies, and GFE latencies extracted from the 'server-timing' header. The implementation integrates this interceptor into the Spanner client builder. My review identified three issues: an unused field that should be removed, an incorrect usage of context.Method.Name instead of context.Method.FullName when extracting method names, and a premature return in the GFE latency parsing logic that prevents processing multiple entries in the 'server-timing' header.

efevans

Looks good and very clean. Now that we have the Interceptor class implementation, is this going too far including metrics, instruments, labels, and methods that are generic enough to extract into a shared library? What if we changed the underlying SpannerBuiltInMetrics static class to an abstract class that extracts the shared resources, and then each API library can extend from that and add service-specific metrics and labels? Then each library would expose an instance of their implementation as a singleton, and pass that to the Interceptor constructor to call for library-specific instrumentation.

Quick peek at the metrics and labels in SpannerBuiltInMetrics and I'm guessing database is the lone Spanner specific label, and other labels like client_name being shared with implementation-specific values that could be obtained through overriding virtual GetClientName methods.

Looking to start a discussion on this before asking for this done since it's a pretty big ask.

efevans

LGTM

amanda-tarafa · 2026-08-03T23:18:09Z

+                // No headers are available if continuation fails, but we can record status (if RpcException) and latency.
+                // An RpcException here is unexpected but defensively handled in case a previous interceptor throws it.
+                string status = ex is RpcException rpcEx ? rpcEx.StatusCode.ToString() : s_statusUnknown;
+                _ = RecordAttemptMetricsAsync(headersTask: null,


Add a sync version of this method that receives no header tasks at all? You can use that one for the blocking unary call as well.

But honestly I wonder if we need to instrument here, where the call didn't even reached the service yet? Do we know what other languages did?

Done. At least in Java this seems to be the case, any failures due to faulty interceptors are caught by the exact same logic handling RPC network exceptions. I will ask spanner team for clarification if this was intentional. Leaving this open for now.

I spoke with spanner team and they said we should "record all the RPCs which are initiated [client side] and has not completed for whatever reason."

amanda-tarafa · 2026-08-04T03:43:45Z

+                    {
+                        if (string.Equals(header.Key, ServerTimingHeader, StringComparison.OrdinalIgnoreCase))
+                        {
+                            EmitServerTimingMetrics(header.Value, GfeMetricPrefix, duration => s_gfeLatency.Record(duration, labels));


We don't need to pass these as a parameters. If we ever parse more headers, then we can add parameters.

Suggested change

EmitServerTimingMetrics(header.Value, GfeMetricPrefix, duration => s_gfeLatency.Record(duration, labels));

EmitServerTimingMetrics(header.Value);

I spoke with Surbhi and got some clarification that we will regardless receive AFE headers - direct path enabled or not. We will need to add some additional headers to get this x-goog-spanner-enable-afe-server-timing. In the meantime I've left the code so it's easily extensible for when that is added along with a TODO.

robertvoinescu-work · 2026-08-21T17:48:05Z

+        private readonly IStopwatchProvider _stopwatchProvider = stopwatchProvider ?? DefaultStopwatchProvider.Instance;
+
+        /// <inheritdoc/>
+        public override AsyncUnaryCall<TResponse> AsyncUnaryCall<TRequest, TResponse>(


@amanda-tarafa I ran some integration tests and we are still hitting this interceptor even with the request id being populated through a CallOptions header mutation. I might have missed something when we originally discussed so let me know if theres a different situation where you expect blocking unary calls not to occur.

Yes, this one will always be used at least for async calls. It's the blocking one that may be unused if the caller added response header handlers. That's because the gRPC blocking client method does not expose response headers, but our client does allow to set response header handlers for both async and blocking. See the code that does that here.

I pinned this to the wrong method, I meant to pin it to the blocking unary. I ran some integration tests and found that blocking unary was still called even when we have the RequestId population as a callback (the current state of things).

But I misunderstood what you were saying originally - it's the response handler that causes the skip as you mentioned - and I just verified that.

amanda-tarafa

Mostly nits, a single thing that we should change immediately. The latency in blocking I think it's being miscalculated on errors.

amanda-tarafa · 2026-09-01T21:13:26Z

+        private readonly IStopwatchProvider _stopwatchProvider = stopwatchProvider ?? DefaultStopwatchProvider.Instance;
+
+        /// <inheritdoc/>
+        public override AsyncUnaryCall<TResponse> AsyncUnaryCall<TRequest, TResponse>(


Yes, this one will always be used at least for async calls. It's the blocking one that may be unused if the caller added response header handlers. That's because the gRPC blocking client method does not expose response headers, but our client does allow to set response header handlers for both async and blocking. See the code that does that here.

amanda-tarafa · 2026-09-01T21:29:27Z

+            }
+            catch (Exception ex)
+            {
+                elapsedMs = stopwatch.ElapsedMilliseconds;


This value will be overwritten in finally, I think you want to stop the stopwatch here and after success as well, and in finally only access the the elapsed milliseconds.

Good catch, fixed!

amanda-tarafa · 2026-09-01T21:35:05Z

+                    RecordAttemptMetrics(elapsedMs, labels);
+                    await RecordServerTimingMetricsAsync(call.ResponseHeadersAsync, labels).ConfigureAwait(false);


Suggested change

RecordAttemptMetrics(elapsedMs, labels);

await RecordServerTimingMetricsAsync(call.ResponseHeadersAsync, labels).ConfigureAwait(false);

var recordTimingTask = RecordServerTimingMetricsAsync(call.ResponseHeadersAsync, labels);

RecordAttemptMetrics(elapsedMs, labels);

await recordTimingTask.ConfigureAwait(false);

robertvoinescu-work · 2026-09-08T20:17:09Z


-            public override AsyncUnaryCall<TResponse> AsyncUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string host, CallOptions options, TRequest request) =>
-                throw new NotImplementedException();
+            public override AsyncUnaryCall<TResponse> AsyncUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string host, CallOptions options, TRequest request)


We need AsyncUnary implemented now that all blocking unary calls are handled as async. Using this interceptor as it was would result in "NotImplementedException".

Can we delete the test code below that uses the blocking call? No need to keep test code around.

Also, let's clean the production code from interceptors blocking calls that have them, leaving a note on why we can do so.

amanda-tarafa

Only nits. Feel free to push those changes here or in a follow up PR.

amanda-tarafa · 2026-09-09T23:31:46Z

+    /// <summary>
+    /// Records server-timing metrics (such as GFE latency) from response headers.
+    /// </summary>
+    internal static void RecordServerTimingMetrics(Metadata headers, KeyValuePair<string, object>[] labels)


nit: Equally, if this is now being called from above only, squash it into a single method.

amanda-tarafa · 2026-09-09T23:33:03Z

+        }
+    }
+
+    internal static void RecordServerTimingMetrics(string header, string metricPrefix, Action<double> recordAction)


nit: And this one can probable be a local method to the method above.

amanda-tarafa · 2026-09-09T23:50:30Z


-            public override AsyncUnaryCall<TResponse> AsyncUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string host, CallOptions options, TRequest request) =>
-                throw new NotImplementedException();
+            public override AsyncUnaryCall<TResponse> AsyncUnaryCall<TRequest, TResponse>(Method<TRequest, TResponse> method, string host, CallOptions options, TRequest request)


Can we delete the test code below that uses the blocking call? No need to keep test code around.

Also, let's clean the production code from interceptors blocking calls that have them, leaving a note on why we can do so.

robertvoinescu-work · 2026-09-10T17:13:18Z

I've left a couple open because I believe they methods will need to be internal for follow-up work, and will look back here once done. I also will remove all blocking unary interceptors in a follow-up (note test code has been cleaned up). Here is the bug to track both:

b/559681168

robertvoinescu-work requested a review from a team as a code owner July 16, 2026 17:31

product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Jul 16, 2026

gemini-code-assist Bot reviewed Jul 16, 2026

View reviewed changes

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch from 5f7d234 to 01b86b9 Compare July 16, 2026 17:46

robertvoinescu-work marked this pull request as draft July 16, 2026 17:54

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch 2 times, most recently from 648b959 to 2df210a Compare July 17, 2026 18:21

robertvoinescu-work marked this pull request as ready for review July 17, 2026 18:23

robertvoinescu-work requested review from amanda-tarafa and efevans July 17, 2026 18:24

efevans requested changes Jul 20, 2026

View reviewed changes

robertvoinescu-work commented Jul 20, 2026

View reviewed changes

Comment thread apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerBuiltInMetrics.Interceptor.cs Outdated

amanda-tarafa requested changes Jul 21, 2026

View reviewed changes

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch from 2df210a to 7105f2b Compare July 23, 2026 22:31

robertvoinescu-work requested review from amanda-tarafa and efevans July 23, 2026 22:35

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch 3 times, most recently from 73c320a to 57d2acd Compare July 24, 2026 19:01

efevans requested changes Jul 25, 2026

View reviewed changes

Comment thread ...ogle.Cloud.Spanner.V1/Google.Cloud.Spanner.V1.Tests/SpannerBuiltInMetricsInterceptorTests.cs Outdated

Comment thread ...ogle.Cloud.Spanner.V1/Google.Cloud.Spanner.V1.Tests/SpannerBuiltInMetricsInterceptorTests.cs Outdated

robertvoinescu-work requested a review from a team July 26, 2026 00:31

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch from 57d2acd to f8f930f Compare July 27, 2026 18:03

robertvoinescu-work requested a review from efevans July 27, 2026 18:04

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch 4 times, most recently from 4ca632d to e0ae9de Compare July 30, 2026 20:39

efevans approved these changes Jul 31, 2026

View reviewed changes

amanda-tarafa requested changes Aug 4, 2026

View reviewed changes

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch from e0ae9de to c4849b6 Compare August 7, 2026 04:53

robertvoinescu-work mentioned this pull request Aug 7, 2026

feat(Spanner.V1): Add attempt metrics for streams #15806

Open

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch 5 times, most recently from 966c7e8 to 49e79dd Compare August 7, 2026 18:00

robertvoinescu-work requested a review from amanda-tarafa August 7, 2026 18:01

robertvoinescu-work commented Aug 21, 2026

View reviewed changes

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch 2 times, most recently from 9908b64 to f492d41 Compare August 28, 2026 19:18

robertvoinescu-work mentioned this pull request Sep 1, 2026

feat(Spanner.V1): Add wrapper for built in metrics #15807

Draft

amanda-tarafa requested changes Sep 1, 2026

View reviewed changes

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch from f492d41 to 3eb4579 Compare September 8, 2026 20:10

robertvoinescu-work requested a review from amanda-tarafa September 8, 2026 20:13

robertvoinescu-work commented Sep 8, 2026

View reviewed changes

amanda-tarafa approved these changes Sep 9, 2026

View reviewed changes

feat(Spanner.V1): Add interceptor for Built-In metrics.

c5bebc6

robertvoinescu-work force-pushed the spanner/builtInMetricsInterceptor branch from 3eb4579 to c5bebc6 Compare September 10, 2026 17:08

robertvoinescu-work merged commit 6f4d5cc into googleapis:main Sep 10, 2026
18 checks passed

	EmitServerTimingMetrics(header.Value, GfeMetricPrefix, duration => s_gfeLatency.Record(duration, labels));
	EmitServerTimingMetrics(header.Value);

		RecordAttemptMetrics(elapsedMs, labels);
		await RecordServerTimingMetricsAsync(call.ResponseHeadersAsync, labels).ConfigureAwait(false);

Conversation

robertvoinescu-work commented Jul 16, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efevans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efevans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amanda-tarafa Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robertvoinescu-work Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amanda-tarafa Aug 4, 2026 •

edited

Loading

robertvoinescu-work Aug 21, 2026 •

edited

Loading

robertvoinescu-work Sep 8, 2026 •

edited

Loading