feat(Spanner.V1): Add operation metrics on unary RPCs for built in metrics by robertvoinescu-work · Pull Request #15807 · googleapis/google-cloud-dotnet

robertvoinescu-work · 2026-08-07T05:13:03Z

b/404948213

gemini-code-assist

Code Review

This pull request updates the Renovate configuration to include the Google Cloud CommerceProducer V1Beta API, bumps dependency versions for SharpCompress and LibGit2Sharp, and updates package descriptions in the README. Additionally, it introduces a large number of auto-generated code snippets for various services under the Google Ads AdManager V1 API, including AdRule, AdSpot, CdnConfig, ContentBundle, CreativeSet, CustomTargetingValue, LiveStream, and Order service clients. There are no review comments to address, and I have no feedback to provide.

robertvoinescu-work · 2026-09-22T15:46:24Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a BuiltInMetricsWrapper for SpannerClient to intercept API calls and record built-in metrics, along with corresponding unit tests and builder integration. The review feedback highlights several important improvements: overriding ClientIdentity in the wrapper to delegate to the inner client, addressing metric inaccuracies for lazy-loaded paged operations (ListSessions), ensuring builder interceptors do not overwrite existing client/task instances, and asserting on exception messages in unit tests to ensure correct propagation.

gemini-code-assist · 2026-09-22T15:49:08Z

+        public override PagedEnumerable<ListSessionsResponse, Session> ListSessions(ListSessionsRequest request, CallSettings callSettings = null)
+            => ExecuteWithInstrumentation(nameof(ListSessions), request, () => _inner.ListSessions(request, callSettings));
+        public override PagedAsyncEnumerable<ListSessionsResponse, Session> ListSessionsAsync(ListSessionsRequest request, CallSettings callSettings = null)
+            => ExecuteWithInstrumentation(nameof(ListSessionsAsync), request, () => _inner.ListSessionsAsync(request, callSettings));


The ListSessions and ListSessionsAsync methods return lazy PagedEnumerable and PagedAsyncEnumerable instances. The actual gRPC calls to the Spanner service are only executed when these enumerables are iterated.

Because ExecuteWithInstrumentation only wraps the initial call that constructs the enumerable, the recorded metrics (latency and status) will only reflect the negligible time taken to instantiate the lazy enumerable (typically ~0ms with OK status), rather than the actual network calls made during iteration.

Consider documenting this limitation, or if accurate metrics are required for paged operations, wrapping the returned enumerables to intercept the actual page-fetching RPCs.

This is to spec leaving as is.

Then do we need to wrap them here? There's no Spanner latency or anything like that we'd be recording. Nor are we encompassing the whole pagination operation.

There's some guidance on how to do this for golden signals I think, maybe we should follow that here as well.

Happy to punt these paginated methods for a later PR while we figure it out.

I will revisit with spanner, but looking more at other languages I see they do not emit operation metrics for these methods. I've made these methods purely pass through for now.

efevans

Looks good, had one question and a few code suggestions

amanda-tarafa

Overall good with a few change requests. When we get to GAPIC observability I'd really like to look into improving this implementation.

amanda-tarafa · 2026-10-01T18:05:08Z

