[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/1020


---

[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1020#discussion_r149994673
  
--- Diff: exec/java-exec/src/main/resources/rest/metrics/metrics.ftl ---
@@ -138,21 +154,14 @@
   });
 };
 
-function updateOthers(metrics) {
-  $.each(["counters", "meters"], function(i, key) {
-if(! $.isEmptyObject(metrics[key])) {
-  $("#" + key + "Val").html(JSON.stringify(metrics[key], null, 2));
-}
-  });
-};
-
 var update = function() {
   $.get("/status/metrics", function(metrics) {
 updateGauges(metrics.gauges);
 updateBars(metrics.gauges);
 if(! $.isEmptyObject(metrics.timers)) createTable(metrics.timers, 
"timers");
 if(! $.isEmptyObject(metrics.histograms)) 
createTable(metrics.histograms, "histograms");
-updateOthers(metrics);
+if(! $.isEmptyObject(metrics.counters)) 
createCountersTable(metrics.counters);
+if(! $.isEmptyObject(metrics.meters)) 
$("#metersVal").html(JSON.stringify(metrics.meters, null, 2));
--- End diff --

Well, sounds good then, thanks for making the changes.


---

[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table

[prasadns14]

Github user prasadns14 commented on a diff in the pull request:

https://github.com/apache/drill/pull/1020#discussion_r149972346
  
--- Diff: exec/java-exec/src/main/resources/rest/metrics/metrics.ftl ---
@@ -138,21 +154,14 @@
   });
 };
 
-function updateOthers(metrics) {
-  $.each(["counters", "meters"], function(i, key) {
-if(! $.isEmptyObject(metrics[key])) {
-  $("#" + key + "Val").html(JSON.stringify(metrics[key], null, 2));
-}
-  });
-};
-
 var update = function() {
   $.get("/status/metrics", function(metrics) {
 updateGauges(metrics.gauges);
 updateBars(metrics.gauges);
 if(! $.isEmptyObject(metrics.timers)) createTable(metrics.timers, 
"timers");
 if(! $.isEmptyObject(metrics.histograms)) 
createTable(metrics.histograms, "histograms");
-updateOthers(metrics);
+if(! $.isEmptyObject(metrics.counters)) 
createCountersTable(metrics.counters);
+if(! $.isEmptyObject(metrics.meters)) 
$("#metersVal").html(JSON.stringify(metrics.meters, null, 2));
--- End diff --

@arina-ielchiieva,
I have considered reusing existing methods before deciding to have a 
separate method.
With the above suggestion, the table will now look as below-

drill.connections.rpc.control.encrypted|  {count: 0}

'|' here is column delimiter. Do we want to display only the number in the 
second column or a key/value pair? 
I just wanted it to be consistent with the other metrics tables. (so I 
print value.count) 

Removed meters section. 


---

[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1020#discussion_r149891566
  
--- Diff: exec/java-exec/src/main/resources/rest/metrics/metrics.ftl ---
@@ -138,21 +154,14 @@
   });
 };
 
-function updateOthers(metrics) {
-  $.each(["counters", "meters"], function(i, key) {
-if(! $.isEmptyObject(metrics[key])) {
-  $("#" + key + "Val").html(JSON.stringify(metrics[key], null, 2));
-}
-  });
-};
-
 var update = function() {
   $.get("/status/metrics", function(metrics) {
 updateGauges(metrics.gauges);
 updateBars(metrics.gauges);
 if(! $.isEmptyObject(metrics.timers)) createTable(metrics.timers, 
"timers");
 if(! $.isEmptyObject(metrics.histograms)) 
createTable(metrics.histograms, "histograms");
-updateOthers(metrics);
+if(! $.isEmptyObject(metrics.counters)) 
createCountersTable(metrics.counters);
+if(! $.isEmptyObject(metrics.meters)) 
$("#metersVal").html(JSON.stringify(metrics.meters, null, 2));
--- End diff --

@prasadns14
1. Thanks for adding the screenshots.
2. Most of the code in `createTable` and `createCountersTable` coincide.  I 
suggested you make one function. For example, with three parameters, 
`createTable(metric, name, addReportingClass)`. When you don't need to add 
reporting class you'll call this method with false. Our goal here is generify 
existing methods rather then adding new specific with almost the same content.
3. If we don't have any meters, let's remove them.



---

[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table

[prasadns14]

Github user prasadns14 commented on a diff in the pull request:

https://github.com/apache/drill/pull/1020#discussion_r149881450
  
--- Diff: exec/java-exec/src/main/resources/rest/metrics/metrics.ftl ---
@@ -138,21 +154,14 @@
   });
 };
 
-function updateOthers(metrics) {
-  $.each(["counters", "meters"], function(i, key) {
-if(! $.isEmptyObject(metrics[key])) {
-  $("#" + key + "Val").html(JSON.stringify(metrics[key], null, 2));
-}
-  });
-};
-
 var update = function() {
   $.get("/status/metrics", function(metrics) {
 updateGauges(metrics.gauges);
 updateBars(metrics.gauges);
 if(! $.isEmptyObject(metrics.timers)) createTable(metrics.timers, 
"timers");
 if(! $.isEmptyObject(metrics.histograms)) 
createTable(metrics.histograms, "histograms");
-updateOthers(metrics);
+if(! $.isEmptyObject(metrics.counters)) 
createCountersTable(metrics.counters);
+if(! $.isEmptyObject(metrics.meters)) 
$("#metersVal").html(JSON.stringify(metrics.meters, null, 2));
--- End diff --

@arina-ielchiieva, please review

1) Added three screenshots before (current behavior), snapshot1(reusing 
createTable function) and snapshot2 (new createCountersTable function)

2) The function "createTable" lists the keys for each metric class, which 
looks good for "timers" and "histograms" as they have many keys. But the 
"counters" metric has only one key "count" (same as guages metric which has 
only key value). So, I have a added a new function to just list the class and 
count value.

3) Currently we do not have any "meters" metric. So, I don't know how many 
keys "meters" metric will have. If we have multiple keys we can use createTable 
or else we may have to create a different function similar to 
createCountersTable. (depending on the key name)

I personally think if we have a single key, we shouldn't use the 
createTable function. We should just display the class and the only key value.

https://user-images.githubusercontent.com/4907022/32592926-b419532e-c4da-11e7-93f1-02514e51e28d.png;>
https://user-images.githubusercontent.com/4907022/32592927-b42e1b38-c4da-11e7-850e-32902b6eb6d0.png;>
https://user-images.githubusercontent.com/4907022/32592928-b4438b30-c4da-11e7-97c8-edc0482b1924.png;>






---

[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/1020#discussion_r149349985
  
--- Diff: exec/java-exec/src/main/resources/rest/metrics/metrics.ftl ---
@@ -138,21 +154,14 @@
   });
 };
 
-function updateOthers(metrics) {
-  $.each(["counters", "meters"], function(i, key) {
-if(! $.isEmptyObject(metrics[key])) {
-  $("#" + key + "Val").html(JSON.stringify(metrics[key], null, 2));
-}
-  });
-};
-
 var update = function() {
   $.get("/status/metrics", function(metrics) {
 updateGauges(metrics.gauges);
 updateBars(metrics.gauges);
 if(! $.isEmptyObject(metrics.timers)) createTable(metrics.timers, 
"timers");
 if(! $.isEmptyObject(metrics.histograms)) 
createTable(metrics.histograms, "histograms");
-updateOthers(metrics);
+if(! $.isEmptyObject(metrics.counters)) 
createCountersTable(metrics.counters);
+if(! $.isEmptyObject(metrics.meters)) 
$("#metersVal").html(JSON.stringify(metrics.meters, null, 2));
--- End diff --

@prasadns14 
1. Please add two screenshots before and after the changes.
2. Can you please think of the way to make create table generic so can be 
used for timers, histograms and counters?
3. What about meters? How they are displayed right now? Maybe we need to 
display them in table as well?
Ideally, we can display all metrics in the same way.


---

[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table

[prasadns14]

GitHub user prasadns14 opened a pull request:

https://github.com/apache/drill/pull/1020

DRILL-5921: Display counter metrics in table

Listed the counter metrics in a table
@arina-ielchiieva please review

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/prasadns14/drill DRILL-5921

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/1020.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1020


commit 3f55847fe93d3296740901f717b4e0cf50736311
Author: Prasad Nagaraj Subramanya 
Date:   2017-11-02T01:59:38Z

DRILL-5921: Display counter metrics in table




---