Skip to content

Commit 06b912c

Browse files
mutianfmutianf
authored
fix: allow factory to export to different projects (#2374)
This fix removed the check on Bigtable project id and gets the BigtableTable resource project id directly from metrics attribute. BigtableDataClientFactory can create one client for multiple projects. Removing the check allows people using BigtableDataClientFactory to export to different projects.
1 parent bac7005 commit 06b912c

File tree

9 files changed

+215
-80
lines changed

9 files changed

+215
-80
lines changed

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/BigtableDataSettings.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import com.google.cloud.bigtable.data.v2.stub.BigtableBatchingCallSettings;
3131
import com.google.cloud.bigtable.data.v2.stub.EnhancedBigtableStubSettings;
3232
import com.google.cloud.bigtable.data.v2.stub.metrics.MetricsProvider;
33+
import com.google.cloud.bigtable.data.v2.stub.metrics.NoopMetricsProvider;
3334
import com.google.common.base.MoreObjects;
3435
import com.google.common.base.Strings;
3536
import io.grpc.ManagedChannelBuilder;
@@ -127,6 +128,7 @@ public static Builder newBuilderForEmulator(String hostname, int port) {
127128
.setEndpoint(hostname + ":" + port)
128129
// disable channel refreshing when creating an emulator
129130
.setRefreshingChannel(false)
131+
.setMetricsProvider(NoopMetricsProvider.INSTANCE) // disable exporting metrics for emulator
130132
.setTransportChannelProvider(
131133
InstantiatingGrpcChannelProvider.newBuilder()
132134
.setMaxInboundMessageSize(256 * 1024 * 1024)

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/BigtableClientContext.java‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -71,14 +71,9 @@ public static BigtableClientContext create(EnhancedBigtableStubSettings settings
7171
try {
7272
// We don't want client side metrics to crash the client, so catch any exception when getting
7373
// the OTEL instance and log the exception instead.
74-
// TODO openTelemetry doesn't need to be tied to a project id. This is incorrect and will be
75-
// fixed in the following PR.
7674
openTelemetry =
7775
getOpenTelemetryFromMetricsProvider(
78-
settings.getProjectId(),
79-
settings.getMetricsProvider(),
80-
credentials,
81-
settings.getMetricsEndpoint());
76+
settings.getMetricsProvider(), credentials, settings.getMetricsEndpoint());
8277
} catch (Throwable t) {
8378
logger.log(Level.WARNING, "Failed to get OTEL, will skip exporting client side metrics", t);
8479
}
@@ -144,7 +139,6 @@ public void close() throws Exception {
144139
}
145140

146141
private static OpenTelemetry getOpenTelemetryFromMetricsProvider(
147-
String projectId,
148142
MetricsProvider metricsProvider,
149143
@Nullable Credentials defaultCredentials,
150144
@Nullable String metricsEndpoint)
@@ -159,7 +153,7 @@ private static OpenTelemetry getOpenTelemetryFromMetricsProvider(
159153
? BigtableDataSettings.getMetricsCredentials()
160154
: defaultCredentials;
161155
DefaultMetricsProvider defaultMetricsProvider = (DefaultMetricsProvider) metricsProvider;
162-
return defaultMetricsProvider.getOpenTelemetry(projectId, metricsEndpoint, credentials);
156+
return defaultMetricsProvider.getOpenTelemetry(metricsEndpoint, credentials);
163157
} else if (metricsProvider instanceof NoopMetricsProvider) {
164158
return null;
165159
}

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableCloudMonitoringExporter.java‎

Lines changed: 36 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import java.util.Arrays;
5959
import java.util.Collection;
6060
import java.util.List;
61+
import java.util.Map;
6162
import java.util.Optional;
6263
import java.util.concurrent.atomic.AtomicBoolean;
6364
import java.util.logging.Level;
@@ -94,7 +95,6 @@ public final class BigtableCloudMonitoringExporter implements MetricExporter {
9495

9596
private final MetricServiceClient client;
9697

97-
private final String bigtableProjectId;
9898
private final String taskId;
9999

100100
// The resource the client application is running on
@@ -128,8 +128,7 @@ public final class BigtableCloudMonitoringExporter implements MetricExporter {
128128
.collect(ImmutableList.toImmutableList());
129129

130130
public static BigtableCloudMonitoringExporter create(
131-
String projectId, @Nullable Credentials credentials, @Nullable String endpoint)
132-
throws IOException {
131+
@Nullable Credentials credentials, @Nullable String endpoint) throws IOException {
133132
MetricServiceSettings.Builder settingsBuilder = MetricServiceSettings.newBuilder();
134133
CredentialsProvider credentialsProvider =
135134
Optional.ofNullable(credentials)
@@ -164,22 +163,17 @@ public static BigtableCloudMonitoringExporter create(
164163
}
165164

166165
return new BigtableCloudMonitoringExporter(
167-
projectId,
168166
MetricServiceClient.create(settingsBuilder.build()),
169167
applicationResource,
170168
BigtableExporterUtils.getDefaultTaskValue());
171169
}
172170

173171
@VisibleForTesting
174172
BigtableCloudMonitoringExporter(
175-
String projectId,
176-
MetricServiceClient client,
177-
@Nullable MonitoredResource applicationResource,
178-
String taskId) {
173+
MetricServiceClient client, @Nullable MonitoredResource applicationResource, String taskId) {
179174
this.client = client;
180175
this.taskId = taskId;
181176
this.applicationResource = applicationResource;
182-
this.bigtableProjectId = projectId;
183177
}
184178

185179
@Override
@@ -211,15 +205,8 @@ private CompletableResultCode exportBigtableResourceMetrics(Collection<MetricDat
211205
return CompletableResultCode.ofSuccess();
212206
}
213207

214-
// Verifies metrics project id are the same as the bigtable project id set on this client
215-
if (!bigtableMetricData.stream()
216-
.flatMap(metricData -> metricData.getData().getPoints().stream())
217-
.allMatch(pd -> bigtableProjectId.equals(BigtableExporterUtils.getProjectId(pd)))) {
218-
logger.log(Level.WARNING, "Metric data has different a projectId. Skip exporting.");
219-
return CompletableResultCode.ofFailure();
220-
}
221-
222-
List<TimeSeries> bigtableTimeSeries;
208+
// List of timeseries by project id
209+
Map<String, List<TimeSeries>> bigtableTimeSeries;
223210
try {
224211
bigtableTimeSeries =
225212
BigtableExporterUtils.convertToBigtableTimeSeries(bigtableMetricData, taskId);
@@ -231,37 +218,39 @@ private CompletableResultCode exportBigtableResourceMetrics(Collection<MetricDat
231218
return CompletableResultCode.ofFailure();
232219
}
233220

234-
ProjectName projectName = ProjectName.of(bigtableProjectId);
235-
ApiFuture<List<Empty>> future = exportTimeSeries(projectName, bigtableTimeSeries);
236-
237221
CompletableResultCode bigtableExportCode = new CompletableResultCode();
238-
ApiFutures.addCallback(
239-
future,
240-
new ApiFutureCallback<List<Empty>>() {
241-
@Override
242-
public void onFailure(Throwable throwable) {
243-
if (bigtableExportFailureLogged.compareAndSet(false, true)) {
244-
String msg = "createServiceTimeSeries request failed for bigtable metrics.";
245-
if (throwable instanceof PermissionDeniedException) {
246-
msg +=
247-
String.format(
248-
" Need monitoring metric writer permission on project=%s. Follow https://cloud.google.com/bigtable/docs/client-side-metrics-setup to set up permissions.",
249-
projectName.getProject());
250-
}
251-
logger.log(Level.WARNING, msg, throwable);
252-
}
253-
bigtableExportCode.fail();
254-
}
222+
bigtableTimeSeries.forEach(
223+
(projectId, ts) -> {
224+
ProjectName projectName = ProjectName.of(projectId);
225+
ApiFuture<List<Empty>> future = exportTimeSeries(projectName, ts);
226+
ApiFutures.addCallback(
227+
future,
228+
new ApiFutureCallback<List<Empty>>() {
229+
@Override
230+
public void onFailure(Throwable throwable) {
231+
if (bigtableExportFailureLogged.compareAndSet(false, true)) {
232+
String msg = "createServiceTimeSeries request failed for bigtable metrics.";
233+
if (throwable instanceof PermissionDeniedException) {
234+
msg +=
235+
String.format(
236+
" Need monitoring metric writer permission on project=%s. Follow https://cloud.google.com/bigtable/docs/client-side-metrics-setup to set up permissions.",
237+
projectName.getProject());
238+
}
239+
logger.log(Level.WARNING, msg, throwable);
240+
}
241+
bigtableExportCode.fail();
242+
}
255243

256-
@Override
257-
public void onSuccess(List<Empty> emptyList) {
258-
// When an export succeeded reset the export failure flag to false so if there's a
259-
// transient failure it'll be logged.
260-
bigtableExportFailureLogged.set(false);
261-
bigtableExportCode.succeed();
262-
}
263-
},
264-
MoreExecutors.directExecutor());
244+
@Override
245+
public void onSuccess(List<Empty> emptyList) {
246+
// When an export succeeded reset the export failure flag to false so if there's a
247+
// transient failure it'll be logged.
248+
bigtableExportFailureLogged.set(false);
249+
bigtableExportCode.succeed();
250+
}
251+
},
252+
MoreExecutors.directExecutor());
253+
});
265254

266255
return bigtableExportCode;
267256
}

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BigtableExporterUtils.java‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
import java.net.UnknownHostException;
6464
import java.util.ArrayList;
6565
import java.util.Collection;
66+
import java.util.HashMap;
6667
import java.util.List;
6768
import java.util.Map;
6869
import java.util.Set;
@@ -110,17 +111,24 @@ static String getProjectId(PointData pointData) {
110111
return pointData.getAttributes().get(BIGTABLE_PROJECT_ID_KEY);
111112
}
112113

113-
static List<TimeSeries> convertToBigtableTimeSeries(List<MetricData> collection, String taskId) {
114-
List<TimeSeries> allTimeSeries = new ArrayList<>();
114+
// Returns a list of timeseries by project id
115+
static Map<String, List<TimeSeries>> convertToBigtableTimeSeries(
116+
List<MetricData> collection, String taskId) {
117+
Map<String, List<TimeSeries>> allTimeSeries = new HashMap<>();
115118

116119
for (MetricData metricData : collection) {
117120
if (!metricData.getInstrumentationScopeInfo().getName().equals(METER_NAME)) {
118121
// Filter out metric data for instruments that are not part of the bigtable builtin metrics
119122
continue;
120123
}
121-
metricData.getData().getPoints().stream()
122-
.map(pointData -> convertPointToBigtableTimeSeries(metricData, pointData, taskId))
123-
.forEach(allTimeSeries::add);
124+
125+
for (PointData pd : metricData.getData().getPoints()) {
126+
String projectId = getProjectId(pd);
127+
List<TimeSeries> current =
128+
allTimeSeries.computeIfAbsent(projectId, ignored -> new ArrayList<>());
129+
current.add(convertPointToBigtableTimeSeries(metricData, pd, taskId));
130+
allTimeSeries.put(projectId, current);
131+
}
124132
}
125133

126134
return allTimeSeries;

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/BuiltinMetricsView.java‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,35 +38,65 @@ private BuiltinMetricsView() {}
3838
/**
3939
* Register built-in metrics on the {@link SdkMeterProviderBuilder} with application default
4040
* credentials and default endpoint.
41+
*
42+
* @deprecated projectId is no longer used. Call {@link
43+
* #registerBuiltinMetrics(SdkMeterProviderBuilder)} instead.
4144
*/
45+
@Deprecated
4246
public static void registerBuiltinMetrics(String projectId, SdkMeterProviderBuilder builder)
4347
throws IOException {
4448
BuiltinMetricsView.registerBuiltinMetrics(
45-
projectId, GoogleCredentials.getApplicationDefault(), builder);
49+
GoogleCredentials.getApplicationDefault(), builder, null);
50+
}
51+
52+
/**
53+
* Register built-in metrics on the {@link SdkMeterProviderBuilder} with application default
54+
* credentials and default endpoint.
55+
*/
56+
public static void registerBuiltinMetrics(SdkMeterProviderBuilder builder) throws IOException {
57+
BuiltinMetricsView.registerBuiltinMetrics(
58+
GoogleCredentials.getApplicationDefault(), builder, null);
4659
}
4760

4861
/**
4962
* Register built-in metrics on the {@link SdkMeterProviderBuilder} with custom credentials and
5063
* default endpoint.
64+
*
65+
* @deprecated projectId is no longer used. Call {@link #registerBuiltinMetrics(Credentials,
66+
* SdkMeterProviderBuilder, String)} instead.
5167
*/
68+
@Deprecated
5269
public static void registerBuiltinMetrics(
5370
String projectId, @Nullable Credentials credentials, SdkMeterProviderBuilder builder)
5471
throws IOException {
55-
BuiltinMetricsView.registerBuiltinMetrics(projectId, credentials, builder, null);
72+
BuiltinMetricsView.registerBuiltinMetrics(credentials, builder, null);
5673
}
5774

5875
/**
5976
* Register built-in metrics on the {@link SdkMeterProviderBuilder} with custom credentials and
6077
* endpoint.
78+
*
79+
* @deprecated projectId is no longer used. Call {@link #registerBuiltinMetrics(Credentials,
80+
* SdkMeterProviderBuilder, String)} instead.
6181
*/
82+
@Deprecated
6283
public static void registerBuiltinMetrics(
6384
String projectId,
6485
@Nullable Credentials credentials,
6586
SdkMeterProviderBuilder builder,
6687
@Nullable String endpoint)
6788
throws IOException {
68-
MetricExporter metricExporter =
69-
BigtableCloudMonitoringExporter.create(projectId, credentials, endpoint);
89+
registerBuiltinMetrics(credentials, builder, endpoint);
90+
}
91+
92+
/**
93+
* Register built-in metrics on the {@link SdkMeterProviderBuilder} with custom credentials and
94+
* endpoint.
95+
*/
96+
public static void registerBuiltinMetrics(
97+
@Nullable Credentials credentials, SdkMeterProviderBuilder builder, @Nullable String endpoint)
98+
throws IOException {
99+
MetricExporter metricExporter = BigtableCloudMonitoringExporter.create(credentials, endpoint);
70100
for (Map.Entry<InstrumentSelector, View> entry :
71101
BuiltinMetricsConstants.getAllViews().entrySet()) {
72102
builder.registerView(entry.getKey(), entry.getValue());

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/CustomOpenTelemetryMetricsProvider.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
* SdkMeterProviderBuilder sdkMeterProvider = SdkMeterProvider.builder();
2828
*
2929
* // register Builtin metrics on your meter provider with default credentials
30-
* BuiltinMetricsView.registerBuiltinMetrics("project-id", sdkMeterProvider);
30+
* BuiltinMetricsView.registerBuiltinMetrics(sdkMeterProvider);
3131
*
3232
* // register other metrics reader and views
3333
* sdkMeterProvider.registerMetricReader(..);

‎google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/metrics/DefaultMetricsProvider.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,9 @@ private DefaultMetricsProvider() {}
3939

4040
@InternalApi
4141
public OpenTelemetry getOpenTelemetry(
42-
String projectId, @Nullable String metricsEndpoint, @Nullable Credentials credentials)
43-
throws IOException {
42+
@Nullable String metricsEndpoint, @Nullable Credentials credentials) throws IOException {
4443
SdkMeterProviderBuilder meterProvider = SdkMeterProvider.builder();
45-
BuiltinMetricsView.registerBuiltinMetrics(
46-
projectId, credentials, meterProvider, metricsEndpoint);
44+
BuiltinMetricsView.registerBuiltinMetrics(credentials, meterProvider, metricsEndpoint);
4745
return OpenTelemetrySdk.builder().setMeterProvider(meterProvider.build()).build();
4846
}
4947

‎google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubTest.java‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -825,15 +825,16 @@ public void testExecuteQueryWaitTimeoutWorksWithMetadataFuture()
825825
settings.setStreamWatchdogProvider(
826826
InstantiatingWatchdogProvider.create().withCheckInterval(WATCHDOG_CHECK_DURATION));
827827

828-
EnhancedBigtableStub stub = EnhancedBigtableStub.create(settings.build());
829-
ApiFuture<ResultSetMetadata> future =
830-
stub.executeQueryCallable().call(Statement.of(WAIT_TIME_QUERY)).metadataFuture();
831-
832-
ExecutionException e = assertThrows(ExecutionException.class, future::get);
833-
assertThat(e.getCause()).isInstanceOf(WatchdogTimeoutException.class);
834-
assertThat(e.getCause().getMessage())
835-
.contains("Canceled due to timeout waiting for next response");
836-
assertThat(e).hasMessageThat().contains("Canceled due to timeout waiting for next response");
828+
try (EnhancedBigtableStub stub = EnhancedBigtableStub.create(settings.build())) {
829+
ApiFuture<ResultSetMetadata> future =
830+
stub.executeQueryCallable().call(Statement.of(WAIT_TIME_QUERY)).metadataFuture();
831+
832+
ExecutionException e = assertThrows(ExecutionException.class, future::get);
833+
assertThat(e.getCause()).isInstanceOf(WatchdogTimeoutException.class);
834+
assertThat(e.getCause().getMessage())
835+
.contains("Canceled due to timeout waiting for next response");
836+
assertThat(e).hasMessageThat().contains("Canceled due to timeout waiting for next response");
837+
}
837838
}
838839

839840
private static class MetadataInterceptor implements ServerInterceptor {

0 commit comments

Comments
 (0)