Skip to content
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

PAYARA-4097 Presets for Monitoring Console #4203

Merged
merged 20 commits into from Sep 17, 2019

Conversation

jbee
Copy link
Contributor

@jbee jbee commented Sep 6, 2019

Summary

Changes in context of to the task goal of having a preset starting page for the Monitoring Console:

  • direct collection of web and jvm statistics from providers (still needs configuration to level HIGH but no JMX/AMX)
  • added metric CPU Usage (%) to provider
  • added metric Heap Usage (%) to provider
  • added per second option to display values as delta per second instead of absolute values
  • added preset with requested starting page used in case no UI configuration is found in local storage
  • added page reset to preset (if available)
  • added manual import/export of UI configuration
  • removed unnecessary house keeping from CpuUsageHealthCheck
  • fix for inconsistent behaviour of web statistics provider after removal of application

This PR also contains numerous other changes and improvements (done between sprints):

  • more readable series name format, shorter names (now 2-3 parts: ns:<ns> [@:<group>] <metric>)
  • removed many uninteresting series from collection
  • allow filtering by instances (basic)
  • fetch all data in one request for any number of charts on the page
  • fixed NPE(s) on server to assemble result sets
  • less noisy REST API URLs

Review Notes
On Javascript code I don't think there is a point in reviewing the exact details. At this point we should focus on the general code style. Please add your suggestions on how to improve.
I split the sources into multiple files which are merged as part of the build so keep it at more manageable sizes. So monitoring-console.js is generated and does not need a review.

Testing

  1. build server.
  2. deploy webapp in appserver/monitoring-console/webapp
  3. Open localhost:8080/monitoring-console-webapp/

Browser should now show the 5 metrics requested by the task. (see Jira)
Request Count and Active Sessions will be zero since monitoring level has to switched to HIGH first. To do that:

  1. goto Configurations => server-config => Monitoring
  2. select all checkboxes in the list
  3. select level HIGH from dropdown and press Change Level
  4. click Save

Request Count should now start to show data. Active Sessions might still be zero because no session exist.

To test the reset move a chart left or right or remove it (x) and then press the rest button (most left on top icon toolbar).
The full UI configuration can also be exported and imported using the buttons right to the reset button in the top icon toolbar.

@jbee jbee self-assigned this Sep 6, 2019
@jbee jbee added this to the 5.194 milestone Sep 6, 2019
public void postConstruct(){
//Register the Web stats providers
registerWebStatsProviders();
}

private synchronized void registerWebStatsProviders() {
if (isWebStatsProvidersRegistered.get()) {
Copy link
Contributor Author

@jbee jbee Sep 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I found an inconsistency in behaviour when investigating why undeloy + deploy would cause configuration level not have the expected effect. The registerWebStatsProviders would be called in postConstruct and when new application was registered since unregistering an application would remove the providers in case this was the last application. This seems inconsistent since on server startup those provider do exist even with no application. Since there might be something depending on this I decided that it was the best fix to never remove them to be consistent over time. This however did not solve the ineffective configuration.

}
else {
times.setEndCpuTime(c);
times.setEndUserTime(u);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: Having to duplicate the computation of CPU usage for the statistic provider where such a value was added I needed to understand what is computed. Doing this I discovered that the temporary map for the statistics per thread does not serve any purpose other than totalling the time of all threads which simply could be done by looping and adding their times directly.

@jbee
Copy link
Contributor Author

jbee commented Sep 6, 2019

jenkins test please

@jbee
Copy link
Contributor Author

jbee commented Sep 6, 2019

jenkins test please

Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question but otherwise seems fine.
Built it & gave it some poking locally and nothing exploded.

<plugins>
<plugin>
<groupId>com.bekioui.maven.plugin</groupId>
<artifactId>merge-maven-plugin</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try to avoid bringing in third-party dependencies and plugins when they aren't strictly necessary.
Is there specifically a reason that you need this plugin to "merge" these files?

License seems to be fine as its Apache, just want to hear your reasoning :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no particular reason other than finding a platform independent way of merging the JS files into one. Having everything in one file got out of hand length wise so I split it to multiple files that are merged. But I also didn't want to pull in the full JS build stack madness so I found the merge plugin that does everything I need for now. Maybe there is a better way that I just have not thought about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pandrex247 Are you happy with the use of the plugin or should we look into how to else solve the issue? I'd imagine a web-server might have some feature to serve a virtual file composed of a list of resource files? :)

@Pandrex247
Copy link
Member

Jenkins test please

@jbee jbee merged commit 8964d30 into payara:master Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants