You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by prasadns14 <gi...@git.apache.org> on 2017/11/02 02:47:09 UTC
[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table
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 <pr...@gmail.com>
Date: 2017-11-02T01:59:38Z
DRILL-5921: Display counter metrics in table
----
---
[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table
Posted by prasadns14 <gi...@git.apache.org>.
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
Posted by prasadns14 <gi...@git.apache.org>.
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.
<img width="1068" alt="before" src="https://user-images.githubusercontent.com/4907022/32592926-b419532e-c4da-11e7-93f1-02514e51e28d.png">
<img width="961" alt="snapshot1" src="https://user-images.githubusercontent.com/4907022/32592927-b42e1b38-c4da-11e7-850e-32902b6eb6d0.png">
<img width="1078" alt="snapshot2" src="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
Posted by arina-ielchiieva <gi...@git.apache.org>.
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
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1020
---
[GitHub] drill issue #1020: DRILL-5921: Display counter metrics in table
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1020
+1, LGTM.
---
[GitHub] drill pull request #1020: DRILL-5921: Display counter metrics in table
Posted by arina-ielchiieva <gi...@git.apache.org>.
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
Posted by arina-ielchiieva <gi...@git.apache.org>.
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.
---