diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java index 928159aa6..25ff2ff30 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java @@ -30,6 +30,7 @@ import com.google.cloud.bigtable.data.v2.stub.BigtableBatchingCallSettings; import com.google.cloud.bigtable.data.v2.stub.EnhancedBigtableStubSettings; import com.google.cloud.bigtable.data.v2.stub.metrics.MetricsProvider; +import com.google.cloud.bigtable.data.v2.stub.metrics.NoopMetricsProvider; import com.google.common.base.MoreObjects; import com.google.common.base.Strings; import io.grpc.ManagedChannelBuilder; @@ -127,6 +128,7 @@ public static Builder newBuilderForEmulator(String hostname, int port) { .setEndpoint(hostname + ":" + port) // disable channel refreshing when creating an emulator .setRefreshingChannel(false) + .setMetricsProvider(NoopMetricsProvider.INSTANCE) // disable exporting metrics for emulator .setTransportChannelProvider( InstantiatingGrpcChannelProvider.newBuilder() .setMaxInboundMessageSize(256 * 1024 * 1024) diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java index 170c5f24e..a2587b0dd 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java @@ -71,14 +71,9 @@ public static BigtableClientContext create(EnhancedBigtableStubSettings settings try { // We don't want client side metrics to crash the client, so catch any exception when getting // the OTEL instance and log the exception instead. - // TODO openTelemetry doesn't need to be tied to a project id. This is incorrect and will be - // fixed in the following PR. openTelemetry = getOpenTelemetryFromMetricsProvider( - settings.getProjectId(), - settings.getMetricsProvider(), - credentials, - settings.getMetricsEndpoint()); + settings.getMetricsProvider(), credentials, settings.getMetricsEndpoint()); } catch (Throwable t) { logger.log(Level.WARNING, "Failed to get OTEL, will skip exporting client side metrics", t); } @@ -145,7 +140,6 @@ public void close() throws Exception { } private static OpenTelemetry getOpenTelemetryFromMetricsProvider( - String projectId, MetricsProvider metricsProvider, @Nullable Credentials defaultCredentials, @Nullable String metricsEndpoint) @@ -160,7 +154,7 @@ private static OpenTelemetry getOpenTelemetryFromMetricsProvider( ? BigtableDataSettings.getMetricsCredentials() : defaultCredentials; DefaultMetricsProvider defaultMetricsProvider = (DefaultMetricsProvider) metricsProvider; - return defaultMetricsProvider.getOpenTelemetry(projectId, metricsEndpoint, credentials); + return defaultMetricsProvider.getOpenTelemetry(metricsEndpoint, credentials); } else if (metricsProvider instanceof NoopMetricsProvider) { return null; } diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableCloudMonitoringExporter.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableCloudMonitoringExporter.java index 8aa53fa19..ff5bcd81c 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableCloudMonitoringExporter.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableCloudMonitoringExporter.java @@ -58,6 +58,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Level; @@ -94,7 +95,6 @@ public final class BigtableCloudMonitoringExporter implements MetricExporter { private final MetricServiceClient client; - private final String bigtableProjectId; private final String taskId; // The resource the client application is running on @@ -128,8 +128,7 @@ public final class BigtableCloudMonitoringExporter implements MetricExporter { .collect(ImmutableList.toImmutableList()); public static BigtableCloudMonitoringExporter create( - String projectId, @Nullable Credentials credentials, @Nullable String endpoint) - throws IOException { + @Nullable Credentials credentials, @Nullable String endpoint) throws IOException { MetricServiceSettings.Builder settingsBuilder = MetricServiceSettings.newBuilder(); CredentialsProvider credentialsProvider = Optional.ofNullable(credentials) @@ -164,7 +163,6 @@ public static BigtableCloudMonitoringExporter create( } return new BigtableCloudMonitoringExporter( - projectId, MetricServiceClient.create(settingsBuilder.build()), applicationResource, BigtableExporterUtils.getDefaultTaskValue()); @@ -172,14 +170,10 @@ public static BigtableCloudMonitoringExporter create( @VisibleForTesting BigtableCloudMonitoringExporter( - String projectId, - MetricServiceClient client, - @Nullable MonitoredResource applicationResource, - String taskId) { + MetricServiceClient client, @Nullable MonitoredResource applicationResource, String taskId) { this.client = client; this.taskId = taskId; this.applicationResource = applicationResource; - this.bigtableProjectId = projectId; } @Override @@ -211,15 +205,8 @@ private CompletableResultCode exportBigtableResourceMetrics(Collection metricData.getData().getPoints().stream()) - .allMatch(pd -> bigtableProjectId.equals(BigtableExporterUtils.getProjectId(pd)))) { - logger.log(Level.WARNING, "Metric data has different a projectId. Skip exporting."); - return CompletableResultCode.ofFailure(); - } - - List bigtableTimeSeries; + // List of timeseries by project id + Map> bigtableTimeSeries; try { bigtableTimeSeries = BigtableExporterUtils.convertToBigtableTimeSeries(bigtableMetricData, taskId); @@ -231,37 +218,39 @@ private CompletableResultCode exportBigtableResourceMetrics(Collection> future = exportTimeSeries(projectName, bigtableTimeSeries); - CompletableResultCode bigtableExportCode = new CompletableResultCode(); - ApiFutures.addCallback( - future, - new ApiFutureCallback>() { - @Override - public void onFailure(Throwable throwable) { - if (bigtableExportFailureLogged.compareAndSet(false, true)) { - String msg = "createServiceTimeSeries request failed for bigtable metrics."; - if (throwable instanceof PermissionDeniedException) { - msg += - String.format( - " Need monitoring metric writer permission on project=%s. Follow https://cloud.google.com/bigtable/docs/client-side-metrics-setup to set up permissions.", - projectName.getProject()); - } - logger.log(Level.WARNING, msg, throwable); - } - bigtableExportCode.fail(); - } + bigtableTimeSeries.forEach( + (projectId, ts) -> { + ProjectName projectName = ProjectName.of(projectId); + ApiFuture> future = exportTimeSeries(projectName, ts); + ApiFutures.addCallback( + future, + new ApiFutureCallback>() { + @Override + public void onFailure(Throwable throwable) { + if (bigtableExportFailureLogged.compareAndSet(false, true)) { + String msg = "createServiceTimeSeries request failed for bigtable metrics."; + if (throwable instanceof PermissionDeniedException) { + msg += + String.format( + " Need monitoring metric writer permission on project=%s. Follow https://cloud.google.com/bigtable/docs/client-side-metrics-setup to set up permissions.", + projectName.getProject()); + } + logger.log(Level.WARNING, msg, throwable); + } + bigtableExportCode.fail(); + } - @Override - public void onSuccess(List emptyList) { - // When an export succeeded reset the export failure flag to false so if there's a - // transient failure it'll be logged. - bigtableExportFailureLogged.set(false); - bigtableExportCode.succeed(); - } - }, - MoreExecutors.directExecutor()); + @Override + public void onSuccess(List emptyList) { + // When an export succeeded reset the export failure flag to false so if there's a + // transient failure it'll be logged. + bigtableExportFailureLogged.set(false); + bigtableExportCode.succeed(); + } + }, + MoreExecutors.directExecutor()); + }); return bigtableExportCode; } diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableExporterUtils.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableExporterUtils.java index 5bf6688e1..821c2295e 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableExporterUtils.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableExporterUtils.java @@ -63,6 +63,7 @@ import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -110,17 +111,24 @@ static String getProjectId(PointData pointData) { return pointData.getAttributes().get(BIGTABLE_PROJECT_ID_KEY); } - static List convertToBigtableTimeSeries(List collection, String taskId) { - List allTimeSeries = new ArrayList<>(); + // Returns a list of timeseries by project id + static Map> convertToBigtableTimeSeries( + List collection, String taskId) { + Map> allTimeSeries = new HashMap<>(); for (MetricData metricData : collection) { if (!metricData.getInstrumentationScopeInfo().getName().equals(METER_NAME)) { // Filter out metric data for instruments that are not part of the bigtable builtin metrics continue; } - metricData.getData().getPoints().stream() - .map(pointData -> convertPointToBigtableTimeSeries(metricData, pointData, taskId)) - .forEach(allTimeSeries::add); + + for (PointData pd : metricData.getData().getPoints()) { + String projectId = getProjectId(pd); + List current = + allTimeSeries.computeIfAbsent(projectId, ignored -> new ArrayList<>()); + current.add(convertPointToBigtableTimeSeries(metricData, pd, taskId)); + allTimeSeries.put(projectId, current); + } } return allTimeSeries; diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsView.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsView.java index ca52581a9..07679af8d 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsView.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsView.java @@ -38,35 +38,65 @@ private BuiltinMetricsView() {} /** * Register built-in metrics on the {@link SdkMeterProviderBuilder} with application default * credentials and default endpoint. + * + * @deprecated projectId is no longer used. Call {@link + * #registerBuiltinMetrics(SdkMeterProviderBuilder)} instead. */ + @Deprecated public static void registerBuiltinMetrics(String projectId, SdkMeterProviderBuilder builder) throws IOException { BuiltinMetricsView.registerBuiltinMetrics( - projectId, GoogleCredentials.getApplicationDefault(), builder); + GoogleCredentials.getApplicationDefault(), builder, null); + } + + /** + * Register built-in metrics on the {@link SdkMeterProviderBuilder} with application default + * credentials and default endpoint. + */ + public static void registerBuiltinMetrics(SdkMeterProviderBuilder builder) throws IOException { + BuiltinMetricsView.registerBuiltinMetrics( + GoogleCredentials.getApplicationDefault(), builder, null); } /** * Register built-in metrics on the {@link SdkMeterProviderBuilder} with custom credentials and * default endpoint. + * + * @deprecated projectId is no longer used. Call {@link #registerBuiltinMetrics(Credentials, + * SdkMeterProviderBuilder, String)} instead. */ + @Deprecated public static void registerBuiltinMetrics( String projectId, @Nullable Credentials credentials, SdkMeterProviderBuilder builder) throws IOException { - BuiltinMetricsView.registerBuiltinMetrics(projectId, credentials, builder, null); + BuiltinMetricsView.registerBuiltinMetrics(credentials, builder, null); } /** * Register built-in metrics on the {@link SdkMeterProviderBuilder} with custom credentials and * endpoint. + * + * @deprecated projectId is no longer used. Call {@link #registerBuiltinMetrics(Credentials, + * SdkMeterProviderBuilder, String)} instead. */ + @Deprecated public static void registerBuiltinMetrics( String projectId, @Nullable Credentials credentials, SdkMeterProviderBuilder builder, @Nullable String endpoint) throws IOException { - MetricExporter metricExporter = - BigtableCloudMonitoringExporter.create(projectId, credentials, endpoint); + registerBuiltinMetrics(credentials, builder, endpoint); + } + + /** + * Register built-in metrics on the {@link SdkMeterProviderBuilder} with custom credentials and + * endpoint. + */ + public static void registerBuiltinMetrics( + @Nullable Credentials credentials, SdkMeterProviderBuilder builder, @Nullable String endpoint) + throws IOException { + MetricExporter metricExporter = BigtableCloudMonitoringExporter.create(credentials, endpoint); for (Map.Entry entry : BuiltinMetricsConstants.getAllViews().entrySet()) { builder.registerView(entry.getKey(), entry.getValue()); diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CustomOpenTelemetryMetricsProvider.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CustomOpenTelemetryMetricsProvider.java index 8c1c5c1c9..d728d657a 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CustomOpenTelemetryMetricsProvider.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CustomOpenTelemetryMetricsProvider.java @@ -27,7 +27,7 @@ * SdkMeterProviderBuilder sdkMeterProvider = SdkMeterProvider.builder(); * * // register Builtin metrics on your meter provider with default credentials - * BuiltinMetricsView.registerBuiltinMetrics("project-id", sdkMeterProvider); + * BuiltinMetricsView.registerBuiltinMetrics(sdkMeterProvider); * * // register other metrics reader and views * sdkMeterProvider.registerMetricReader(..); diff --git a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/DefaultMetricsProvider.java b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/DefaultMetricsProvider.java index d1870dd83..ae4df8589 100644 --- a/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/DefaultMetricsProvider.java +++ b/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/DefaultMetricsProvider.java @@ -39,11 +39,9 @@ private DefaultMetricsProvider() {} @InternalApi public OpenTelemetry getOpenTelemetry( - String projectId, @Nullable String metricsEndpoint, @Nullable Credentials credentials) - throws IOException { + @Nullable String metricsEndpoint, @Nullable Credentials credentials) throws IOException { SdkMeterProviderBuilder meterProvider = SdkMeterProvider.builder(); - BuiltinMetricsView.registerBuiltinMetrics( - projectId, credentials, meterProvider, metricsEndpoint); + BuiltinMetricsView.registerBuiltinMetrics(credentials, meterProvider, metricsEndpoint); return OpenTelemetrySdk.builder().setMeterProvider(meterProvider.build()).build(); } diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java index 495250fe1..67ac3f24d 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java @@ -825,15 +825,16 @@ public void testExecuteQueryWaitTimeoutWorksWithMetadataFuture() settings.setStreamWatchdogProvider( InstantiatingWatchdogProvider.create().withCheckInterval(WATCHDOG_CHECK_DURATION)); - EnhancedBigtableStub stub = EnhancedBigtableStub.create(settings.build()); - ApiFuture future = - stub.executeQueryCallable().call(Statement.of(WAIT_TIME_QUERY)).metadataFuture(); - - ExecutionException e = assertThrows(ExecutionException.class, future::get); - assertThat(e.getCause()).isInstanceOf(WatchdogTimeoutException.class); - assertThat(e.getCause().getMessage()) - .contains("Canceled due to timeout waiting for next response"); - assertThat(e).hasMessageThat().contains("Canceled due to timeout waiting for next response"); + try (EnhancedBigtableStub stub = EnhancedBigtableStub.create(settings.build())) { + ApiFuture future = + stub.executeQueryCallable().call(Statement.of(WAIT_TIME_QUERY)).metadataFuture(); + + ExecutionException e = assertThrows(ExecutionException.class, future::get); + assertThat(e.getCause()).isInstanceOf(WatchdogTimeoutException.class); + assertThat(e.getCause().getMessage()) + .contains("Canceled due to timeout waiting for next response"); + assertThat(e).hasMessageThat().contains("Canceled due to timeout waiting for next response"); + } } private static class MetadataInterceptor implements ServerInterceptor { diff --git a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableCloudMonitoringExporterTest.java b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableCloudMonitoringExporterTest.java index 81629e2d9..657db7d8a 100644 --- a/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableCloudMonitoringExporterTest.java +++ b/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableCloudMonitoringExporterTest.java @@ -24,7 +24,10 @@ import static com.google.cloud.bigtable.data.v2.stub.metrics.BuiltinMetricsConstants.TABLE_ID_KEY; import static com.google.cloud.bigtable.data.v2.stub.metrics.BuiltinMetricsConstants.ZONE_ID_KEY; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.google.api.Distribution; @@ -35,6 +38,7 @@ import com.google.cloud.monitoring.v3.MetricServiceClient; import com.google.cloud.monitoring.v3.stub.MetricServiceStub; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.monitoring.v3.CreateTimeSeriesRequest; import com.google.monitoring.v3.TimeSeries; import com.google.protobuf.Empty; @@ -53,6 +57,8 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.List; +import java.util.Map; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -89,7 +95,7 @@ public void setUp() { exporter = new BigtableCloudMonitoringExporter( - projectId, fakeMetricServiceClient, /* applicationResource= */ null, taskId); + fakeMetricServiceClient, /* applicationResource= */ null, taskId); attributes = Attributes.builder() @@ -301,7 +307,6 @@ public void testTimeSeriesForMetricWithGceOrGkeResource() { String gceProjectId = "fake-gce-project"; BigtableCloudMonitoringExporter exporter = new BigtableCloudMonitoringExporter( - projectId, fakeMetricServiceClient, MonitoredResource.newBuilder() .setType("gce-instance") @@ -377,6 +382,114 @@ public void testTimeSeriesForMetricWithGceOrGkeResource() { taskId); } + @Test + public void testExportingToMultipleProjects() { + ArgumentCaptor argumentCaptor = + ArgumentCaptor.forClass(CreateTimeSeriesRequest.class); + + UnaryCallable mockCallable = mock(UnaryCallable.class); + when(mockMetricServiceStub.createServiceTimeSeriesCallable()).thenReturn(mockCallable); + ApiFuture future = ApiFutures.immediateFuture(Empty.getDefaultInstance()); + when(mockCallable.futureCall(any())).thenReturn(future); + + long startEpoch = 10; + long endEpoch = 15; + HistogramPointData histogramPointData1 = + ImmutableHistogramPointData.create( + startEpoch, + endEpoch, + attributes, + 3d, + true, + 1d, // min + true, + 2d, // max + Arrays.asList(1.0), + Arrays.asList(1L, 2L)); + + MetricData histogramData1 = + ImmutableMetricData.createDoubleHistogram( + resource, + scope, + "bigtable.googleapis.com/internal/client/operation_latencies", + "description", + "ms", + ImmutableHistogramData.create( + AggregationTemporality.CUMULATIVE, ImmutableList.of(histogramPointData1))); + + HistogramPointData histogramPointData2 = + ImmutableHistogramPointData.create( + startEpoch, + endEpoch, + attributes.toBuilder().put(BIGTABLE_PROJECT_ID_KEY, "another-project").build(), + 50d, + true, + 5d, // min + true, + 30d, // max + Arrays.asList(1.0), + Arrays.asList(5L, 10L)); + + MetricData histogramData2 = + ImmutableMetricData.createDoubleHistogram( + resource, + scope, + "bigtable.googleapis.com/internal/client/operation_latencies", + "description", + "ms", + ImmutableHistogramData.create( + AggregationTemporality.CUMULATIVE, ImmutableList.of(histogramPointData2))); + + exporter.export(Arrays.asList(histogramData1, histogramData2)); + + verify(mockCallable, times(2)).futureCall(argumentCaptor.capture()); + + List allValues = argumentCaptor.getAllValues(); + + assertThat(allValues).hasSize(2); + + List> labelsMap = new ArrayList<>(); + List counts = new ArrayList<>(); + allValues.forEach( + value -> { + labelsMap.add(value.getTimeSeriesList().get(0).getResource().getLabelsMap()); + counts.add( + value + .getTimeSeriesList() + .get(0) + .getPoints(0) + .getValue() + .getDistributionValue() + .getCount()); + }); + + assertThat(labelsMap) + .containsExactly( + ImmutableMap.of( + BIGTABLE_PROJECT_ID_KEY.getKey(), + projectId, + INSTANCE_ID_KEY.getKey(), + instanceId, + TABLE_ID_KEY.getKey(), + tableId, + CLUSTER_ID_KEY.getKey(), + cluster, + ZONE_ID_KEY.getKey(), + zone), + ImmutableMap.of( + BIGTABLE_PROJECT_ID_KEY.getKey(), + "another-project", + INSTANCE_ID_KEY.getKey(), + instanceId, + TABLE_ID_KEY.getKey(), + tableId, + CLUSTER_ID_KEY.getKey(), + cluster, + ZONE_ID_KEY.getKey(), + zone)); + assertThat(counts).containsExactly(3l, 15l); + } + private static class FakeMetricServiceClient extends MetricServiceClient { protected FakeMetricServiceClient(MetricServiceStub stub) {