From 3117484e162105dec2bc2ad69fc9e2e413bee5f0 Mon Sep 17 00:00:00 2001 From: surbhigarg92 Date: Fri, 16 Aug 2024 15:16:32 +0530 Subject: [PATCH] review comments --- .../BuiltInOpenTelemetryMetricsProvider.java | 5 ++-- .../google/cloud/spanner/SpannerOptions.java | 6 ++-- .../cloud/spanner/spi/v1/GapicSpannerRpc.java | 29 ++++++++++++------- ...OpenTelemetryBuiltInMetricsTracerTest.java | 8 ++--- .../spanner/it/ITBuiltInMetricsTest.java | 11 ------- 5 files changed, 28 insertions(+), 31 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInOpenTelemetryMetricsProvider.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInOpenTelemetryMetricsProvider.java index b006938bede..58f98db22c7 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInOpenTelemetryMetricsProvider.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInOpenTelemetryMetricsProvider.java @@ -46,8 +46,7 @@ final class BuiltInOpenTelemetryMetricsProvider { - public static BuiltInOpenTelemetryMetricsProvider INSTANCE = - new BuiltInOpenTelemetryMetricsProvider(); + static BuiltInOpenTelemetryMetricsProvider INSTANCE = new BuiltInOpenTelemetryMetricsProvider(); private static final Logger logger = Logger.getLogger(BuiltInOpenTelemetryMetricsProvider.class.getName()); @@ -77,7 +76,7 @@ OpenTelemetry getOrCreateOpenTelemetry(String projectId, @Nullable Credentials c } } - Map getClientAttributes( + Map createClientAttributes( String projectId, boolean isDirectPathChannelCreated, String client_name) { Map clientAttributes = new HashMap<>(); clientAttributes.put(LOCATION_ID_KEY.getKey(), detectClientLocation()); 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 965c0f4e008..cce615d172a 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 @@ -1674,7 +1674,7 @@ private ApiTracerFactory createApiTracerFactory( // and if emulator is not enabled. if (isEnableBuiltInMetrics() && !isAdminClient && !isEmulatorEnabled) { ApiTracerFactory metricsTracerFactory = - getMetricsApiTracerFactory(isDirectPathChannelCreated); + createMetricsApiTracerFactory(isDirectPathChannelCreated); if (metricsTracerFactory != null) { apiTracerFactories.add(metricsTracerFactory); } @@ -1699,7 +1699,7 @@ private ApiTracerFactory getDefaultApiTracerFactory() { return BaseApiTracerFactory.getInstance(); } - private ApiTracerFactory getMetricsApiTracerFactory(boolean isDirectPathChannelCreated) { + private ApiTracerFactory createMetricsApiTracerFactory(boolean isDirectPathChannelCreated) { OpenTelemetry openTelemetry = this.builtInOpenTelemetryMetricsProvider.getOrCreateOpenTelemetry( getDefaultProjectId(), getCredentials()); @@ -1707,7 +1707,7 @@ private ApiTracerFactory getMetricsApiTracerFactory(boolean isDirectPathChannelC return openTelemetry != null ? new MetricsTracerFactory( new OpenTelemetryMetricsRecorder(openTelemetry, BuiltInMetricsConstant.METER_NAME), - builtInOpenTelemetryMetricsProvider.getClientAttributes( + builtInOpenTelemetryMetricsProvider.createClientAttributes( getDefaultProjectId(), isDirectPathChannelCreated, "spanner-java/" + GaxProperties.getLibraryVersion(getClass()))) 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 b0c5ed78c7d..eaa7f4529a8 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 @@ -366,16 +366,19 @@ public GapicSpannerRpc(final SpannerOptions options) { // If it is enabled in options uses the channel pool provided by the gRPC-GCP extension. maybeEnableGrpcGcpExtension(defaultChannelProviderBuilder, options); - InstantiatingGrpcChannelProvider defaultChannelProvider = - defaultChannelProviderBuilder.build(); + InstantiatingGrpcChannelProvider defaultChannelProvider = null; + boolean isDirectPathChannelCreated = false; + + if (options.getChannelProvider() == null) { + defaultChannelProvider = defaultChannelProviderBuilder.build(); + isDirectPathChannelCreated = + defaultChannelProvider.canUseDirectPath() + && defaultChannelProvider.isDirectPathXdsEnabled(); + } TransportChannelProvider channelProvider = MoreObjects.firstNonNull(options.getChannelProvider(), defaultChannelProvider); - boolean isDirectPathChannelCreated = - defaultChannelProvider.canUseDirectPath() - && defaultChannelProvider.isDirectPathXdsEnabled(); - CredentialsProvider credentialsProvider = GrpcTransportOptions.setUpCredentialsProvider(options); @@ -405,7 +408,7 @@ public GapicSpannerRpc(final SpannerOptions options) { .setTracerFactory( options.getApiTracerFactory( isDirectPathChannelCreated, - false, + /* isAdminClient = */ false, isEmulatorEnabled(options, emulatorHost))) .build()); this.readRetrySettings = @@ -436,7 +439,9 @@ public GapicSpannerRpc(final SpannerOptions options) { .setStreamWatchdogProvider(watchdogProvider) .setTracerFactory( options.getApiTracerFactory( - isDirectPathChannelCreated, false, isEmulatorEnabled(options, emulatorHost))) + isDirectPathChannelCreated, + /* isAdminClient = */ false, + isEmulatorEnabled(options, emulatorHost))) .executeSqlSettings() .setRetrySettings(partitionedDmlRetrySettings); pdmlSettings.executeStreamingSqlSettings().setRetrySettings(partitionedDmlRetrySettings); @@ -465,7 +470,9 @@ isDirectPathChannelCreated, false, isEmulatorEnabled(options, emulatorHost))) .setStreamWatchdogProvider(watchdogProvider) .setTracerFactory( options.getApiTracerFactory( - isDirectPathChannelCreated, true, isEmulatorEnabled(options, emulatorHost))) + isDirectPathChannelCreated, + /* isAdminClient = */ true, + isEmulatorEnabled(options, emulatorHost))) .build(); this.instanceAdminStub = GrpcInstanceAdminStub.create(instanceAdminStubSettings); @@ -478,7 +485,9 @@ isDirectPathChannelCreated, true, isEmulatorEnabled(options, emulatorHost))) .setStreamWatchdogProvider(watchdogProvider) .setTracerFactory( options.getApiTracerFactory( - isDirectPathChannelCreated, true, isEmulatorEnabled(options, emulatorHost))) + isDirectPathChannelCreated, + /* isAdminClient = */ true, + isEmulatorEnabled(options, emulatorHost))) .build(); // Automatically retry RESOURCE_EXHAUSTED for GetOperation if auto-throttling of 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 791e87ec2b9..7bfdb99ca90 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 @@ -88,7 +88,7 @@ public static void setup() { String client_name = "spanner-java/"; openTelemetry = OpenTelemetrySdk.builder().setMeterProvider(meterProvider.build()).build(); - attributes = provider.getClientAttributes("test-project", true, client_name); + attributes = provider.createClientAttributes("test-project", true, client_name); expectedBaseAttributes = Attributes.builder() @@ -240,7 +240,7 @@ private MetricData getMetricData(InMemoryMetricReader reader, String metricName) Collection allMetricData = Collections.emptyList(); // Fetch the MetricData with retries - for (int attemptsLeft = 10; attemptsLeft > 0; attemptsLeft--) { + for (int attemptsLeft = 1000; attemptsLeft > 0; attemptsLeft--) { allMetricData = reader.collectAllMetrics(); List matchingMetadata = allMetricData.stream() @@ -257,14 +257,14 @@ private MetricData getMetricData(InMemoryMetricReader reader, String metricName) } try { - Thread.sleep(100); + Thread.sleep(1); } catch (InterruptedException interruptedException) { Thread.currentThread().interrupt(); throw new RuntimeException(interruptedException); } } - assertFalse(String.format("MetricData is missing for metric {0}", fullMetricName), false); + assertTrue(String.format("MetricData is missing for metric {0}", fullMetricName), false); return null; } diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITBuiltInMetricsTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITBuiltInMetricsTest.java index d2624e97ab1..dfcc0731161 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITBuiltInMetricsTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITBuiltInMetricsTest.java @@ -18,7 +18,6 @@ import static com.google.common.truth.Truth.assertWithMessage; -import com.google.api.gax.longrunning.OperationFuture; import com.google.cloud.monitoring.v3.MetricServiceClient; import com.google.cloud.spanner.Database; import com.google.cloud.spanner.DatabaseClient; @@ -31,9 +30,7 @@ import com.google.monitoring.v3.ProjectName; import com.google.monitoring.v3.TimeInterval; import com.google.protobuf.util.Timestamps; -import com.google.spanner.admin.database.v1.UpdateDatabaseDdlMetadata; import java.io.IOException; -import java.util.Collections; import java.util.concurrent.TimeUnit; import org.junit.BeforeClass; import org.junit.ClassRule; @@ -78,14 +75,6 @@ public void testBuiltinMetricsWithDefaultOTEL() throws Exception { .setStartTime(Timestamps.fromMillis(start.toEpochMilli())) .setEndTime(Timestamps.fromMillis(end.toEpochMilli())) .build(); - String ddl = - "CREATE TABLE FOO (" - + " K STRING(MAX) NOT NULL," - + " V INT64," - + ") PRIMARY KEY (K)"; - OperationFuture op = - db.updateDdl(Collections.singletonList(ddl), null); - op.get(); client .readWriteTransaction()