PDavid commented on code in PR #8058:
URL: https://github.com/apache/hbase/pull/8058#discussion_r3257813273
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsUserAggregateImpl.java:
##########
@@ -193,4 +204,51 @@ private MetricsUserSource getOrCreateMetricsUser(String
user) {
userMetricLossyCounting.add(userSource);
return userSource;
}
+
+ private ClientConnectionContext getClientConnectionContext() {
+ String hostAddress = null;
+ String userName = "Unknown";
+ String clientVersion = "Unknown";
+ String serviceName = "Unknown";
+
+ Optional<ServerRpcConnection> rpcConnectionOptional =
RpcServer.getCurrentServerRpcConnection();
+ if (rpcConnectionOptional.isPresent()) {
+ ServerRpcConnection rpcConnection = rpcConnectionOptional.get();
+ hostAddress = rpcConnection.getLocalHostAddress();
Review Comment:
The field `localHostAddress` in `ServerRpcConnection` represents the
server-side IP (the RS's own address), but it's surfaced in the UI under the
column "ClientIP" (via `getHostAddress()`).
This is confusing, the value shown as "ClientIP" is actually the region
server's local bind address, not the client's IP. Shouldn't we use
`hostAddress` from `ServerRpcConnection` here instead?
##########
hbase-server/src/main/resources/hbase-webapps/master/regionServerListClientConnectionsStats.jsp:
##########
@@ -0,0 +1,54 @@
+<%@ page contentType="text/html;charset=UTF-8"
+ import="org.apache.hadoop.hbase.ServerName"
+ import="org.apache.hadoop.hbase.master.HMaster"
+ import="org.apache.hadoop.hbase.ServerMetrics"
+ import="org.apache.hadoop.hbase.master.ServerManager"
+ import="org.apache.hadoop.hbase.UserMetrics"
+ import="java.util.Map" %>
+
+<%
+ HMaster master = (HMaster) getServletContext().getAttribute(HMaster.MASTER);
+ ServerName[] serverNames = (ServerName[])
request.getAttribute("serverNames");
+ ServerManager serverManager = master.getServerManager();
+%>
+
+<table id="clientConnectionsStatsTable" class="tablesorter table
table-striped">
+ <thead>
+ <tr>
+ <th class="cls_separator">ClientIP</th>
+ <th class="cls_separator">UserName</th>
+ <th class="cls_separator">ClientVersion</th>
+ <th class="cls_separator">ServiceName</th>
+ <th class="cls_separator">ServerInfo</th>
+ </tr>
+ </thead>
+ <tbody>
+ <%
+ for (ServerName serverName: serverNames) {
+ ServerMetrics serverMetrics = serverManager.getLoad(serverName);
+ if(serverMetrics != null) {
+
+ Map<byte[], UserMetrics> userMetricsMap =
serverMetrics.getUserMetrics();
+ for(Map.Entry<byte[], UserMetrics> entry : userMetricsMap.entrySet()) {
+ UserMetrics userMetrics = entry.getValue();
+ Map<String, UserMetrics.ClientMetrics> clientMetricsMap =
userMetrics.getClientMetrics();
+
+ for(Map.Entry<String, UserMetrics.ClientMetrics> clientEntry :
clientMetricsMap.entrySet()) {
+ UserMetrics.ClientMetrics clientConnection =
clientEntry.getValue();
+
+ %>
+ <tr>
+ <td><%= clientConnection.getHostAddress() %></td>
+ <td><%= clientConnection.getUserName() %></td>
+ <td><%= clientConnection.getClientVersion() %></td>
+ <td><%= clientConnection.getServiceName() %></td>
+ <td><%= serverName.getServerName() %></td>cc
Review Comment:
Here there's a stray `cc` after the closing </td> tag that will render as
visible text in the HTML.
```suggestion
<td><%= serverName.getServerName() %></td>
```
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MetricsUserAggregateImpl.java:
##########
@@ -23,10 +23,15 @@
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.CompatibilitySingletonFactory;
import org.apache.hadoop.hbase.ipc.RpcServer;
+import org.apache.hadoop.hbase.ipc.ServerRpcConnection;
import org.apache.hadoop.hbase.security.User;
import org.apache.hadoop.hbase.security.UserProvider;
import org.apache.hadoop.hbase.util.LossyCounting;
import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
Review Comment:
These are unused imports. Can we remove them?
##########
hbase-server/src/main/resources/hbase-webapps/master/regionServerListClientConnectionsStats.jsp:
##########
@@ -0,0 +1,54 @@
+<%@ page contentType="text/html;charset=UTF-8"
+ import="org.apache.hadoop.hbase.ServerName"
+ import="org.apache.hadoop.hbase.master.HMaster"
+ import="org.apache.hadoop.hbase.ServerMetrics"
+ import="org.apache.hadoop.hbase.master.ServerManager"
+ import="org.apache.hadoop.hbase.UserMetrics"
+ import="java.util.Map" %>
+
+<%
+ HMaster master = (HMaster) getServletContext().getAttribute(HMaster.MASTER);
+ ServerName[] serverNames = (ServerName[])
request.getAttribute("serverNames");
+ ServerManager serverManager = master.getServerManager();
+%>
+
+<table id="clientConnectionsStatsTable" class="tablesorter table
table-striped">
+ <thead>
+ <tr>
+ <th class="cls_separator">ClientIP</th>
+ <th class="cls_separator">UserName</th>
+ <th class="cls_separator">ClientVersion</th>
+ <th class="cls_separator">ServiceName</th>
+ <th class="cls_separator">ServerInfo</th>
+ </tr>
+ </thead>
+ <tbody>
+ <%
+ for (ServerName serverName: serverNames) {
+ ServerMetrics serverMetrics = serverManager.getLoad(serverName);
+ if(serverMetrics != null) {
+
+ Map<byte[], UserMetrics> userMetricsMap =
serverMetrics.getUserMetrics();
+ for(Map.Entry<byte[], UserMetrics> entry : userMetricsMap.entrySet()) {
+ UserMetrics userMetrics = entry.getValue();
+ Map<String, UserMetrics.ClientMetrics> clientMetricsMap =
userMetrics.getClientMetrics();
+
+ for(Map.Entry<String, UserMetrics.ClientMetrics> clientEntry :
clientMetricsMap.entrySet()) {
+ UserMetrics.ClientMetrics clientConnection =
clientEntry.getValue();
+
+ %>
+ <tr>
+ <td><%= clientConnection.getHostAddress() %></td>
+ <td><%= clientConnection.getUserName() %></td>
+ <td><%= clientConnection.getClientVersion() %></td>
+ <td><%= clientConnection.getServiceName() %></td>
Review Comment:
Here we directly interpolate values from these client-controlled fields
(`userName`, `clientVersion`, `serviceName`) using `<%= ... %>` without HTML
escaping. Malicious or buggy clients could inject `<script>` tags. Other HBase
JSPs use `StringEscapeUtils.escapeHtml4()` - the same should be applied here.
##########
hbase-protocol-shaded/src/main/protobuf/server/ClusterStatus.proto:
##########
@@ -212,6 +206,11 @@ message ClientMetrics {
/** the current total filtered requests made from a client */
optional uint64 filtered_requests_count = 4;
+
+ optional string host_address = 5;
+ optional string user_name = 6;
+ optional string client_version = 7;
+ optional string service_name = 8;
Review Comment:
Other fields are all having comments. Would it make sense to add comments
for these new fields as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