+        partial void InterceptBuild(ref SpannerClient client)
+        {
+            client ??= MaybeCreateEmulatorClientBuilder()?.Build();
+            client ??= Build();


Please call BuildImpl here and wrap the result on your client. That way, if we change the implementation of BuildImpl you don't have to make changes to this code. For instance, right now you need to add a call to Validate on the wrapping method. If in a month we add a "log client creation" call on BuildImp for all GAPICs, then you'll have to come here and change this code to add that.

Done, but there's a cascading effect to this change as there's no way to share the client Id between the interceptor and client except through a side channel - this was making me lean towards this initially, but let me know what you think about have an async local build context instead - I've added a comment to highlight it.

amanda-tarafa · 2026-10-01T18:06:03Z

+        private SpannerClient BuildWithMetrics(CallInvoker callInvoker)
+        {
+            Validate();
+            var identity = Labeler.GenerateIdentity();


This I don't understand.

Ah, is it because the attempt interceptor needs it? So, it's the attempt interceptor and the client wrapper that need it. can you just remove it from SpannerClient?

Correct, removed.

amanda-tarafa · 2026-10-01T19:37:34Z

+        public override SpannerSettings Settings => _inner.Settings;
+
+        ///<inheritdoc/>
+        internal override ClientIdentity ClientIdentity => _clientIdentity;


I think you just can remove ClientIdentity from SpannerClient.

amanda-tarafa · 2026-10-01T20:17:00Z

+    /// A delegating wrapper around <see cref="SpannerClient"/> that intercepts
+    /// execution of all methods to record metrics.
+    /// </summary>
+    internal class BuiltInMetricsWrapper : SpannerClient


Because we cannot extend SpannerClientImp what will happen when Spanner adds a new method is not that we won't instrument it, it's that it will be unusable because the client we are returning will throw Unimplemented. We need to write a unit test for this that via reflection checks that all Unimplemented methods in SpannerClient are implemented in BuiltInMetricWrapper.

I've added a unit test that explicitly states which methods need to be implemented.

amanda-tarafa · 2026-10-01T20:27:15Z

+        private static StatusCode GetStatus(Exception ex) => ex switch
+        {
+            RpcException rpcEx => rpcEx.StatusCode,
+            OperationCanceledException => StatusCode.Cancelled,


Hmmm. I think we are not doing this in the attempt, and I'm not sure we should do it here either. We probably care about RPC errors only?

Removed the operation cancelled exception casting. And moved this method into the base class so it can be used here and in the interceptor.

amanda-tarafa · 2026-10-01T20:31:24Z

+    private static readonly Exception s_rpcException = new Grpc.Core.RpcException(s_timeoutStatus);
+
+    [Fact]
+    public void Properties_DelegateToInner()


We need to this via reflection and for methods as well.

Good point. I added a test to make sure that the methods are also wired up correctly - checking for eventual delegation to the inner client..

amanda-tarafa

Looking good. I have a suggestion for new tests that may come in a later PR, and a couple small things. Thanks!

amanda-tarafa · 2026-10-07T16:45:10Z

+        private BuiltInMetricsWrapper(SpannerClient inner, ClientIdentity? clientIdentity = null)
+        {
+            _inner = GaxPreconditions.CheckNotNull(inner, nameof(inner));
+            _clientIdentity = clientIdentity ?? Labeler.GenerateIdentity();


Let's instead check that the identity is not null?

Done, I mostly had this for convenience in the tests, but it's not really saving much lines of code there.

amanda-tarafa · 2026-10-07T16:52:02Z

-        partial void InterceptBuild(ref SpannerClient client) => client = MaybeCreateEmulatorClientBuilder()?.Build();
+        partial void InterceptBuild(ref SpannerClient client)
+        {
+            client = MaybeCreateEmulatorClientBuilder()?.Build() ?? Build();


I'm sorry, I know I suggested we didn't wrap the emulator client earlier. But thinking about it more, I think that the emulator client call invoker will have the attempt interceptor and there's no easy way to get rid of that. So maybe we should just wrap it here for completeness. I still think it's weird to have these metrics for the emulator, but it'd be weirder to have them partially. And in any case, this is all opt-in.

What do you think?

I like having as much consistency between emulator and non emulator makes things a bit easier to follow - if it wasn't opt in I might lean the other way. On another note if users want to consume these metrics it might provide an easy onramp for them if they can get these in an emulated environment first. I moved to wrap everything consistently.

amanda-tarafa · 2026-10-07T17:08:11Z

+    }
+
+    [Fact]
+    public async Task Builder_ClientIdentity_MatchesBetweenInterceptorAndWrapper_Sync()


Left a comment earlier. This test is good and tests the happy case. But let's add a couple other tests for checking this same thing when the builder is used to build several clients at the same time. We can build enough clients (20. 50?) to make sure that it will at least flake if we are doing something wrong or if we remove the async local.

This is a good point, I've added more tests to cover all the cases mentioned: multiple clients run a mix of sync and async builds.

robertvoinescu-work · 2026-10-07T23:18:01Z

+            builder.InitializeLocalBuildContext();
+            try
+            {
+                var rawClient = builder.BuildImpl();


NOTE: Both emulated and non-emulated now use BuildImpl whereas before the emulator used Build. This makes the code a little cleaner by keeping things symmetric. It behaves effecitvely the same, but wheras before we would go through the MaybeCreateEmulatorClientBuilder a second time returning null and treating it like a normal client builder from that point on we now just avoid that recursion immediately going to BuildImpl.

amanda-tarafa

Thanks!

amanda-tarafa · 2026-10-07T23:37:25Z

+            var tasks = new List<Task<BuiltInMetricsWrapper>>();
+            for (int i = 0; i < clientCount; i++)
+            {
+                if (buildMode == BuildMode.AsyncOnly || (buildMode == BuildMode.Mixed && i % 2 == 0))


robertvoinescu-work · 2026-10-08T16:24:53Z

Integration Tests for Spanner.V1 and Spanner.Data pass, merging.

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

robertvoinescu-work force-pushed the spanner/builtInWrapper branch from 51dee39 to 6892696 Compare August 7, 2026 05:15

gemini-code-assist Bot reviewed Aug 7, 2026

View reviewed changes

robertvoinescu-work force-pushed the spanner/builtInWrapper branch 2 times, most recently from c660e31 to 0ecadfd Compare August 7, 2026 05:46

robertvoinescu-work force-pushed the spanner/builtInWrapper branch from 0ecadfd to f26d752 Compare September 22, 2026 07:05

gemini-code-assist Bot reviewed Sep 22, 2026

View reviewed changes

chore: pull out common fakes for built in metrics tests.

5143c6b

robertvoinescu-work force-pushed the spanner/builtInWrapper branch from f26d752 to e6d2907 Compare September 22, 2026 16:16

robertvoinescu-work marked this pull request as ready for review September 22, 2026 16:17

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

robertvoinescu-work changed the title ~~feat(Spanner.V1): Add wrapper for built in metrics~~ feat(Spanner.V1): Add operation metrics on unary RPCs for built in metrics Sep 22, 2026

robertvoinescu-work requested review from amanda-tarafa and efevans September 22, 2026 16:20

efevans requested changes Sep 22, 2026

View reviewed changes

robertvoinescu-work force-pushed the spanner/builtInWrapper branch from e6d2907 to 51e2899 Compare September 29, 2026 21:16

amanda-tarafa requested changes Oct 1, 2026

View reviewed changes

robertvoinescu-work force-pushed the spanner/builtInWrapper branch 3 times, most recently from 1045e36 to e9b4d21 Compare October 6, 2026 00:08

robertvoinescu-work commented Oct 6, 2026

View reviewed changes

Comment thread apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerClientBuilderPartial.cs

robertvoinescu-work force-pushed the spanner/builtInWrapper branch 2 times, most recently from b76c42d to 593c136 Compare October 6, 2026 18:26

amanda-tarafa reviewed Oct 7, 2026

View reviewed changes

feat(Spanner.V1): Instrument unary operations for BuiltInMetrics.

aed8b7b

robertvoinescu-work force-pushed the spanner/builtInWrapper branch from 593c136 to aed8b7b Compare October 7, 2026 22:53

robertvoinescu-work commented Oct 7, 2026

View reviewed changes

robertvoinescu-work requested review from amanda-tarafa and efevans October 7, 2026 23:19

amanda-tarafa approved these changes Oct 7, 2026

View reviewed changes

efevans approved these changes Oct 8, 2026

View reviewed changes

robertvoinescu-work merged commit f4b0ace into googleapis:main Oct 8, 2026
21 checks passed

Conversation

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

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

robertvoinescu-work commented Sep 22, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist Bot Sep 22, 2026

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!

efevans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertvoinescu-work Oct 5, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amanda-tarafa left a comment

Choose a reason for hiding this comment

Uh oh!

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

robertvoinescu-work commented Aug 7, 2026 •

edited

Loading

robertvoinescu-work Oct 5, 2026 •

edited

Loading

robertvoinescu-work Oct 7, 2026 •

edited

Loading