review

Author: surbhigarg92

google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsConstant.java

@@ -51,7 +51,8 @@ public class BuiltInMetricsConstant {
           "grpc.lb.rls.default_target_picks",
           "grpc.lb.rls.target_picks",
           "grpc.xds_client.server_failure",
-          "grpc.xds_client.resource_updates_invalid");
+          "grpc.xds_client.resource_updates_invalid",
+          "grpc.xds_client.resource_updates_valid");
 
   public static final String SPANNER_RESOURCE_TYPE = "spanner_instance_client";
 

google-cloud-spanner/src/main/java/com/google/cloud/spanner/BuiltInMetricsProvider.java

@@ -75,7 +75,6 @@ OpenTelemetry getOrCreateOpenTelemetry(
         BuiltInMetricsView.registerBuiltinMetrics(
             SpannerCloudMonitoringExporter.create(projectId, credentials, monitoringHost),
             sdkMeterProviderBuilder);
-
         sdkMeterProviderBuilder.setResource(Resource.create(createResourceAttributes(projectId)));
         SdkMeterProvider sdkMeterProvider = sdkMeterProviderBuilder.build();
         this.openTelemetry = OpenTelemetrySdk.builder().setMeterProvider(sdkMeterProvider).build();
@@ -91,7 +90,7 @@ OpenTelemetry getOrCreateOpenTelemetry(
     }
   }
 
