From 381c86b34c695eeaa95fa36f259343d0c7b2870b Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Wed, 10 Dec 2025 20:04:49 +0000 Subject: [PATCH 1/3] feat: next release is v6.105.0-rc1 From c5f1af72fbcb510bd92a0a0c7f58a768f446543e Mon Sep 17 00:00:00 2001 From: surbhigarg92 Date: Tue, 9 Dec 2025 16:26:49 +0530 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20Refine=20connecitivity=20metrics=20t?= =?UTF-8?q?o=20capture=20RPCs=20with=20no=20response=20he=E2=80=A6=20(#425?= =?UTF-8?q?2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Refine connecitivity metrics to capture RPCs with no response headers * test fix --- .../clirr-ignored-differences.xml | 30 ++++ .../cloud/spanner/BuiltInMetricsRecorder.java | 28 ++-- .../cloud/spanner/BuiltInMetricsTracer.java | 32 ++-- .../google/cloud/spanner/CompositeTracer.java | 47 +----- .../spanner/spi/v1/HeaderInterceptor.java | 151 ++++++++---------- ...OpenTelemetryBuiltInMetricsTracerTest.java | 22 +-- .../cloud/spanner/spi/v1/GfeLatencyTest.java | 14 +- .../spanner/spi/v1/SpannerRpcMetricsTest.java | 21 +-- 8 files changed, 157 insertions(+), 188 deletions(-) diff --git a/google-cloud-spanner/clirr-ignored-differences.xml b/google-cloud-spanner/clirr-ignored-differences.xml index fd420f42457..21b2eb228cb 100644 --- a/google-cloud-spanner/clirr-ignored-differences.xml +++ b/google-cloud-spanner/clirr-ignored-differences.xml @@ -1062,4 +1062,34 @@ com/google/cloud/spanner/connection/Connection java.lang.Object getConnectionPropertyValue(com.google.cloud.spanner.connection.ConnectionProperty) + + + 7002 + com/google/cloud/spanner/CompositeTracer + void recordAFELatency(java.lang.Long) + + + 7002 + com/google/cloud/spanner/CompositeTracer + void recordAFELatency(java.lang.Float) + + + 7002 + com/google/cloud/spanner/CompositeTracer + void recordAfeHeaderMissingCount(java.lang.Long) + + + 7002 + com/google/cloud/spanner/CompositeTracer + void recordGFELatency(java.lang.Long) + + + 7002 + com/google/cloud/spanner/CompositeTracer + void recordGFELatency(java.lang.Float) + + 7002 + com/google/cloud/spanner/CompositeTracer + void recordGfeHeaderMissingCount(java.lang.Long) + diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsRecorder.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsRecorder.java index 2eb7c8d2971..67e75a1d383 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsRecorder.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsRecorder.java @@ -99,21 +99,23 @@ class BuiltInMetricsRecorder extends OpenTelemetryMetricsRecorder { void recordServerTimingHeaderMetrics( Float gfeLatency, Float afeLatency, - Long gfeHeaderMissingCount, - Long afeHeaderMissingCount, - Map attributes) { + Map attributes, + boolean isDirectPathUsed, + boolean isAfeEnabled) { io.opentelemetry.api.common.Attributes otelAttributes = toOtelAttributes(attributes); - if (gfeLatency != null) { - gfeLatencyRecorder.record(gfeLatency, otelAttributes); + if (!isDirectPathUsed) { + if (gfeLatency != null) { + gfeLatencyRecorder.record(gfeLatency, otelAttributes); + } else { + gfeHeaderMissingCountRecorder.add(1, otelAttributes); + } } - if (gfeHeaderMissingCount > 0) { - gfeHeaderMissingCountRecorder.add(gfeHeaderMissingCount, otelAttributes); - } - if (afeLatency != null) { - afeLatencyRecorder.record(afeLatency, otelAttributes); - } - if (afeHeaderMissingCount > 0) { - afeHeaderMissingCountRecorder.add(afeHeaderMissingCount, otelAttributes); + if (isAfeEnabled) { + if (afeLatency != null) { + afeLatencyRecorder.record(afeLatency, otelAttributes); + } else { + afeHeaderMissingCountRecorder.add(1, otelAttributes); + } } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsTracer.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsTracer.java index cdf7dda2e6f..a982a3f11ab 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsTracer.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsTracer.java @@ -39,10 +39,10 @@ class BuiltInMetricsTracer extends MetricsTracer implements ApiTracer { private final Map attributes = new HashMap<>(); private Float gfeLatency = null; private Float afeLatency = null; - private TraceWrapper traceWrapper; - private long gfeHeaderMissingCount = 0; - private long afeHeaderMissingCount = 0; + private final TraceWrapper traceWrapper; private final ISpan currentSpan; + private boolean isDirectPathUsed; + private boolean isAfeEnabled; BuiltInMetricsTracer( MethodName methodName, @@ -66,7 +66,7 @@ public void attemptSucceeded() { super.attemptSucceeded(); attributes.put(STATUS_ATTRIBUTE, StatusCode.Code.OK.toString()); builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics( - gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes); + gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled); } } @@ -80,7 +80,7 @@ public void attemptCancelled() { super.attemptCancelled(); attributes.put(STATUS_ATTRIBUTE, StatusCode.Code.CANCELLED.toString()); builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics( - gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes); + gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled); } } @@ -98,7 +98,7 @@ public void attemptFailedDuration(Throwable error, java.time.Duration delay) { super.attemptFailedDuration(error, delay); attributes.put(STATUS_ATTRIBUTE, extractStatus(error)); builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics( - gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes); + gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled); } } @@ -115,7 +115,7 @@ public void attemptFailedRetriesExhausted(Throwable error) { super.attemptFailedRetriesExhausted(error); attributes.put(STATUS_ATTRIBUTE, extractStatus(error)); builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics( - gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes); + gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled); } } @@ -132,24 +132,16 @@ public void attemptPermanentFailure(Throwable error) { super.attemptPermanentFailure(error); attributes.put(STATUS_ATTRIBUTE, extractStatus(error)); builtInOpenTelemetryMetricsRecorder.recordServerTimingHeaderMetrics( - gfeLatency, afeLatency, gfeHeaderMissingCount, afeHeaderMissingCount, attributes); + gfeLatency, afeLatency, attributes, isDirectPathUsed, isAfeEnabled); } } - void recordGFELatency(Float gfeLatency) { + public void recordServerTimingHeaderMetrics( + Float gfeLatency, Float afeLatency, boolean isDirectPathUsed, boolean isAfeEnabled) { this.gfeLatency = gfeLatency; - } - - void recordAFELatency(Float afeLatency) { + this.isDirectPathUsed = isDirectPathUsed; this.afeLatency = afeLatency; - } - - void recordGfeHeaderMissingCount(Long value) { - this.gfeHeaderMissingCount = value; - } - - void recordAfeHeaderMissingCount(Long value) { - this.afeHeaderMissingCount = value; + this.isAfeEnabled = isAfeEnabled; } @Override diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java index 5d3b416788f..105dbd0a512 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java @@ -191,50 +191,13 @@ public void addAttributes(Map attributes) { } } - public void recordGFELatency(Long gfeLatency) { + public void recordServerTimingHeaderMetrics( + Float gfeLatency, Float afeLatency, boolean isDirectPathUsed, boolean isAfeEnabled) { for (ApiTracer child : children) { if (child instanceof BuiltInMetricsTracer) { - ((BuiltInMetricsTracer) child).recordGFELatency(Float.valueOf(gfeLatency)); - } - } - } - - public void recordGfeHeaderMissingCount(Long value) { - for (ApiTracer child : children) { - if (child instanceof BuiltInMetricsTracer) { - ((BuiltInMetricsTracer) child).recordGfeHeaderMissingCount(value); - } - } - } - - public void recordAFELatency(Long afeLatency) { - for (ApiTracer child : children) { - if (child instanceof BuiltInMetricsTracer) { - ((BuiltInMetricsTracer) child).recordAFELatency(Float.valueOf(afeLatency)); - } - } - } - - public void recordAfeHeaderMissingCount(Long value) { - for (ApiTracer child : children) { - if (child instanceof BuiltInMetricsTracer) { - ((BuiltInMetricsTracer) child).recordAfeHeaderMissingCount(value); - } - } - } - - public void recordGFELatency(Float gfeLatency) { - for (ApiTracer child : children) { - if (child instanceof BuiltInMetricsTracer) { - ((BuiltInMetricsTracer) child).recordGFELatency(gfeLatency); - } - } - } - - public void recordAFELatency(Float afeLatency) { - for (ApiTracer child : children) { - if (child instanceof BuiltInMetricsTracer) { - ((BuiltInMetricsTracer) child).recordAFELatency(afeLatency); + ((BuiltInMetricsTracer) child) + .recordServerTimingHeaderMetrics( + gfeLatency, afeLatency, isDirectPathUsed, isAfeEnabled); } } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/HeaderInterceptor.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/HeaderInterceptor.java index 0b02238a807..985ad68a429 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/HeaderInterceptor.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/HeaderInterceptor.java @@ -28,14 +28,9 @@ import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.spanner.admin.database.v1.DatabaseName; -import io.grpc.CallOptions; -import io.grpc.Channel; -import io.grpc.ClientCall; -import io.grpc.ClientInterceptor; +import io.grpc.*; import io.grpc.ForwardingClientCall.SimpleForwardingClientCall; import io.grpc.ForwardingClientCallListener.SimpleForwardingClientCallListener; -import io.grpc.Metadata; -import io.grpc.MethodDescriptor; import io.grpc.alts.AltsContextUtil; import io.opencensus.stats.MeasureMap; import io.opencensus.stats.Stats; @@ -91,6 +86,8 @@ class HeaderInterceptor implements ClientInterceptor { private static final Logger LOGGER = Logger.getLogger(HeaderInterceptor.class.getName()); private static final Level LEVEL = Level.INFO; private final SpannerRpcMetrics spannerRpcMetrics; + private Float gfeLatency; + private Float afeLatency; HeaderInterceptor(SpannerRpcMetrics spannerRpcMetrics) { this.spannerRpcMetrics = spannerRpcMetrics; @@ -113,24 +110,46 @@ public void start(Listener responseListener, Metadata headers) { TagContext tagContext = getTagContext(key, method.getFullMethodName(), databaseName); Attributes attributes = getMetricAttributes(key, method.getFullMethodName(), databaseName); - Map builtInMetricsAttributes = - getBuiltInMetricAttributes(key, databaseName); - builtInMetricsAttributes.put(BuiltInMetricsConstant.REQUEST_ID_KEY.getKey(), requestId); - addBuiltInMetricAttributes(compositeTracer, builtInMetricsAttributes); - if (span != null) { - span.setAttribute(XGoogSpannerRequestId.REQUEST_ID, requestId); - } + super.start( new SimpleForwardingClientCallListener(responseListener) { @Override public void onHeaders(Metadata metadata) { - // Check if the call uses DirectPath by inspecting the ALTS context. - boolean isDirectPathUsed = AltsContextUtil.check(getAttributes()); - addDirectPathUsedAttribute(compositeTracer, isDirectPathUsed); - processHeader( - metadata, tagContext, attributes, span, compositeTracer, isDirectPathUsed); + String serverTiming = metadata.get(SERVER_TIMING_HEADER_KEY); + try { + // Get gfe and afe Latency value + Map serverTimingMetrics = parseServerTimingHeader(serverTiming); + gfeLatency = serverTimingMetrics.get(GFE_TIMING_HEADER); + afeLatency = serverTimingMetrics.get(AFE_TIMING_HEADER); + } catch (NumberFormatException e) { + LOGGER.log(LEVEL, "Invalid server-timing object in header: {}", serverTiming); + } + super.onHeaders(metadata); } + + @Override + public void onClose(Status status, Metadata trailers) { + // Record Built-in Metrics + boolean isDirectPathUsed = AltsContextUtil.check(getAttributes()); + boolean isAfeEnabled = GapicSpannerRpc.isEnableAFEServerTiming(); + recordSpan(span, requestId); + recordCustomMetrics(tagContext, attributes, isDirectPathUsed); + Map builtInMetricsAttributes = new HashMap<>(); + try { + builtInMetricsAttributes = getBuiltInMetricAttributes(key, databaseName); + } catch (ExecutionException e) { + LOGGER.log( + LEVEL, "Unable to get built-in metric attributes {}", e.getMessage()); + } + recordBuiltInMetrics( + compositeTracer, + builtInMetricsAttributes, + requestId, + isDirectPathUsed, + isAfeEnabled); + super.onClose(status, trailers); + } }, headers); } catch (ExecutionException executionException) { @@ -141,29 +160,12 @@ public void onHeaders(Metadata metadata) { }; } - private void processHeader( - Metadata metadata, - TagContext tagContext, - Attributes attributes, - Span span, - CompositeTracer compositeTracer, - boolean isDirectPathUsed) { + private void recordCustomMetrics( + TagContext tagContext, Attributes attributes, Boolean isDirectPathUsed) { + // Record OpenCensus and Custom OpenTelemetry Metrics MeasureMap measureMap = STATS_RECORDER.newMeasureMap(); - String serverTiming = metadata.get(SERVER_TIMING_HEADER_KEY); - try { - // Previous implementation parsed the GFE latency directly using: - // long latency = Long.parseLong(serverTiming.substring("gfet4t7; dur=".length())); - // This approach assumed the serverTiming header contained exactly one metric "gfet4t7". - // If additional metrics were introduced in the header, older versions of the library - // would fail to parse it correctly. To make the parsing more robust, the logic has been - // updated to handle multiple metrics gracefully. - - Map serverTimingMetrics = parseServerTimingHeader(serverTiming); - Float gfeLatency = serverTimingMetrics.get(GFE_TIMING_HEADER); - boolean isAfeEnabled = GapicSpannerRpc.isEnableAFEServerTiming(); - Float afeLatency = isAfeEnabled ? serverTimingMetrics.get(AFE_TIMING_HEADER) : null; - // Record OpenCensus and Custom OpenTelemetry Metrics + if (!isDirectPathUsed) { if (gfeLatency != null) { long gfeVal = gfeLatency.longValue(); measureMap.put(SPANNER_GFE_LATENCY, gfeVal); @@ -174,39 +176,35 @@ private void processHeader( measureMap.put(SPANNER_GFE_HEADER_MISSING_COUNT, 1L); spannerRpcMetrics.recordGfeHeaderMissingCount(1L, attributes); } - measureMap.record(tagContext); + } + measureMap.record(tagContext); + } - // Record Built-in Metrics - if (compositeTracer != null) { - // GFE Latency Metrics - if (!isDirectPathUsed) { - if (gfeLatency != null) { - compositeTracer.recordGFELatency(gfeLatency); - } else { - compositeTracer.recordGfeHeaderMissingCount(1L); - } - } - // AFE Tracing - if (isAfeEnabled) { - if (afeLatency != null) { - compositeTracer.recordAFELatency(afeLatency); - } else { - compositeTracer.recordAfeHeaderMissingCount(1L); - } - } + private void recordSpan(Span span, String requestId) { + if (span != null) { + if (gfeLatency != null) { + span.setAttribute("gfe_latency", gfeLatency.toString()); } - - // Record Span Attributes - if (span != null) { - if (gfeLatency != null) { - span.setAttribute("gfe_latency", gfeLatency.toString()); - } - if (afeLatency != null) { - span.setAttribute("afe_latency", afeLatency.toString()); - } + if (afeLatency != null) { + span.setAttribute("afe_latency", afeLatency.toString()); } - } catch (NumberFormatException e) { - LOGGER.log(LEVEL, "Invalid server-timing object in header: {}", serverTiming); + span.setAttribute(XGoogSpannerRequestId.REQUEST_ID, requestId); + } + } + + private void recordBuiltInMetrics( + CompositeTracer compositeTracer, + Map builtInMetricsAttributes, + String requestId, + Boolean isDirectPathUsed, + Boolean isAfeEnabled) { + if (compositeTracer != null) { + builtInMetricsAttributes.put(BuiltInMetricsConstant.REQUEST_ID_KEY.getKey(), requestId); + builtInMetricsAttributes.put( + BuiltInMetricsConstant.DIRECT_PATH_USED_KEY.getKey(), Boolean.toString(isDirectPathUsed)); + compositeTracer.addAttributes(builtInMetricsAttributes); + compositeTracer.recordServerTimingHeaderMetrics( + gfeLatency, afeLatency, isDirectPathUsed, isAfeEnabled); } } @@ -309,19 +307,4 @@ private Map getBuiltInMetricAttributes(String key, DatabaseName return attributes; }); } - - private void addBuiltInMetricAttributes( - CompositeTracer compositeTracer, Map builtInMetricsAttributes) { - if (compositeTracer != null) { - compositeTracer.addAttributes(builtInMetricsAttributes); - } - } - - private void addDirectPathUsedAttribute( - CompositeTracer compositeTracer, Boolean isDirectPathUsed) { - if (compositeTracer != null) { - compositeTracer.addAttributes( - BuiltInMetricsConstant.DIRECT_PATH_USED_KEY.getKey(), Boolean.toString(isDirectPathUsed)); - } - } } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/OpenTelemetryBuiltInMetricsTracerTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/OpenTelemetryBuiltInMetricsTracerTest.java index b5a62ba43b6..ce1bb87ed4a 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/OpenTelemetryBuiltInMetricsTracerTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/OpenTelemetryBuiltInMetricsTracerTest.java @@ -69,21 +69,17 @@ public class OpenTelemetryBuiltInMetricsTracerTest extends AbstractNettyMockServ private static final Statement SELECT_RANDOM = Statement.of("SELECT * FROM random"); private static final Statement UPDATE_RANDOM = Statement.of("UPDATE random SET foo=1 WHERE id=1"); private static InMemoryMetricReader metricReader; - private static Map attributes = + private static final Map attributes = BuiltInMetricsProvider.INSTANCE.createClientAttributes(); - private static Attributes expectedCommonBaseAttributes = + private static final Attributes expectedCommonBaseAttributes = Attributes.builder() .put(BuiltInMetricsConstant.CLIENT_NAME_KEY, "spanner-java/") .put(BuiltInMetricsConstant.CLIENT_UID_KEY, attributes.get("client_uid")) .put(BuiltInMetricsConstant.INSTANCE_ID_KEY, "i") .put(BuiltInMetricsConstant.DATABASE_KEY, "d") .put(BuiltInMetricsConstant.DIRECT_PATH_ENABLED_KEY, "false") + .put(BuiltInMetricsConstant.DIRECT_PATH_USED_KEY, "false") .build(); - ; - private static Attributes expectedCommonRequestAttributes = - Attributes.builder().put(BuiltInMetricsConstant.DIRECT_PATH_USED_KEY, "false").build(); - ; - private static final double MIN_LATENCY = 0; private DatabaseClient client; @@ -175,7 +171,6 @@ public void testMetricsSingleUseQuery() { double elapsed = stopwatch.elapsed(TimeUnit.MILLISECONDS); Attributes expectedAttributes = expectedCommonBaseAttributes.toBuilder() - .putAll(expectedCommonRequestAttributes) .put(BuiltInMetricsConstant.STATUS_KEY, "OK") .put(BuiltInMetricsConstant.METHOD_KEY, "Spanner.ExecuteStreamingSql") .build(); @@ -237,7 +232,6 @@ public void testMetricsWithGaxRetryUnaryRpc() { Attributes expectedAttributesBeginTransactionOK = expectedCommonBaseAttributes.toBuilder() - .putAll(expectedCommonRequestAttributes) .put(BuiltInMetricsConstant.STATUS_KEY, "OK") .put(BuiltInMetricsConstant.METHOD_KEY, "Spanner.BeginTransaction") .build(); @@ -323,7 +317,6 @@ public void testNoNetworkConnection() { Attributes expectedAttributesCreateSessionOK = expectedCommonBaseAttributes.toBuilder() - .putAll(expectedCommonRequestAttributes) .put(BuiltInMetricsConstant.STATUS_KEY, "OK") .put(BuiltInMetricsConstant.METHOD_KEY, "Spanner.CreateSession") // Include the additional attributes that are added by the HeaderInterceptor in the @@ -347,6 +340,10 @@ public void testNoNetworkConnection() { // Attempt count should have a failed metric point for CreateSession. assertEquals( 1, getAggregatedValue(attemptCountMetricData, expectedAttributesCreateSessionFailed), 0); + assertTrue( + checkIfMetricExists(metricReader, BuiltInMetricsConstant.GFE_CONNECTIVITY_ERROR_NAME)); + assertTrue( + checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME)); } @Test @@ -383,13 +380,16 @@ public void testNoServerTimingHeader() throws IOException, InterruptedException Attributes expectedAttributes = expectedCommonBaseAttributes.toBuilder() - .putAll(expectedCommonRequestAttributes) .put(BuiltInMetricsConstant.STATUS_KEY, "OK") .put(BuiltInMetricsConstant.METHOD_KEY, "Spanner.ExecuteSql") .build(); assertFalse(checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_LATENCIES_NAME)); assertFalse(checkIfMetricExists(metricReader, BuiltInMetricsConstant.GFE_LATENCIES_NAME)); + assertTrue( + checkIfMetricExists(metricReader, BuiltInMetricsConstant.GFE_CONNECTIVITY_ERROR_NAME)); + assertTrue( + checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME)); spannerNoHeader.close(); serverNoHeader.shutdown(); diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GfeLatencyTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GfeLatencyTest.java index 5067eb09ff9..2f75a251fec 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GfeLatencyTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/GfeLatencyTest.java @@ -17,8 +17,7 @@ package com.google.cloud.spanner.spi.v1; import static com.google.cloud.spanner.DisableDefaultMtlsProvider.disableDefaultMtlsProvider; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.*; import com.google.auth.oauth2.AccessToken; import com.google.auth.oauth2.OAuth2Credentials; @@ -55,10 +54,7 @@ import java.util.Random; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import org.junit.After; -import org.junit.AfterClass; -import org.junit.BeforeClass; -import org.junit.Test; +import org.junit.*; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -259,7 +255,7 @@ public void testGfeMissingHeaderCountExecuteStreamingSql() throws InterruptedExc SpannerRpcViews.SPANNER_GFE_HEADER_MISSING_COUNT_VIEW, "google.spanner.v1.Spanner/ExecuteStreamingSql", true); - assertEquals(1, count1); + assertTrue(count1 >= 1); } @Test @@ -270,7 +266,7 @@ public void testGfeMissingHeaderExecuteSql() throws InterruptedException { long count = getMetric( SpannerRpcViews.SPANNER_GFE_HEADER_MISSING_COUNT_VIEW, - "google.spanner.v1.Spanner/ExecuteSql", + "google.spanner.v1.Spanner/Commit", false); assertEquals(0, count); @@ -280,7 +276,7 @@ public void testGfeMissingHeaderExecuteSql() throws InterruptedException { long count1 = getMetric( SpannerRpcViews.SPANNER_GFE_HEADER_MISSING_COUNT_VIEW, - "google.spanner.v1.Spanner/ExecuteSql", + "google.spanner.v1.Spanner/Commit", true); assertEquals(1, count1); } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/SpannerRpcMetricsTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/SpannerRpcMetricsTest.java index 89c874fe204..95faea16f62 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/SpannerRpcMetricsTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/spi/v1/SpannerRpcMetricsTest.java @@ -50,10 +50,7 @@ import java.util.Random; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; -import org.junit.After; -import org.junit.AfterClass; -import org.junit.BeforeClass; -import org.junit.Test; +import org.junit.*; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -73,6 +70,7 @@ public class SpannerRpcMetricsTest { private static DatabaseClient databaseClientNoHeader; private static String instanceId = "fake-instance"; private static String databaseId = "fake-database"; + private static String noHeaderdatabaseId = "fake-database-1"; private static String projectId = "fake-project"; private static AtomicInteger fakeServerTiming = new AtomicInteger(new Random().nextInt(1000) + 1); private static final Statement SELECT1AND2 = @@ -183,7 +181,7 @@ public void sendHeaders(Metadata headers) { createSpannerOptions(addressNoHeader, serverNoHeader).getService(); databaseClientNoHeader = spannerNoHeaderNoOpenTelemetry.getDatabaseClient( - DatabaseId.of(projectId, instanceId, databaseId)); + DatabaseId.of(projectId, instanceId, noHeaderdatabaseId)); } @AfterClass @@ -228,7 +226,8 @@ public void testGfeMissingHeaderExecuteSqlWithGlobalOpenTelemetry() throws Inter long count = getHeaderLatencyMetric( getMetricData("spanner/gfe_header_missing_count", inMemoryMetricReaderInjected), - "google.spanner.v1.Spanner/ExecuteSql"); + "google.spanner.v1.Spanner/Commit", + databaseId); assertEquals(0, count); databaseClientNoHeader @@ -237,7 +236,8 @@ public void testGfeMissingHeaderExecuteSqlWithGlobalOpenTelemetry() throws Inter long count1 = getHeaderLatencyMetric( getMetricData("spanner/gfe_header_missing_count", inMemoryMetricReader), - "google.spanner.v1.Spanner/ExecuteSql"); + "google.spanner.v1.Spanner/Commit", + noHeaderdatabaseId); assertEquals(1, count1); } @@ -273,9 +273,12 @@ private static SpannerOptions createSpannerOptions(InetSocketAddress address, Se .build(); } - private long getHeaderLatencyMetric(MetricData metricData, String methodName) { + private long getHeaderLatencyMetric(MetricData metricData, String methodName, String databaseId) { return metricData.getLongSumData().getPoints().stream() - .filter(x -> x.getAttributes().asMap().containsValue(methodName)) + .filter( + x -> + x.getAttributes().asMap().containsValue(methodName) + && x.getAttributes().asMap().containsValue(databaseId)) .findFirst() .get() .getValue(); From 05448e85ff24d929c0002a71cc8b42511c3f2682 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Knut=20Olav=20L=C3=B8ite?= Date: Wed, 10 Dec 2025 17:00:35 +0100 Subject: [PATCH 3/3] fix: retry as PDML dit not retry Resource limit exceeded (#4258) Most transactions that exceed the mutation limit for an atomic transaction will fail with the error "The transaction contains too many mutations.". However, it is also possible that the transaction fails with the more generic error message "Transaction resource limits exceeded". This error did not trigger a retry of the statement using a PDML transaction. Fixes #4253 --- ...sactionMutationLimitExceededException.java | 14 +++++- ...etryDmlAsPartitionedDmlMockServerTest.java | 45 ++++++++++++++++--- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionMutationLimitExceededException.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionMutationLimitExceededException.java index c89fedd7fe3..f6b0a4efd22 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionMutationLimitExceededException.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionMutationLimitExceededException.java @@ -28,6 +28,9 @@ public class TransactionMutationLimitExceededException extends SpannerException private static final String ERROR_MESSAGE = "The transaction contains too many mutations."; + private static final String TRANSACTION_RESOURCE_LIMIT_EXCEEDED_MESSAGE = + "Transaction resource limits exceeded"; + /** Private constructor. Use {@link SpannerExceptionFactory} to create instances. */ TransactionMutationLimitExceededException( DoNotConstructDirectly token, @@ -40,13 +43,17 @@ public class TransactionMutationLimitExceededException extends SpannerException } static boolean isTransactionMutationLimitException(ErrorCode code, String message) { - return code == ErrorCode.INVALID_ARGUMENT && message != null && message.contains(ERROR_MESSAGE); + return code == ErrorCode.INVALID_ARGUMENT + && message != null + && (message.contains(ERROR_MESSAGE) + || message.contains(TRANSACTION_RESOURCE_LIMIT_EXCEEDED_MESSAGE)); } static boolean isTransactionMutationLimitException(Throwable cause, ApiException apiException) { if (cause == null || cause.getMessage() == null - || !cause.getMessage().contains(ERROR_MESSAGE)) { + || !(cause.getMessage().contains(ERROR_MESSAGE) + || cause.getMessage().contains(TRANSACTION_RESOURCE_LIMIT_EXCEEDED_MESSAGE))) { return false; } // Spanner includes a hint that points to the Spanner limits documentation page when the error @@ -66,6 +73,9 @@ static boolean isTransactionMutationLimitException(Throwable cause, ApiException .getLinks(0) .getUrl() .equals("https://cloud.google.com/spanner/docs/limits"); + } else if (cause.getMessage().contains(TRANSACTION_RESOURCE_LIMIT_EXCEEDED_MESSAGE)) { + // This more generic error does not contain any additional details. + return true; } return false; } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/RetryDmlAsPartitionedDmlMockServerTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/RetryDmlAsPartitionedDmlMockServerTest.java index cce6b7fe083..d5e44fdcefc 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/RetryDmlAsPartitionedDmlMockServerTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/RetryDmlAsPartitionedDmlMockServerTest.java @@ -43,10 +43,37 @@ import io.grpc.StatusRuntimeException; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; -@RunWith(JUnit4.class) +@RunWith(Parameterized.class) public class RetryDmlAsPartitionedDmlMockServerTest extends AbstractMockServerTest { + private enum ExceptionType { + MutationLimitExceeded { + @Override + StatusRuntimeException createException() { + return createTransactionMutationLimitExceededException(); + } + }, + ResourceLimitExceeded { + @Override + StatusRuntimeException createException() { + return createTransactionResourceLimitExceededException(); + } + }; + + abstract StatusRuntimeException createException(); + } + + @Parameters(name = "exception = {0}") + public static Object[] data() { + return ExceptionType.values(); + } + + @SuppressWarnings("ClassEscapesDefinedScope") + @Parameter + public ExceptionType exceptionType; static StatusRuntimeException createTransactionMutationLimitExceededException() { Metadata.Key key = @@ -70,10 +97,16 @@ static StatusRuntimeException createTransactionMutationLimitExceededException() .asRuntimeException(trailers); } + static StatusRuntimeException createTransactionResourceLimitExceededException() { + return Status.INVALID_ARGUMENT + .withDescription("Transaction resource limits exceeded") + .asRuntimeException(); + } + @Test public void testTransactionMutationLimitExceeded_isNotRetriedByDefault() { mockSpanner.setExecuteSqlExecutionTime( - SimulatedExecutionTime.ofException(createTransactionMutationLimitExceededException())); + SimulatedExecutionTime.ofException(exceptionType.createException())); try (Connection connection = createConnection()) { connection.setAutocommit(true); @@ -95,7 +128,7 @@ public void testTransactionMutationLimitExceeded_isNotRetriedByDefault() { public void testTransactionMutationLimitExceeded_canBeRetriedAsPDML() { Statement statement = Statement.of("update test set value=1 where true"); mockSpanner.setExecuteSqlExecutionTime( - SimulatedExecutionTime.ofException(createTransactionMutationLimitExceededException())); + SimulatedExecutionTime.ofException(exceptionType.createException())); mockSpanner.putStatementResult( MockSpannerServiceImpl.StatementResult.update(statement, 100000L)); @@ -134,7 +167,7 @@ public void testTransactionMutationLimitExceeded_retryAsPDMLFails() { Statement statement = Statement.of("insert into test (id, value) select -id, value from test"); // The transactional update statement uses ExecuteSql(..). mockSpanner.setExecuteSqlExecutionTime( - SimulatedExecutionTime.ofException(createTransactionMutationLimitExceededException())); + SimulatedExecutionTime.ofException(exceptionType.createException())); mockSpanner.putStatementResult( MockSpannerServiceImpl.StatementResult.exception( statement, @@ -230,7 +263,7 @@ public void testTransactionMutationLimitExceeded_isWrappedAsCauseOfBatchUpdateEx Statement statement = Statement.of(sql); mockSpanner.putStatementResult( MockSpannerServiceImpl.StatementResult.exception( - statement, createTransactionMutationLimitExceededException())); + statement, exceptionType.createException())); try (Connection connection = createConnection()) { connection.setAutocommit(true);