diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java index 0995c478427..a25e8bfa997 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java @@ -923,8 +923,16 @@ public boolean isEnableBuiltInMetrics() { @Override public boolean isEnableGRPCBuiltInMetrics() { - return "false" - .equalsIgnoreCase(System.getenv(SPANNER_DISABLE_DIRECT_ACCESS_GRPC_BUILTIN_METRICS)); + // Enable gRPC built-in metrics if: + // 1. The env var SPANNER_DISABLE_DIRECT_ACCESS_GRPC_BUILTIN_METRICS is explicitly set to + // "false", OR + // 2. DirectPath is enabled AND the env var is not set to "true" + // This allows metrics to be enabled by default when DirectPath is on, unless explicitly + // disabled via env. + String grpcDisableEnv = System.getenv("SPANNER_DISABLE_DIRECT_ACCESS_GRPC_BUILTIN_METRICS"); + boolean isDirectPathEnabled = GapicSpannerRpc.isEnableDirectPathXdsEnv(); + return ("false".equalsIgnoreCase(grpcDisableEnv)) + || (isDirectPathEnabled && !"true".equalsIgnoreCase(grpcDisableEnv)); } @Override diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java index fa5719c95c4..c43bbe1f11b 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/GapicSpannerRpc.java @@ -367,8 +367,7 @@ public GapicSpannerRpc(final SpannerOptions options) { .withEncoding(compressorName)) .setHeaderProvider(headerProviderWithUserAgent) .setAllowNonDefaultServiceAccount(true); - String directPathXdsEnv = System.getenv("GOOGLE_SPANNER_ENABLE_DIRECT_ACCESS"); - boolean isAttemptDirectPathXds = Boolean.parseBoolean(directPathXdsEnv); + boolean isAttemptDirectPathXds = isEnableDirectPathXdsEnv(); if (isAttemptDirectPathXds) { defaultChannelProviderBuilder.setAttemptDirectPath(true); defaultChannelProviderBuilder.setAttemptDirectPathXds(); @@ -678,7 +677,19 @@ private static boolean isEmulatorEnabled(SpannerOptions options, String emulator } public static boolean isEnableAFEServerTiming() { - return "false".equalsIgnoreCase(System.getenv("SPANNER_DISABLE_AFE_SERVER_TIMING")); + // Enable AFE metrics and add AFE header if: + // 1. The env var SPANNER_DISABLE_AFE_SERVER_TIMING is explicitly set to "false", OR + // 2. DirectPath is enabled AND the env var is not set to "true" + // This allows metrics to be enabled by default when DirectPath is on, unless explicitly + // disabled via env. + String afeDisableEnv = System.getenv("SPANNER_DISABLE_AFE_SERVER_TIMING"); + boolean isDirectPathEnabled = isEnableDirectPathXdsEnv(); + return ("false".equalsIgnoreCase(afeDisableEnv)) + || (isDirectPathEnabled && !"true".equalsIgnoreCase(afeDisableEnv)); + } + + public static boolean isEnableDirectPathXdsEnv() { + return Boolean.parseBoolean(System.getenv("GOOGLE_SPANNER_ENABLE_DIRECT_ACCESS")); } private static final RetrySettings ADMIN_REQUESTS_LIMIT_EXCEEDED_RETRY_SETTINGS = 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 32e6f2c3b96..e3bc848dc1e 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 @@ -133,7 +133,8 @@ public void onHeaders(Metadata metadata) { Boolean isDirectPathUsed = isDirectPathUsed(getAttributes().get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR)); addDirectPathUsedAttribute(compositeTracer, isDirectPathUsed); - processHeader(metadata, tagContext, attributes, span, compositeTracer); + processHeader( + metadata, tagContext, attributes, span, compositeTracer, isDirectPathUsed); super.onHeaders(metadata); } }, @@ -151,7 +152,8 @@ private void processHeader( TagContext tagContext, Attributes attributes, Span span, - CompositeTracer compositeTracer) { + CompositeTracer compositeTracer, + boolean isDirectPathUsed) { MeasureMap measureMap = STATS_RECORDER.newMeasureMap(); String serverTiming = metadata.get(SERVER_TIMING_HEADER_KEY); try { @@ -172,7 +174,7 @@ private void processHeader( spannerRpcMetrics.recordGfeLatency((long) gfeLatency, attributes); spannerRpcMetrics.recordGfeHeaderMissingCount(0L, attributes); - if (compositeTracer != null) { + if (compositeTracer != null && !isDirectPathUsed) { compositeTracer.recordGFELatency(gfeLatency); } if (span != null) { @@ -181,7 +183,7 @@ private void processHeader( } else { measureMap.put(SPANNER_GFE_HEADER_MISSING_COUNT, 1L).record(tagContext); spannerRpcMetrics.recordGfeHeaderMissingCount(1L, attributes); - if (compositeTracer != null) { + if (compositeTracer != null && !isDirectPathUsed) { compositeTracer.recordGfeHeaderMissingCount(1L); } } 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 acc94ea56e5..55c5cf47714 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 @@ -34,6 +34,7 @@ import com.google.cloud.spanner.MockSpannerServiceImpl.SimulatedExecutionTime; import com.google.cloud.spanner.MockSpannerServiceImpl.StatementResult; import com.google.cloud.spanner.connection.RandomResultSetGenerator; +import com.google.cloud.spanner.spi.v1.GapicSpannerRpc; import com.google.common.base.Stopwatch; import com.google.common.collect.ImmutableList; import com.google.common.collect.Range; @@ -190,16 +191,23 @@ public void testMetricsSingleUseQuery() { assertNotNull(attemptCountMetricData); assertThat(getAggregatedValue(attemptCountMetricData, expectedAttributes)).isEqualTo(1); - MetricData gfeLatencyMetricData = - getMetricData(metricReader, BuiltInMetricsConstant.GFE_LATENCIES_NAME); - double gfeLatencyValue = getAggregatedValue(gfeLatencyMetricData, expectedAttributes); - assertEquals(fakeServerTiming.get(), gfeLatencyValue, 0); - assertFalse( checkIfMetricExists(metricReader, BuiltInMetricsConstant.GFE_CONNECTIVITY_ERROR_NAME)); - assertFalse(checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_LATENCIES_NAME)); assertFalse( checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME)); + if (GapicSpannerRpc.isEnableDirectPathXdsEnv()) { + // AFE metrics are enabled for DirectPath. + MetricData afeLatencyMetricData = + getMetricData(metricReader, BuiltInMetricsConstant.AFE_LATENCIES_NAME); + double afeLatencyValue = getAggregatedValue(afeLatencyMetricData, expectedAttributes); + assertEquals(fakeAFEServerTiming.get(), afeLatencyValue, 1e-6); + } else { + MetricData gfeLatencyMetricData = + getMetricData(metricReader, BuiltInMetricsConstant.GFE_LATENCIES_NAME); + double gfeLatencyValue = getAggregatedValue(gfeLatencyMetricData, expectedAttributes); + assertEquals(fakeServerTiming.get(), gfeLatencyValue, 1e-6); + assertFalse(checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_LATENCIES_NAME)); + } } private boolean isJava8() { @@ -261,20 +269,19 @@ public void testMetricsSingleUseQueryWithAfeEnabled() throws Exception { assertNotNull(attemptCountMetricData); assertThat(getAggregatedValue(attemptCountMetricData, expectedAttributes)).isEqualTo(1); - MetricData gfeLatencyMetricData = - getMetricData(metricReader, BuiltInMetricsConstant.GFE_LATENCIES_NAME); - double gfeLatencyValue = getAggregatedValue(gfeLatencyMetricData, expectedAttributes); - assertEquals(fakeServerTiming.get(), gfeLatencyValue, 0); - assertFalse( checkIfMetricExists(metricReader, BuiltInMetricsConstant.GFE_CONNECTIVITY_ERROR_NAME)); - + assertFalse( + checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME)); MetricData afeLatencyMetricData = getMetricData(metricReader, BuiltInMetricsConstant.AFE_LATENCIES_NAME); double afeLatencyValue = getAggregatedValue(afeLatencyMetricData, expectedAttributes); - assertEquals(fakeAFEServerTiming.get(), afeLatencyValue, 0); - assertFalse( - checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME)); + assertEquals(fakeAFEServerTiming.get(), afeLatencyValue, 1e-6); + + MetricData gfeLatencyMetricData = + getMetricData(metricReader, BuiltInMetricsConstant.GFE_LATENCIES_NAME); + double gfeLatencyValue = getAggregatedValue(gfeLatencyMetricData, expectedAttributes); + assertEquals(fakeServerTiming.get(), gfeLatencyValue, 1e-6); } finally { writeableEnvironmentVariables.remove("GOOGLE_CLOUD_SPANNER_MULTIPLEXED_SESSIONS"); } @@ -445,13 +452,20 @@ public void testNoServerTimingHeader() throws IOException, InterruptedException .put(BuiltInMetricsConstant.METHOD_KEY, "Spanner.ExecuteSql") .build(); - MetricData gfeConnectivityMetricData = - getMetricData(metricReader, BuiltInMetricsConstant.GFE_CONNECTIVITY_ERROR_NAME); - assertThat(getAggregatedValue(gfeConnectivityMetricData, expectedAttributes)).isEqualTo(1); assertFalse(checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_LATENCIES_NAME)); assertFalse(checkIfMetricExists(metricReader, BuiltInMetricsConstant.GFE_LATENCIES_NAME)); - assertFalse( - checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME)); + if (GapicSpannerRpc.isEnableDirectPathXdsEnv()) { + MetricData afeConnectivityMetricData = + getMetricData(metricReader, BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME); + assertThat(getAggregatedValue(afeConnectivityMetricData, expectedAttributes)).isEqualTo(1); + } else { + MetricData gfeConnectivityMetricData = + getMetricData(metricReader, BuiltInMetricsConstant.GFE_CONNECTIVITY_ERROR_NAME); + assertThat(getAggregatedValue(gfeConnectivityMetricData, expectedAttributes)).isEqualTo(1); + assertFalse( + checkIfMetricExists(metricReader, BuiltInMetricsConstant.AFE_CONNECTIVITY_ERROR_NAME)); + } + spannerNoHeader.close(); serverNoHeader.shutdown(); serverNoHeader.awaitTermination();