-  public void enableGrpcMetrics(
+  void enableGrpcMetrics(
       InstantiatingGrpcChannelProvider.Builder channelProviderBuilder,
       String projectId,
       @Nullable Credentials credentials,

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerCloudMonitoringExporter.java

@@ -37,7 +37,6 @@
 import io.opentelemetry.sdk.metrics.InstrumentType;
 import io.opentelemetry.sdk.metrics.data.AggregationTemporality;
 import io.opentelemetry.sdk.metrics.data.MetricData;
-import io.opentelemetry.sdk.metrics.data.PointData;
 import io.opentelemetry.sdk.metrics.export.MetricExporter;
 import io.opentelemetry.sdk.resources.Resource;
 import java.io.IOException;
@@ -113,7 +112,7 @@ public CompletableResultCode export(@Nonnull Collection<MetricData> collection)
 
   /** Export client built in metrics */
   private CompletableResultCode exportSpannerClientMetrics(Collection<MetricData> collection) {
-    // Filter spanner metrics. Only include metrics that contain a project and instance ID.
+    // Filter spanner metrics. Only include metrics that contain a valid project.
     List<MetricData> spannerMetricData = collection.stream().collect(Collectors.toList());
 
     // Log warnings for metrics that will be skipped.
@@ -125,12 +124,7 @@ private CompletableResultCode exportSpannerClientMetrics(Collection<MetricData>
           Level.WARNING, "Some metric data contain a different projectId. These will be skipped.");
       mustFilter = true;
     }
-    if (spannerMetricData.stream()
-        .flatMap(metricData -> metricData.getData().getPoints().stream())
-        .anyMatch(this::shouldSkipPointDataDueToMissingInstanceId)) {
-      logger.log(Level.WARNING, "Some metric data miss instanceId. These will be skipped.");
-      mustFilter = true;
-    }
+
     if (mustFilter) {
       spannerMetricData =
           spannerMetricData.stream()
@@ -194,19 +188,13 @@ public void onSuccess(List<Empty> empty) {
   }
 
   private boolean shouldSkipMetricData(MetricData metricData) {
-    return shouldSkipPointDataDueToProjectId(metricData.getResource())
-        || metricData.getData().getPoints().stream()
-            .anyMatch(pd -> shouldSkipPointDataDueToMissingInstanceId(pd));
+    return shouldSkipPointDataDueToProjectId(metricData.getResource());
   }
 
   private boolean shouldSkipPointDataDueToProjectId(Resource resource) {
     return !spannerProjectId.equals(SpannerCloudMonitoringExporterUtils.getProjectId(resource));
   }
 
-  private boolean shouldSkipPointDataDueToMissingInstanceId(PointData pointData) {
-    return SpannerCloudMonitoringExporterUtils.getInstanceId(pointData) == null;
-  }
-
   boolean lastExportSkippedData() {
     return this.lastExportSkippedData.get();
   }

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerCloudMonitoringExporterUtils.java

@@ -24,7 +24,6 @@
 import static com.google.api.MetricDescriptor.ValueType.INT64;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.GAX_METER_NAME;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.GRPC_METER_NAME;
-import static com.google.cloud.spanner.BuiltInMetricsConstant.INSTANCE_ID_KEY;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.PROJECT_ID_KEY;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.SPANNER_METER_NAME;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.SPANNER_PROMOTED_RESOURCE_LABELS;
@@ -70,10 +69,6 @@ static String getProjectId(Resource resource) {
     return resource.getAttributes().get(PROJECT_ID_KEY);
   }
 
-  static String getInstanceId(PointData pointData) {
-    return pointData.getAttributes().get(INSTANCE_ID_KEY);
-  }
-
   static List<TimeSeries> convertToSpannerTimeSeries(List<MetricData> collection) {
     List<TimeSeries> allTimeSeries = new ArrayList<>();
 

google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerCloudMonitoringExporterTest.java

@@ -16,7 +16,6 @@
 
 package com.google.cloud.spanner;
 
-import static com.google.cloud.spanner.BuiltInMetricsConstant.ATTEMPT_COUNT_NAME;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.CLIENT_HASH_KEY;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.CLIENT_NAME_KEY;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.CLIENT_UID_KEY;
@@ -32,7 +31,6 @@
 import static com.google.cloud.spanner.BuiltInMetricsConstant.PROJECT_ID_KEY;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -47,7 +45,6 @@
 import com.google.monitoring.v3.TimeSeries;
 import com.google.protobuf.Empty;
 import io.opentelemetry.api.common.Attributes;
-import io.opentelemetry.sdk.common.CompletableResultCode;
 import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
 import io.opentelemetry.sdk.metrics.InstrumentType;
 import io.opentelemetry.sdk.metrics.data.AggregationTemporality;
@@ -358,56 +355,6 @@ public void getAggregationTemporality() throws IOException {
         .isEqualTo(AggregationTemporality.CUMULATIVE);
   }
 
-  @Test
-  public void testSkipExportingDataIfMissingInstanceId() throws IOException {
-    Attributes attributesWithoutInstanceId =
-        Attributes.builder().putAll(attributes).remove(INSTANCE_ID_KEY).build();
-
-    SpannerCloudMonitoringExporter actualExporter =
-        SpannerCloudMonitoringExporter.create(projectId, null, null);
-    assertThat(actualExporter.getAggregationTemporality(InstrumentType.COUNTER))
-        .isEqualTo(AggregationTemporality.CUMULATIVE);
-    ArgumentCaptor<CreateTimeSeriesRequest> argumentCaptor =
-        ArgumentCaptor.forClass(CreateTimeSeriesRequest.class);
-
-    UnaryCallable<CreateTimeSeriesRequest, Empty> mockCallable = Mockito.mock(UnaryCallable.class);
-    Mockito.when(mockMetricServiceStub.createServiceTimeSeriesCallable()).thenReturn(mockCallable);
-    ApiFuture<Empty> future = ApiFutures.immediateFuture(Empty.getDefaultInstance());
-    Mockito.when(mockCallable.futureCall(argumentCaptor.capture())).thenReturn(future);
-
-    long fakeValue = 11L;
-
-    long startEpoch = 10;
-    long endEpoch = 15;
-    LongPointData longPointData =
-        ImmutableLongPointData.create(startEpoch, endEpoch, attributesWithoutInstanceId, fakeValue);
-
-    MetricData operationLongData =
-        ImmutableMetricData.createLongSum(
-            resource,
-            scope,
-            "spanner.googleapis.com/internal/client/" + OPERATION_COUNT_NAME,
-            "description",
-            "1",
-            ImmutableSumData.create(
-                true, AggregationTemporality.CUMULATIVE, ImmutableList.of(longPointData)));
-
-    MetricData attemptLongData =
-        ImmutableMetricData.createLongSum(
-            resource,
-            scope,
-            "spanner.googleapis.com/internal/client/" + ATTEMPT_COUNT_NAME,
-            "description",
-            "1",
-            ImmutableSumData.create(
-                true, AggregationTemporality.CUMULATIVE, ImmutableList.of(longPointData)));
-
-    CompletableResultCode resultCode =
-        exporter.export(Arrays.asList(operationLongData, attemptLongData));
-    assertTrue(resultCode.isSuccess());
-    assertTrue(exporter.lastExportSkippedData());
-  }
-
   private static class FakeMetricServiceClient extends MetricServiceClient {
 
     protected FakeMetricServiceClient(MetricServiceStub stub) {