From cebec6691ee69c46dc2f6938d7d865c9821b70b5 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Mon, 28 Mar 2022 08:50:10 -0400 Subject: [PATCH 1/3] HBASE-26891 Make MetricsConnection scope configurable --- .../hbase/client/AsyncConnectionImpl.java | 4 ++- .../hbase/client/MetricsConnection.java | 31 ++++++++++++++++++- .../hbase/client/TestMetricsConnection.java | 22 +++++++++++++ 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java index 1eebcab4c93c..c1cdae61d36f 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java @@ -24,6 +24,7 @@ import static org.apache.hadoop.hbase.client.ConnectionUtils.NO_NONCE_GENERATOR; import static org.apache.hadoop.hbase.client.ConnectionUtils.getStubKey; import static org.apache.hadoop.hbase.client.MetricsConnection.CLIENT_SIDE_METRICS_ENABLED_KEY; +import static org.apache.hadoop.hbase.client.MetricsConnection.METRICS_SCOPE_KEY; import static org.apache.hadoop.hbase.client.NonceGenerator.CLIENT_NONCES_ENABLED_KEY; import static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.SERVER_NAME_KEY; import static org.apache.hadoop.hbase.util.FutureUtils.addListener; @@ -126,7 +127,8 @@ public AsyncConnectionImpl(Configuration conf, ConnectionRegistry registry, Stri this.connConf = new AsyncConnectionConfiguration(conf); this.registry = registry; if (conf.getBoolean(CLIENT_SIDE_METRICS_ENABLED_KEY, false)) { - this.metrics = Optional.of(new MetricsConnection(this.toString(), () -> null, () -> null)); + String scope = MetricsConnection.getScope(conf, clusterId, this); + this.metrics = Optional.of(new MetricsConnection(scope, () -> null, () -> null)); } else { this.metrics = Optional.empty(); } diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java index 8566ec551e72..76f4a9ab8cfe 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/MetricsConnection.java @@ -34,6 +34,7 @@ import java.util.concurrent.TimeUnit; import java.util.function.Supplier; +import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos.ClientService; import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos.MutateRequest; @@ -58,6 +59,34 @@ public class MetricsConnection implements StatisticTrackable { /** Set this key to {@code true} to enable metrics collection of client requests. */ public static final String CLIENT_SIDE_METRICS_ENABLED_KEY = "hbase.client.metrics.enable"; + /** + * Set to specify a custom scope for the metrics published through {@link MetricsConnection}. + * The scope is added to JMX MBean objectName, and defaults to a combination of the Connection's + * clusterId and hashCode. For example, a default value for a connection to cluster "foo" might + * be "foo-7d9d0818", where "7d9d0818" is the hashCode of the underlying AsyncConnectionImpl. + * Users may set this key to give a more contextual name for this scope. For example, one might + * want to differentiate a read connection from a write connection by setting the scopes to + * "foo-read" and "foo-write" respectively. + * + * Scope is the only thing that lends any uniqueness to the metrics. Care should be taken to + * avoid using the same scope for multiple Connections, otherwise the metrics may aggregate in + * unforeseen ways. + */ + public static final String METRICS_SCOPE_KEY = "hbase.client.metrics.scope"; + + /** + * Returns the scope for a MetricsConnection based on the configured {@link #METRICS_SCOPE_KEY} + * or by generating a default from the passed clusterId and connectionObj's hashCode. + * @param conf configuration for the connection + * @param clusterId clusterId for the connection + * @param connectionObj either a Connection or AsyncConnectionImpl, the instance + * creating this MetricsConnection. + */ + static String getScope(Configuration conf, String clusterId, Object connectionObj) { + return conf.get(METRICS_SCOPE_KEY, + clusterId + "@" + Integer.toHexString(connectionObj.hashCode())); + } + private static final String CNT_BASE = "rpcCount_"; private static final String DRTN_BASE = "rpcCallDurationMs_"; private static final String REQ_BASE = "rpcCallRequestSizeBytes_"; @@ -252,7 +281,7 @@ private static interface NewMetric { private final MetricRegistry registry; private final JmxReporter reporter; - private final String scope; + protected final String scope; private final NewMetric timerFactory = new NewMetric() { @Override public Timer newMetric(Class clazz, String name, String scope) { diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java index d48806def23d..7abbbd0d72bc 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java @@ -23,9 +23,12 @@ import com.codahale.metrics.RatioGauge; import com.codahale.metrics.RatioGauge.Ratio; import java.io.IOException; +import java.util.Optional; import java.util.concurrent.Executors; import java.util.concurrent.ThreadPoolExecutor; +import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.security.User; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.MetricsTests; import org.apache.hadoop.hbase.testclassification.SmallTests; @@ -67,6 +70,25 @@ public static void afterClass() { METRICS.shutdown(); } + @Test + public void testMetricsConnectionScope() throws IOException { + Configuration conf = new Configuration(); + String clusterId = "foo"; + String scope = "testScope"; + conf.setBoolean(MetricsConnection.CLIENT_SIDE_METRICS_ENABLED_KEY, true); + + AsyncConnectionImpl impl = new AsyncConnectionImpl(conf, null, "foo", null, User.getCurrent()); + Optional metrics = impl.getConnectionMetrics(); + assertTrue("Metrics should be present", metrics.isPresent()); + assertEquals(clusterId + "@" + Integer.toHexString(impl.hashCode()), metrics.get().scope); + conf.set(MetricsConnection.METRICS_SCOPE_KEY, scope); + impl = new AsyncConnectionImpl(conf, null, "foo", null, User.getCurrent()); + + metrics = impl.getConnectionMetrics(); + assertTrue("Metrics should be present", metrics.isPresent()); + assertEquals(scope, metrics.get().scope); + } + @Test public void testStaticMetrics() throws IOException { final byte[] foo = Bytes.toBytes("foo"); From 81e16d9a9863b45c04b088152a974e7218d920c7 Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Tue, 12 Apr 2022 17:48:53 -0400 Subject: [PATCH 2/3] Add blocking client support --- .../client/ConnectionImplementation.java | 16 +++++----- .../hbase/client/TestMetricsConnection.java | 31 +++++++++++++++++-- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java index 2fc3d54d2209..61b5ab71c54c 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java @@ -308,13 +308,6 @@ public class ConnectionImplementation implements ClusterConnection, Closeable { this.rpcCallerFactory = RpcRetryingCallerFactory.instantiate(conf, interceptor, this.stats); this.backoffPolicy = ClientBackoffPolicyFactory.create(conf); this.asyncProcess = new AsyncProcess(this, conf, rpcCallerFactory, rpcControllerFactory); - if (conf.getBoolean(CLIENT_SIDE_METRICS_ENABLED_KEY, false)) { - this.metrics = - new MetricsConnection(this.toString(), this::getBatchPool, this::getMetaLookupPool); - } else { - this.metrics = null; - } - this.metaCache = new MetaCache(this.metrics); boolean shouldListen = conf.getBoolean(HConstants.STATUS_PUBLISHED, HConstants.STATUS_PUBLISHED_DEFAULT); @@ -333,6 +326,15 @@ public class ConnectionImplementation implements ClusterConnection, Closeable { } retrieveClusterId(); + if (conf.getBoolean(CLIENT_SIDE_METRICS_ENABLED_KEY, false)) { + String scope = MetricsConnection.getScope(conf, clusterId, this); + this.metrics = + new MetricsConnection(scope, this::getBatchPool, this::getMetaLookupPool); + } else { + this.metrics = null; + } + this.metaCache = new MetaCache(this.metrics); + this.rpcClient = RpcClientFactory.createClient(this.conf, this.clusterId, this.metrics); // Do we publish the status? diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java index 7abbbd0d72bc..2b1d9f369c18 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java @@ -18,12 +18,14 @@ package org.apache.hadoop.hbase.client; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import com.codahale.metrics.RatioGauge; import com.codahale.metrics.RatioGauge.Ratio; import java.io.IOException; import java.util.Optional; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executors; import java.util.concurrent.ThreadPoolExecutor; import org.apache.hadoop.conf.Configuration; @@ -50,6 +52,7 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos.ScanRequest; import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos.RegionSpecifier; import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos.RegionSpecifier.RegionSpecifierType; +import org.mockito.Mockito; @Category({ClientTests.class, MetricsTests.class, SmallTests.class}) public class TestMetricsConnection { @@ -71,24 +74,46 @@ public static void afterClass() { } @Test - public void testMetricsConnectionScope() throws IOException { + public void testMetricsConnectionScopeAsyncClient() throws IOException { Configuration conf = new Configuration(); String clusterId = "foo"; String scope = "testScope"; conf.setBoolean(MetricsConnection.CLIENT_SIDE_METRICS_ENABLED_KEY, true); - AsyncConnectionImpl impl = new AsyncConnectionImpl(conf, null, "foo", null, User.getCurrent()); + AsyncConnectionImpl impl = new AsyncConnectionImpl(conf, null, "foo", User.getCurrent()); Optional metrics = impl.getConnectionMetrics(); assertTrue("Metrics should be present", metrics.isPresent()); assertEquals(clusterId + "@" + Integer.toHexString(impl.hashCode()), metrics.get().scope); conf.set(MetricsConnection.METRICS_SCOPE_KEY, scope); - impl = new AsyncConnectionImpl(conf, null, "foo", null, User.getCurrent()); + impl = new AsyncConnectionImpl(conf, null, "foo", User.getCurrent()); metrics = impl.getConnectionMetrics(); assertTrue("Metrics should be present", metrics.isPresent()); assertEquals(scope, metrics.get().scope); } + @Test + public void testMetricsConnectionScopeBlockingClient() throws IOException { + Configuration conf = new Configuration(); + String clusterId = "foo"; + String scope = "testScope"; + conf.setBoolean(MetricsConnection.CLIENT_SIDE_METRICS_ENABLED_KEY, true); + + ConnectionRegistry mockRegistry = Mockito.mock(ConnectionRegistry.class); + Mockito.when(mockRegistry.getClusterId()).thenReturn(CompletableFuture.completedFuture(clusterId)); + + ConnectionImplementation impl = new ConnectionImplementation(conf, null, User.getCurrent(), mockRegistry); + MetricsConnection metrics = impl.getConnectionMetrics(); + assertNotNull("Metrics should be present", metrics); + assertEquals(clusterId + "@" + Integer.toHexString(impl.hashCode()), metrics.scope); + conf.set(MetricsConnection.METRICS_SCOPE_KEY, scope); + impl = new ConnectionImplementation(conf, null, User.getCurrent(), mockRegistry); + + metrics = impl.getConnectionMetrics(); + assertNotNull("Metrics should be present", metrics); + assertEquals(scope, metrics.scope); + } + @Test public void testStaticMetrics() throws IOException { final byte[] foo = Bytes.toBytes("foo"); From c1cc337a84808087ca04ec661030c90eae41d40c Mon Sep 17 00:00:00 2001 From: Bryan Beaudreault Date: Fri, 22 Apr 2022 10:17:45 -0400 Subject: [PATCH 3/3] checkstyle --- .../hadoop/hbase/client/AsyncConnectionImpl.java | 1 - .../hadoop/hbase/client/TestMetricsConnection.java | 11 +++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java index c1cdae61d36f..cbc3eaa2e460 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java @@ -24,7 +24,6 @@ import static org.apache.hadoop.hbase.client.ConnectionUtils.NO_NONCE_GENERATOR; import static org.apache.hadoop.hbase.client.ConnectionUtils.getStubKey; import static org.apache.hadoop.hbase.client.MetricsConnection.CLIENT_SIDE_METRICS_ENABLED_KEY; -import static org.apache.hadoop.hbase.client.MetricsConnection.METRICS_SCOPE_KEY; import static org.apache.hadoop.hbase.client.NonceGenerator.CLIENT_NONCES_ENABLED_KEY; import static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.SERVER_NAME_KEY; import static org.apache.hadoop.hbase.util.FutureUtils.addListener; diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java index 2b1d9f369c18..69d0389eb943 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestMetricsConnection.java @@ -20,7 +20,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; - import com.codahale.metrics.RatioGauge; import com.codahale.metrics.RatioGauge.Ratio; import java.io.IOException; @@ -40,9 +39,8 @@ import org.junit.ClassRule; import org.junit.Test; import org.junit.experimental.categories.Category; - +import org.mockito.Mockito; import org.apache.hbase.thirdparty.com.google.protobuf.ByteString; - import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil; import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos.ClientService; import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos.GetRequest; @@ -52,7 +50,6 @@ import org.apache.hadoop.hbase.shaded.protobuf.generated.ClientProtos.ScanRequest; import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos.RegionSpecifier; import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos.RegionSpecifier.RegionSpecifierType; -import org.mockito.Mockito; @Category({ClientTests.class, MetricsTests.class, SmallTests.class}) public class TestMetricsConnection { @@ -100,9 +97,11 @@ public void testMetricsConnectionScopeBlockingClient() throws IOException { conf.setBoolean(MetricsConnection.CLIENT_SIDE_METRICS_ENABLED_KEY, true); ConnectionRegistry mockRegistry = Mockito.mock(ConnectionRegistry.class); - Mockito.when(mockRegistry.getClusterId()).thenReturn(CompletableFuture.completedFuture(clusterId)); + Mockito.when(mockRegistry.getClusterId()) + .thenReturn(CompletableFuture.completedFuture(clusterId)); - ConnectionImplementation impl = new ConnectionImplementation(conf, null, User.getCurrent(), mockRegistry); + ConnectionImplementation impl = new ConnectionImplementation(conf, null, + User.getCurrent(), mockRegistry); MetricsConnection metrics = impl.getConnectionMetrics(); assertNotNull("Metrics should be present", metrics); assertEquals(clusterId + "@" + Integer.toHexString(impl.hashCode()), metrics.scope);