From 625d1fcf1ca425a086707b7266170151e681bf0c Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Sun, 15 Mar 2020 13:24:53 +0100 Subject: [PATCH 1/2] fix: only register metrics once The default MetricsRegistry used by OpenCensus is a singleton that does not allow the same metrics to be registered multiple times. The session pool metrics gathering did not take this into account, and when multiple session pools were created within the same class loader, it would throw an InvalidArgumentException for the second session pool. Fixes #106. --- .../spanner/MetricRegistryConstants.java | 12 +- .../com/google/cloud/spanner/SessionPool.java | 147 ++++++++++++------ .../com/google/cloud/spanner/SpannerImpl.java | 11 +- .../google/cloud/spanner/SessionPoolTest.java | 11 +- 4 files changed, 107 insertions(+), 74 deletions(-) diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/MetricRegistryConstants.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/MetricRegistryConstants.java index 601116180c8..5902598c895 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/MetricRegistryConstants.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/MetricRegistryConstants.java @@ -15,6 +15,7 @@ */ package com.google.cloud.spanner; +import com.google.api.gax.core.GaxProperties; import com.google.common.collect.ImmutableList; import io.opencensus.metrics.LabelKey; import io.opencensus.metrics.LabelValue; @@ -23,20 +24,13 @@ class MetricRegistryConstants { // The label keys are used to uniquely identify timeseries. - private static final LabelKey DATABASE = LabelKey.create("database", "Target database"); - private static final LabelKey INSTANCE_ID = - LabelKey.create("instance_id", "Name of the instance"); private static final LabelKey LIBRARY_VERSION = LabelKey.create("library_version", "Library version"); - /** The label value is used to represent missing value. */ - private static final LabelValue UNSET_LABEL = LabelValue.create(null); - - static final ImmutableList SPANNER_LABEL_KEYS = - ImmutableList.of(DATABASE, INSTANCE_ID, LIBRARY_VERSION); + static final ImmutableList SPANNER_LABEL_KEYS = ImmutableList.of(LIBRARY_VERSION); static final ImmutableList SPANNER_DEFAULT_LABEL_VALUES = - ImmutableList.of(UNSET_LABEL, UNSET_LABEL, UNSET_LABEL); + ImmutableList.of(LabelValue.create(GaxProperties.getLibraryVersion(SpannerImpl.class))); /** Unit to represent counts. */ static final String COUNT = "1"; diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java index 5023f0fafb9..729b01f79c9 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java @@ -69,10 +69,13 @@ import io.opencensus.trace.Status; import io.opencensus.trace.Tracer; import io.opencensus.trace.Tracing; +import java.util.Arrays; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.Queue; import java.util.Random; import java.util.Set; @@ -1088,6 +1091,11 @@ private static enum Position { RANDOM; } + private static final Object POOLS_LOCK = new Object(); + + @GuardedBy("POOLS_LOCK") + private static final Map> REGISTERED_POOLS = new HashMap<>(); + private final SessionPoolOptions options; private final SessionClient sessionClient; private final ScheduledExecutorService executor; @@ -1150,15 +1158,12 @@ private static enum Position { * Return pool is immediately ready for use, though getting a session might block for sessions to * be created. */ - static SessionPool createPool( - SpannerOptions spannerOptions, SessionClient sessionClient, List labelValues) { + static SessionPool createPool(SpannerOptions spannerOptions, SessionClient sessionClient) { return createPool( spannerOptions.getSessionPoolOptions(), ((GrpcTransportOptions) spannerOptions.getTransportOptions()).getExecutorFactory(), sessionClient, - new Clock(), - Metrics.getMetricRegistry(), - labelValues); + new Clock()); } static SessionPool createPool( @@ -1210,6 +1215,14 @@ private SessionPool( Clock clock, MetricRegistry metricRegistry, List labelValues) { + synchronized (POOLS_LOCK) { + if (!REGISTERED_POOLS.containsKey(metricRegistry)) { + initMetricsCollection(metricRegistry, labelValues); + REGISTERED_POOLS.put(metricRegistry, new LinkedList<>(Arrays.asList(this))); + } else { + REGISTERED_POOLS.get(metricRegistry).add(this); + } + } this.options = options; this.executorFactory = executorFactory; this.executor = executor; @@ -1229,7 +1242,6 @@ private SessionPool( this.sessionClient = sessionClient; this.clock = clock; this.poolMaintainer = new PoolMaintainer(); - this.initMetricsCollection(metricRegistry, labelValues); } @VisibleForTesting @@ -1862,11 +1874,36 @@ public void onSessionCreateFailure(Throwable t, int createFailureForSessionCount } } + private static final class Sum implements ToLongFunction { + private final MetricRegistry registry; + private final Function function; + + static Sum of(MetricRegistry registry, Function function) { + return new Sum(registry, function); + } + + private Sum(MetricRegistry registry, Function function) { + this.registry = registry; + this.function = function; + } + + @Override + public long applyAsLong(Void input) { + long res = 0L; + synchronized (POOLS_LOCK) { + for (SessionPool pool : REGISTERED_POOLS.get(registry)) { + res += function.apply(pool); + } + } + return res; + } + }; + /** * Initializes and creates Spanner session relevant metrics. When coupled with an exporter, it * allows users to monitor client behavior. */ - private void initMetricsCollection(MetricRegistry metricRegistry, List labelValues) { + static void initMetricsCollection(MetricRegistry metricRegistry, List labelValues) { DerivedLongGauge maxInUseSessionsMetric = metricRegistry.addDerivedLongGauge( MAX_IN_USE_SESSIONS, @@ -1925,68 +1962,80 @@ private void initMetricsCollection(MetricRegistry metricRegistry, List() { - @Override - public long applyAsLong(SessionPool sessionPool) { - return sessionPool.maxSessionsInUse; - } - }); + null, + Sum.of( + metricRegistry, + new Function() { + @Override + public Long apply(SessionPool input) { + return Long.valueOf(input.maxSessionsInUse); + } + })); // The value of a maxSessions is observed from a callback function. This function is invoked // whenever metrics are collected. maxAllowedSessionsMetric.createTimeSeries( labelValues, - options, - new ToLongFunction() { - @Override - public long applyAsLong(SessionPoolOptions options) { - return options.getMaxSessions(); - } - }); + null, + Sum.of( + metricRegistry, + new Function() { + @Override + public Long apply(SessionPool input) { + return Long.valueOf(input.options.getMaxSessions()); + } + })); // The value of a numSessionsInUse is observed from a callback function. This function is // invoked whenever metrics are collected. numInUseSessionsMetric.createTimeSeries( labelValues, - this, - new ToLongFunction() { - @Override - public long applyAsLong(SessionPool sessionPool) { - return sessionPool.numSessionsInUse; - } - }); + null, + Sum.of( + metricRegistry, + new Function() { + @Override + public Long apply(SessionPool input) { + return Long.valueOf(input.numSessionsInUse); + } + })); // The value of a numWaiterTimeouts is observed from a callback function. This function is // invoked whenever metrics are collected. sessionsTimeouts.createTimeSeries( labelValues, - this, - new ToLongFunction() { - @Override - public long applyAsLong(SessionPool sessionPool) { - return sessionPool.getNumWaiterTimeouts(); - } - }); + null, + Sum.of( + metricRegistry, + new Function() { + @Override + public Long apply(SessionPool input) { + return input.getNumWaiterTimeouts(); + } + })); numAcquiredSessionsMetric.createTimeSeries( labelValues, - this, - new ToLongFunction() { - @Override - public long applyAsLong(SessionPool sessionPool) { - return sessionPool.numSessionsAcquired; - } - }); + null, + Sum.of( + metricRegistry, + new Function() { + @Override + public Long apply(SessionPool input) { + return input.numSessionsAcquired; + } + })); numReleasedSessionsMetric.createTimeSeries( labelValues, - this, - new ToLongFunction() { - @Override - public long applyAsLong(SessionPool sessionPool) { - return sessionPool.numSessionsReleased; - } - }); + null, + Sum.of( + metricRegistry, + new Function() { + @Override + public Long apply(SessionPool input) { + return input.numSessionsReleased; + } + })); } } diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index 08089c89e41..0178d26a6e6 100644 --- a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -16,7 +16,6 @@ package com.google.cloud.spanner; -import com.google.api.gax.core.GaxProperties; import com.google.api.gax.paging.Page; import com.google.cloud.BaseService; import com.google.cloud.PageImpl; @@ -28,11 +27,9 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Strings; -import com.google.common.collect.ImmutableList; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.spanner.v1.ExecuteSqlRequest.QueryOptions; -import io.opencensus.metrics.LabelValue; import io.opencensus.trace.Tracer; import io.opencensus.trace.Tracing; import java.util.ArrayList; @@ -153,14 +150,8 @@ public DatabaseClient getDatabaseClient(DatabaseId db) { if (dbClients.containsKey(db)) { return dbClients.get(db); } else { - List labelValues = - ImmutableList.of( - LabelValue.create(db.getDatabase()), - LabelValue.create(db.getInstanceId().getName()), - LabelValue.create(GaxProperties.getLibraryVersion(getOptions().getClass()))); SessionPool pool = - SessionPool.createPool( - getOptions(), SpannerImpl.this.getSessionClient(db), labelValues); + SessionPool.createPool(getOptions(), SpannerImpl.this.getSessionClient(db)); DatabaseClientImpl dbClient = createDatabaseClient(pool); dbClients.put(db, dbClient); return dbClient; diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java index 2047b6c8586..962d8489da9 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java @@ -1577,11 +1577,7 @@ public void testSessionMetrics() throws Exception { FakeClock clock = new FakeClock(); clock.currentTimeMillis = System.currentTimeMillis(); FakeMetricRegistry metricRegistry = new FakeMetricRegistry(); - List labelValues = - Arrays.asList( - LabelValue.create("database1"), - LabelValue.create("instance1"), - LabelValue.create("1.0.0")); + List labelValues = Arrays.asList(LabelValue.create("1.0.0")); setupMockSessionCreation(); pool = createPool(clock, metricRegistry, labelValues); @@ -1590,7 +1586,10 @@ public void testSessionMetrics() throws Exception { MetricsRecord record = metricRegistry.pollRecord(); assertThat(record.getMetrics().size()).isEqualTo(6); - assertThat(record.getMetrics()).containsEntry(MetricRegistryConstants.IN_USE_SESSIONS, 2L); + assertThat(record.getMetrics()).containsKey(MetricRegistryConstants.IN_USE_SESSIONS); + assertThat(record.getMetrics().get(MetricRegistryConstants.IN_USE_SESSIONS)).isEqualTo(2L); + // assertThat(record.getMetrics()).containsEntry(MetricRegistryConstants.IN_USE_SESSIONS, + // 2L); assertThat(record.getMetrics()).containsEntry(MetricRegistryConstants.MAX_IN_USE_SESSIONS, 2L); assertThat(record.getMetrics()).containsEntry(MetricRegistryConstants.GET_SESSION_TIMEOUTS, 0L); assertThat(record.getMetrics()) From 4a251ea5bd3306ceee4fd5c39e712f727d3ac11f Mon Sep 17 00:00:00 2001 From: Olav Loite Date: Sun, 15 Mar 2020 13:31:32 +0100 Subject: [PATCH 2/2] fix: remove commented out code --- .../test/java/com/google/cloud/spanner/SessionPoolTest.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java index 962d8489da9..7a507d80980 100644 --- a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java +++ b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SessionPoolTest.java @@ -1586,10 +1586,7 @@ public void testSessionMetrics() throws Exception { MetricsRecord record = metricRegistry.pollRecord(); assertThat(record.getMetrics().size()).isEqualTo(6); - assertThat(record.getMetrics()).containsKey(MetricRegistryConstants.IN_USE_SESSIONS); - assertThat(record.getMetrics().get(MetricRegistryConstants.IN_USE_SESSIONS)).isEqualTo(2L); - // assertThat(record.getMetrics()).containsEntry(MetricRegistryConstants.IN_USE_SESSIONS, - // 2L); + assertThat(record.getMetrics()).containsEntry(MetricRegistryConstants.IN_USE_SESSIONS, 2L); assertThat(record.getMetrics()).containsEntry(MetricRegistryConstants.MAX_IN_USE_SESSIONS, 2L); assertThat(record.getMetrics()).containsEntry(MetricRegistryConstants.GET_SESSION_TIMEOUTS, 0L); assertThat(record.getMetrics())