fix: Client built in metrics. Skip export if instance id is null (#3447)

* fix: skip instance id, and public doc * review comments * revert:review comments
googleapis · Nov 6, 2024 · 8b2e5ef · 8b2e5ef
1 parent 07b777d
commit 8b2e5ef
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 1 deletion.
diff --git a/...-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerCloudMonitoringExporter.java b/...-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerCloudMonitoringExporter.java
@@ -138,6 +138,14 @@ private CompletableResultCode exportSpannerClientMetrics(Collection<MetricData>
       return CompletableResultCode.ofFailure();
     }
 
+    // Verifies if metrics data has missing instance id.
+    if (spannerMetricData.stream()
+        .flatMap(metricData -> metricData.getData().getPoints().stream())
+        .anyMatch(pd -> SpannerCloudMonitoringExporterUtils.getInstanceId(pd) == null)) {
+      logger.log(Level.WARNING, "Metric data has missing instanceId. Skipping export.");
+      return CompletableResultCode.ofFailure();
+    }
+
     List<TimeSeries> spannerTimeSeries;
     try {
       spannerTimeSeries =
@@ -166,7 +174,7 @@ public void onFailure(Throwable throwable) {
                 // TODO: Add the link of public documentation when available in the log message.
                 msg +=
                     String.format(
-                        " Need monitoring metric writer permission on project=%s.",
+                        " Need monitoring metric writer permission on project=%s. Follow https://cloud.google.com/spanner/docs/view-manage-client-side-metrics#access-client-side-metrics to set up permissions",
                         projectName.getProject());
               }
               logger.log(Level.WARNING, msg, throwable);

diff --git a/...d-spanner/src/main/java/com/google/cloud/spanner/SpannerCloudMonitoringExporterUtils.java b/...d-spanner/src/main/java/com/google/cloud/spanner/SpannerCloudMonitoringExporterUtils.java
@@ -23,6 +23,7 @@
 import static com.google.api.MetricDescriptor.ValueType.DOUBLE;
 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.INSTANCE_ID_KEY;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.PROJECT_ID_KEY;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.SPANNER_PROMOTED_RESOURCE_LABELS;
 import static com.google.cloud.spanner.BuiltInMetricsConstant.SPANNER_RESOURCE_TYPE;
@@ -66,6 +67,10 @@ static String getProjectId(PointData pointData) {
     return pointData.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<>();
 

diff --git a/...ud-spanner/src/test/java/com/google/cloud/spanner/SpannerCloudMonitoringExporterTest.java b/...ud-spanner/src/test/java/com/google/cloud/spanner/SpannerCloudMonitoringExporterTest.java
@@ -16,6 +16,7 @@
 
 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;
@@ -44,6 +45,7 @@
 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;
@@ -340,6 +342,55 @@ 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);
+    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));
+    assertThat(resultCode).isEqualTo(CompletableResultCode.ofFailure());
+  }
+
   private static class FakeMetricServiceClient extends MetricServiceClient {
 
     protected FakeMetricServiceClient(MetricServiceStub stub) {