-
Notifications
You must be signed in to change notification settings - Fork 468
Manager info on Monitor rest/manager -> rest-v2/manager
#5894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Manager info on the Monitor (displayed on homepage (`/`) and Manager page (`/manager`)) now obtains and displays info from the existing (but unused) `rest-v2/manager` endpoint. * Updated the homepage to display table data corresponding to the `rest-v2/manager` endpoint. * Updated the manager page to display table data corresponding to the `rest-v2/manager` endpoint. * Created new endpoint `rest-v2/manager/metrics` which returns the Manager metrics as json. This is linked in the Manager table in both the homepage and the manager page. * Server navigation bar now only considers the Manager status (as obtained from `rest/status`) (ERROR, WARN, OK). Previously took into account the manager state (e.g., if the state or goal state was SAFE_MODE or CLEAN_STOP), but this info was only included in the `rest/manager` endpoint. * Deleted the `rest/manager` endpoint and associated code used to gather data for this endpoint. * Deleted `systemAlert.js` and `systemAlert.ftl` as this alert was entirely based on Manager info included in `rest/manager` (manager state being SAFE_MODE or CLEAN_STOP). No longer applicable with `rest-v2/manager`.
| * @since 2.0.0 | ||
| */ | ||
| @XmlRootElement(name = "stats") | ||
| public class SummaryInformation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is used for summarizing Monitor info at /xml and /json. Maybe there's info in rest-v2/manager that we would want to include in the summary. For now, I just deleted all the rest/manager related data.
| // show the manager error banner and hide manager table | ||
| $('#managerRunningBanner').show(); | ||
| $('#managerStatusTable').hide(); | ||
| } else { | ||
| // otherwise, hide the error banner and show manager table | ||
| $('#managerRunningBanner').hide(); | ||
| $('#managerStatusTable').show(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
managerStatus_wrapper does not exist (preexisting bug). Fixed here
| getManager().then(function () { | ||
| const managerData = JSON.parse(sessionStorage.manager); | ||
| const managerState = managerData.managerState; | ||
| const managerGoalState = managerData.managerGoalState; | ||
|
|
||
| const isStateGoalSame = managerState === managerGoalState; | ||
|
|
||
| // if the manager state is normal and the goal state is the same as the current state, | ||
| // or of the manager is not running, hide the state banner and return early | ||
| if ((managerState === 'NORMAL' && isStateGoalSame) || managerState === null) { | ||
| $('#managerStateBanner').hide(); | ||
| return; | ||
| } | ||
|
|
||
| // update the manager state banner message and show it | ||
| let bannerMessage = 'Manager state: ' + managerState; | ||
| if (!isStateGoalSame) { | ||
| // only show the goal state if it differs from the manager's current state | ||
| bannerMessage += '. Manager goal state: ' + managerGoalState; | ||
| } | ||
| $('#manager-banner-message').text(bannerMessage); | ||
| $('#managerStateBanner').show(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer applicable as the rest-v2/manager does not contain state of the manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add Metrics for the Manager.ManagerState enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#5894 (comment) (only first part relevant)
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/functions.js
Show resolved
Hide resolved
|
Ran locally. The manager pages worked but I could never get metric information to display at |
|
Enabled auto-refresh and did get an error message |
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/js/navbar.js
Show resolved
Hide resolved
| return monitor.getInformationFetcher().getAllMetrics().asMap().get(s); | ||
| } | ||
|
|
||
| @GET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The metrics are already returned as part of the MetricResponse json from the getManager call. Just wondering if we really need this, it seems duplicative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this so we can easily link to just the "metrics" key of the "rest-v2/manager" json endpoint via an href:
<a href=\"rest-v2/manager/metrics\">Metrics</a>
Not sure of a way to easily do this without more custom javascript/html, and I think the extra Java code is more maintainable than more js/html
| getManager().then(function () { | ||
| const managerData = JSON.parse(sessionStorage.manager); | ||
| const managerState = managerData.managerState; | ||
| const managerGoalState = managerData.managerGoalState; | ||
|
|
||
| const isStateGoalSame = managerState === managerGoalState; | ||
|
|
||
| // if the manager state is normal and the goal state is the same as the current state, | ||
| // or of the manager is not running, hide the state banner and return early | ||
| if ((managerState === 'NORMAL' && isStateGoalSame) || managerState === null) { | ||
| $('#managerStateBanner').hide(); | ||
| return; | ||
| } | ||
|
|
||
| // update the manager state banner message and show it | ||
| let bannerMessage = 'Manager state: ' + managerState; | ||
| if (!isStateGoalSame) { | ||
| // only show the goal state if it differs from the manager's current state | ||
| bannerMessage += '. Manager goal state: ' + managerGoalState; | ||
| } | ||
| $('#manager-banner-message').text(bannerMessage); | ||
| $('#managerStateBanner').show(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add Metrics for the Manager.ManagerState enum?
| }); | ||
|
|
||
| // Generates the recovery table | ||
| recoveryListTable = $('#recoveryList').DataTable({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add metrics for recovery?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I just made the front-end adhere to what is in the v2 endpoint. I don't know the history of why some things were removed from v1 to v2, but I assumed they were intentional.
Did you make this comment under the assumption I removed this code? This looks like a weird GitHub bug... This comment is showing that I deleted this code, but this has always been present in my commits. Very strange never seen something like this
- Fix error on using auto-refresh - Move REST_V2_PREFIX to top of script
Metrics aren't configured to emit by default. Can view the pictures I posted above for an idea, or configure accumulo to gather metrics. |
| function refreshManagerTables() { | ||
| ajaxReloadTable(managerStatusTable); | ||
| refreshManagerBanners(); | ||
| if (managerStatus !== 'ERROR') { | ||
| ajaxReloadTable(managerStatusTable); | ||
| } | ||
| ajaxReloadTable(recoveryListTable); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some things we need to fix here I think. If the manager process is killed after the page loads, the table still tries to refresh since we do not update the managerStatus variable. Also the table will never update properly for the same reason.
I think to fix this we could refactor out the creation logic for the managerStatusTable so we can call it in the $() function (where the table is created now) as well as in the refreshManagerTables function after we refresh managerStatus by calling getStatus() again and updating managerStatus by parsing the session storage again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find!
fixed in d1c8630 for both the homepage and manager page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did a bad job of explaining the two issues I saw. You fixed the status update one. But the other issue still exists.
If the manager page is loaded while the manager is down, the datatable will never be initialized even if the manager comes back up. I think to fix this we can refactor the creation of the managerStatusTable into a new method and conditionally call that on page load (when the status of the manager is not ERROR. And also call it in the refreshManagerTables() when the managerStatusTable is undefined (which means has never been initialized in the main run method) AND the status is not ERROR. That way, if we load the manager page while the manager is down, once the manager comes back up, the auto refresh should properly initialize the datatable.
server/monitor/src/main/java/org/apache/accumulo/monitor/next/Endpoints.java
Outdated
Show resolved
Hide resolved
Did not properly auto-refresh "/manager" and "/" pages when the manager was killed after the page was already loaded











Manager info on the Monitor (displayed on homepage (
/) and Manager page (/manager)) now obtains and displays info from the existing (but unused)rest-v2/managerendpoint.rest-v2/managerendpoint.rest-v2/managerendpoint.rest-v2/manager/metricswhich returns the Manager metrics as json. This is linked in the Manager table in both the homepage and the manager page.rest/status) (ERROR, WARN, OK). Previously took into account the manager state (e.g., if the state or goal state was SAFE_MODE or CLEAN_STOP), but this info was only included in therest/managerendpoint.rest/managerendpoint and associated code used to gather data for this endpoint.systemAlert.jsandsystemAlert.ftlas this alert was entirely based on Manager info included inrest/manager(manager state being SAFE_MODE or CLEAN_STOP). No longer applicable withrest-v2/manager./tserverspage.closes #5882