DRILL-5921: Display counter metrics in table #1020
Conversation
| 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)); |
There was a problem hiding this comment.
- Please add two screenshots before and after the changes.
- Can you please think of the way to make create table generic so can be used for timers, histograms and counters?
- 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.
There was a problem hiding this comment.
@arina-ielchiieva, please review
-
Added three screenshots before (current behavior), snapshot1(reusing createTable function) and snapshot2 (new createCountersTable function)
-
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.
-
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.
There was a problem hiding this comment.
- Thanks for adding the screenshots.
- Most of the code in
createTableandcreateCountersTablecoincide. 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. - If we don't have any meters, let's remove them.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Well, sounds good then, thanks for making the changes.
prasadns14
force-pushed
the
DRILL-5921
branch
from
3f55847
to
5987ec4
Compare
|
+1, LGTM. |
Listed the counter metrics in a table
@arina-ielchiieva please review