From d1d84ac42fd430d5ba78cb5be2d8984d3af7f507 Mon Sep 17 00:00:00 2001 From: Victor Li Date: Mon, 19 Dec 2022 17:49:26 -0800 Subject: [PATCH 1/3] HBASE-27540: add client side counter metrics for failed rpc calls. --- .../hbase/client/MetricsConnection.java | 5 ++- .../hadoop/hbase/ipc/AbstractRpcClient.java | 7 +++-- .../hbase/client/TestMetricsConnection.java | 31 +++++++++++++------ 3 files changed, 30 insertions(+), 13 deletions(-) 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 49f4e9ba0a5d..2cf6e0d99058 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 @@ -637,7 +637,7 @@ private void shutdown() { } /** Report RPC context to metrics system. */ - public void updateRpc(MethodDescriptor method, Message param, CallStats stats) { + public void updateRpc(MethodDescriptor method, Message param, CallStats stats, boolean failed) { int callsPerServer = stats.getConcurrentCallsPerServer(); if (callsPerServer > 0) { concurrentCallsPerServerHist.update(callsPerServer); @@ -645,6 +645,9 @@ public void updateRpc(MethodDescriptor method, Message param, CallStats stats) { // Update the counter that tracks RPCs by type. final String methodName = method.getService().getName() + "_" + method.getName(); getMetric(CNT_BASE + methodName, rpcCounters, counterFactory).inc(); + if (failed) { + getMetric(CNT_BASE + methodName + "_Failure", rpcCounters, counterFactory).inc(); + } // this implementation is tied directly to protobuf implementation details. would be better // if we could dispatch based on something static, ie, request Message type. if (method.getService() == ClientService.getDescriptor()) { diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java index 0afd07d79513..6459ecf0d2a9 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java @@ -376,11 +376,12 @@ private void onCallFinished(Call call, HBaseRpcController hrc, Address addr, RpcCallback callback) { call.callStats.setCallTimeMs(EnvironmentEdgeManager.currentTime() - call.getStartTime()); if (metrics != null) { - metrics.updateRpc(call.md, call.param, call.callStats); + metrics.updateRpc(call.md, call.param, call.callStats, (call.error != null) ? true : false); } if (LOG.isTraceEnabled()) { - LOG.trace("CallId: {}, call: {}, startTime: {}ms, callTime: {}ms", call.id, call.md.getName(), - call.getStartTime(), call.callStats.getCallTimeMs()); + LOG.trace("CallId: {}, call: {}, startTime: {}ms, callTime: {}ms, result: {}", + call.id, call.md.getName(), call.getStartTime(), call.callStats.getCallTimeMs(), + (call.error != null) ? "failed" : "successful"); } if (call.error != null) { if (call.error instanceof RemoteException) { 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 01b3ad549955..f98b3836902f 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,6 +20,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import com.codahale.metrics.Counter; import com.codahale.metrics.RatioGauge; import com.codahale.metrics.RatioGauge.Ratio; import java.io.IOException; @@ -149,37 +150,49 @@ public void testStaticMetrics() throws IOException { for (int i = 0; i < loop; i++) { METRICS.updateRpc(ClientService.getDescriptor().findMethodByName("Get"), - GetRequest.getDefaultInstance(), MetricsConnection.newCallStats()); + GetRequest.getDefaultInstance(), MetricsConnection.newCallStats(), false); METRICS.updateRpc(ClientService.getDescriptor().findMethodByName("Scan"), - ScanRequest.getDefaultInstance(), MetricsConnection.newCallStats()); + ScanRequest.getDefaultInstance(), MetricsConnection.newCallStats(), false); METRICS.updateRpc(ClientService.getDescriptor().findMethodByName("Multi"), - MultiRequest.getDefaultInstance(), MetricsConnection.newCallStats()); + MultiRequest.getDefaultInstance(), MetricsConnection.newCallStats(), true); METRICS.updateRpc(ClientService.getDescriptor().findMethodByName("Mutate"), MutateRequest.newBuilder() .setMutation(ProtobufUtil.toMutation(MutationType.APPEND, new Append(foo))) .setRegion(region).build(), - MetricsConnection.newCallStats()); + MetricsConnection.newCallStats(), false); METRICS.updateRpc(ClientService.getDescriptor().findMethodByName("Mutate"), MutateRequest.newBuilder() .setMutation(ProtobufUtil.toMutation(MutationType.DELETE, new Delete(foo))) .setRegion(region).build(), - MetricsConnection.newCallStats()); + MetricsConnection.newCallStats(), false); METRICS.updateRpc(ClientService.getDescriptor().findMethodByName("Mutate"), MutateRequest.newBuilder() .setMutation(ProtobufUtil.toMutation(MutationType.INCREMENT, new Increment(foo))) .setRegion(region).build(), - MetricsConnection.newCallStats()); + MetricsConnection.newCallStats(), false); METRICS.updateRpc(ClientService.getDescriptor().findMethodByName("Mutate"), MutateRequest.newBuilder() .setMutation(ProtobufUtil.toMutation(MutationType.PUT, new Put(foo))).setRegion(region) .build(), - MetricsConnection.newCallStats()); + MetricsConnection.newCallStats(), false); } + final String rpcCountPrefix = "rpcCount_" + ClientService.getDescriptor().getName() + "_"; + String metricKey; + long metricVal; + Counter counter; for (String method : new String[] { "Get", "Scan", "Mutate" }) { - final String metricKey = "rpcCount_" + ClientService.getDescriptor().getName() + "_" + method; - final long metricVal = METRICS.getRpcCounters().get(metricKey).getCount(); + metricKey = rpcCountPrefix + method; + metricVal = METRICS.getRpcCounters().get(metricKey).getCount(); assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal >= loop); + metricKey += "_Failure"; + counter = METRICS.getRpcCounters().get(metricKey); + metricVal = (counter != null) ? counter.getCount() : 0; + assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal == 0); } + metricKey = rpcCountPrefix + "Multi" + "_Failure"; + counter = METRICS.getRpcCounters().get(metricKey); + metricVal = (counter != null) ? counter.getCount() : 0; + assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal == loop); for (MetricsConnection.CallTracker t : new MetricsConnection.CallTracker[] { METRICS.getGetTracker(), METRICS.getScanTracker(), METRICS.getMultiTracker(), METRICS.getAppendTracker(), METRICS.getDeleteTracker(), METRICS.getIncrementTracker(), From 687b71cd9866b3e0b5f1dc7ee0b91531c87f8f7a Mon Sep 17 00:00:00 2001 From: Victor Li Date: Mon, 19 Dec 2022 19:41:55 -0800 Subject: [PATCH 2/3] Update the failed client rpc count metric names with rpcFailureCount_. --- .../apache/hadoop/hbase/client/MetricsConnection.java | 3 ++- .../org/apache/hadoop/hbase/ipc/AbstractRpcClient.java | 9 +++++---- .../hadoop/hbase/client/TestMetricsConnection.java | 9 ++++++--- 3 files changed, 13 insertions(+), 8 deletions(-) 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 2cf6e0d99058..75dc7aea27ce 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 @@ -117,6 +117,7 @@ static String getScope(Configuration conf, String clusterId, Object connectionOb } private static final String CNT_BASE = "rpcCount_"; + private static final String FAILURE_CNT_BASE = "rpcFailureCount_"; private static final String DRTN_BASE = "rpcCallDurationMs_"; private static final String REQ_BASE = "rpcCallRequestSizeBytes_"; private static final String RESP_BASE = "rpcCallResponseSizeBytes_"; @@ -646,7 +647,7 @@ public void updateRpc(MethodDescriptor method, Message param, CallStats stats, b final String methodName = method.getService().getName() + "_" + method.getName(); getMetric(CNT_BASE + methodName, rpcCounters, counterFactory).inc(); if (failed) { - getMetric(CNT_BASE + methodName + "_Failure", rpcCounters, counterFactory).inc(); + getMetric(FAILURE_CNT_BASE + methodName, rpcCounters, counterFactory).inc(); } // this implementation is tied directly to protobuf implementation details. would be better // if we could dispatch based on something static, ie, request Message type. diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java index 6459ecf0d2a9..e563c6cbde27 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java @@ -375,15 +375,16 @@ private T getConnection(ConnectionId remoteId) throws IOException { private void onCallFinished(Call call, HBaseRpcController hrc, Address addr, RpcCallback callback) { call.callStats.setCallTimeMs(EnvironmentEdgeManager.currentTime() - call.getStartTime()); + final boolean failed = (call.error != null) ? true : false; if (metrics != null) { - metrics.updateRpc(call.md, call.param, call.callStats, (call.error != null) ? true : false); + metrics.updateRpc(call.md, call.param, call.callStats, failed); } if (LOG.isTraceEnabled()) { - LOG.trace("CallId: {}, call: {}, startTime: {}ms, callTime: {}ms, result: {}", + LOG.trace("CallId: {}, call: {}, startTime: {}ms, callTime: {}ms, status: {}", call.id, call.md.getName(), call.getStartTime(), call.callStats.getCallTimeMs(), - (call.error != null) ? "failed" : "successful"); + failed ? "failed" : "successful"); } - if (call.error != null) { + if (failed) { if (call.error instanceof RemoteException) { call.error.fillInStackTrace(); hrc.setFailed(call.error); 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 f98b3836902f..17f32045d55f 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 @@ -176,7 +176,10 @@ public void testStaticMetrics() throws IOException { .build(), MetricsConnection.newCallStats(), false); } - final String rpcCountPrefix = "rpcCount_" + ClientService.getDescriptor().getName() + "_"; + final String rpcCountPrefix = + "rpcCount_" + ClientService.getDescriptor().getName() + "_"; + final String rpcFailureCountPrefix = + "rpcFailureCount_" + ClientService.getDescriptor().getName() + "_"; String metricKey; long metricVal; Counter counter; @@ -184,12 +187,12 @@ public void testStaticMetrics() throws IOException { metricKey = rpcCountPrefix + method; metricVal = METRICS.getRpcCounters().get(metricKey).getCount(); assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal >= loop); - metricKey += "_Failure"; + metricKey = rpcFailureCountPrefix + method; counter = METRICS.getRpcCounters().get(metricKey); metricVal = (counter != null) ? counter.getCount() : 0; assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal == 0); } - metricKey = rpcCountPrefix + "Multi" + "_Failure"; + metricKey = rpcFailureCountPrefix + "Multi"; counter = METRICS.getRpcCounters().get(metricKey); metricVal = (counter != null) ? counter.getCount() : 0; assertTrue("metric: " + metricKey + " val: " + metricVal, metricVal == loop); From 694a18b8fa21af5dfe204916244cf915eabef76d Mon Sep 17 00:00:00 2001 From: Victor Li Date: Tue, 20 Dec 2022 08:12:16 -0800 Subject: [PATCH 3/3] run spotless:apply. --- .../java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java | 4 ++-- .../org/apache/hadoop/hbase/client/TestMetricsConnection.java | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java index e563c6cbde27..8b56406ed58e 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java @@ -380,8 +380,8 @@ private void onCallFinished(Call call, HBaseRpcController hrc, Address addr, metrics.updateRpc(call.md, call.param, call.callStats, failed); } if (LOG.isTraceEnabled()) { - LOG.trace("CallId: {}, call: {}, startTime: {}ms, callTime: {}ms, status: {}", - call.id, call.md.getName(), call.getStartTime(), call.callStats.getCallTimeMs(), + LOG.trace("CallId: {}, call: {}, startTime: {}ms, callTime: {}ms, status: {}", call.id, + call.md.getName(), call.getStartTime(), call.callStats.getCallTimeMs(), failed ? "failed" : "successful"); } if (failed) { 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 17f32045d55f..edc846754f6f 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 @@ -176,8 +176,7 @@ public void testStaticMetrics() throws IOException { .build(), MetricsConnection.newCallStats(), false); } - final String rpcCountPrefix = - "rpcCount_" + ClientService.getDescriptor().getName() + "_"; + final String rpcCountPrefix = "rpcCount_" + ClientService.getDescriptor().getName() + "_"; final String rpcFailureCountPrefix = "rpcFailureCount_" + ClientService.getDescriptor().getName() + "_"; String metricKey;