Migrate Scan Server monitor page to use backend-derived DTO#6165
Migrate Scan Server monitor page to use backend-derived DTO#6165DomGarguilo merged 7 commits intoapache:mainfrom
Conversation
dlmarion
left a comment
dlmarion
left a comment
There was a problem hiding this comment.
I like the direction this is going, but I think we can pay the computation cost once instead of on page refresh for every user of the Monitor. Also, since we are working on optimization, there is an option to return the JSON from the server in CBOR encoding. I have done this in another project, here, and Jackson already supports it. We would need to decode the response in the UI javascript, but I'm sure that's already supported.
server/monitor/src/main/java/org/apache/accumulo/monitor/next/Endpoints.java
Show resolved
Hide resolved
server/monitor/src/main/java/org/apache/accumulo/monitor/next/Endpoints.java
Outdated
Show resolved
Hide resolved
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/navbar.js
Show resolved
Hide resolved
I think CBOR encoding would reduce the payload size for each bundle of information sent to the monitor frontend but I'm not sure the extra complexity is worth it here. The payloads are already pretty small. I do think your other commend about precomputing/caching this new DTO in some capacity would be worth it. I think I will work on adding that to this PR. |
server/monitor/src/main/java/org/apache/accumulo/monitor/next/SystemInformation.java
Outdated
Show resolved
Hide resolved
| private static Map<String,Number> metricValuesByName(MetricResponse response) { | ||
| var values = new HashMap<String,Number>(); | ||
| if (response == null || response.getMetrics() == null || response.getMetrics().isEmpty()) { | ||
| return values; | ||
| } | ||
|
|
||
| for (var binary : response.getMetrics()) { | ||
| var metric = FMetric.getRootAsFMetric(binary); | ||
| var metricStatistic = extractStatistic(metric); | ||
| if (metricStatistic == null || metricStatistic.equals("value") | ||
| || metricStatistic.equals("count")) { | ||
| values.putIfAbsent(metric.name(), metricNumericValue(metric)); | ||
| } | ||
| } | ||
| return values; | ||
| } | ||
|
|
||
| private static String extractStatistic(FMetric metric) { | ||
| for (int i = 0; i < metric.tagsLength(); i++) { | ||
| FTag tag = metric.tags(i); | ||
| if (MetricResponseWrapper.STATISTIC_TAG.equals(tag.key())) { | ||
| return normalizeStatistic(tag.value()); | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static String normalizeStatistic(String statistic) { | ||
| if (statistic == null) { | ||
| return null; | ||
| } | ||
| return statistic.toLowerCase(); | ||
| } | ||
|
|
||
| private static Number metricNumericValue(FMetric metric) { | ||
| if (metric.ivalue() != 0) { | ||
| return metric.ivalue(); | ||
| } | ||
| if (metric.lvalue() != 0L) { | ||
| return metric.lvalue(); | ||
| } | ||
| if (metric.dvalue() != 0.0d) { | ||
| return metric.dvalue(); | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
If we are going to create other PRs to create simpler JSON on the server side, then we likely will want to move this to a FMetricUtil (or some other name) class.
3 participants
This PR replaces the complex javascript parsing of raw metrics with a backend-built Data Transfer Object (DTO) which is consumed by the ScanServer monitor page UI. This is a refactor/improvement pass only and does not include any functional changes.