[GitHub] [accumulo] ddanielr commented on a diff in pull request #3297: fix metrics tags, tserver hostname, remove tags from thrift metrics

["ddanielr (via GitHub)" <gi...@apache.org>]

ddanielr commented on code in PR #3297:
URL: https://github.com/apache/accumulo/pull/3297#discussion_r1167357351


##########
test/src/main/java/org/apache/accumulo/test/metrics/MetricsIT.java:
##########
@@ -158,4 +174,25 @@ public void registerMetrics(MeterRegistry registry) {
     // unused; this class only extends MetricsProducer to easily reference its methods/constants
   }
 
+  @Test
+  public void metricTags() throws Exception {
+
+    doWorkToGenerateMetrics();
+    cluster.stop();
+
+    List<String> statsDMetrics;
+
+    // loop until we run out of lines or until we see all expected metrics
+    while (!(statsDMetrics = sink.getLines()).isEmpty()) {
+      statsDMetrics.stream().filter(line -> line.startsWith("accumulo"))
+          .map(TestStatsDSink::parseStatsDMetric).forEach(a -> {
+            var t = a.getTags();
+            log.trace("METRICS, name: '{}' num tags: {}, tags: {}", a.getName(), t.size(), t);
+            // check hostname is always set and is valid
+            assertNotEquals("0.0.0.0", a.getTags().get("host"));
+            // check the length of the tag value is sane
+            a.getTags().forEach((k, v) -> assertTrue(v.length() < 128));

Review Comment:
   Nit: Use a named variable like `maxTagLength=128` value for readability and future troubleshooting of test failures. 
   



##########
core/src/main/java/org/apache/accumulo/core/metrics/MetricsUtil.java:
##########
@@ -79,9 +79,14 @@ private static void initializeMetrics(boolean enabled, boolean jvmMetricsEnabled
       List<Tag> tags = new ArrayList<>();
       tags.add(Tag.of("process.name", processName));
 
-      if (address != null) {
-        tags.add(Tag.of("Address", address.toString()));
+      if (address != null && !address.getHost().isEmpty()) {
+        tags.add(Tag.of("host", address.getHost()));
       }
+
+      if (address != null && address.getPort() > 0) {
+        tags.add(Tag.of("port", Integer.toString(address.getPort())));
+      }
+

Review Comment:
   Use a nested if statement since both checks rely on the same primary condition. 
   
   ```suggestion
         if (address != null) {
           if (!address.getHost().isEmpty()) {
             tags.add(Tag.of("host", address.getHost()));
           }
           if (address.getPort() > 0) {
             tags.add(Tag.of("port", Integer.toString(address.getPort())));
           }
        } 
   ```



-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org