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-4238 Monitoring Console Hardening #4312

Merged
merged 8 commits into from Nov 21, 2019

Conversation

@jbee
Copy link
Contributor

jbee commented Nov 11, 2019

Summary

Fixes a number of smaller issues found during testing:

  • malformed series names (e.g. originating from an outdated config) do no longer cause server side exceptions when requesting data for them
  • legend of for request tracing details view does not overlap with the graph any longer
  • request tracing details view no longer shown "on top" of the grid but instead of it (avoid grid showing underneath)
  • names of pages with presets are now read-only
  • low and high watermark of RangeStatistic are no longer tracked as a separate metric since each metric has a all time min/max already
  • zero is always formatted as just 0 (has no specific unit as that sometimes is odd, e.g. saying 0gb for values that mostly are in kb range)
  • decimal numbers (e.g. in legend) are always shown with unit
  • formatting with units works correctly for negative numbers
  • gaps above spanned column rows are not filled when possible
  • widgets are now single select (select exclusively) since multi-select features have not been implemented yet
  • added upscaling to allow corrects to value range as e.g. caused by #4292

Improved settings inputs to prevent malformed user input and script errors as well as give user better feedback on what is valid or expected and what inputs do:

  • added input pattern attribute where suitable
  • added input title to describe the pattern where suitable
  • where a pattern is used invalid input is highlighted with a red'ish background
  • settings are updated when Unit is changes since that has an effect on other setting fields
  • range input can be cleared effectively (NaN vs undefined problem)
  • settings inputs can have an id; if not provided an automatic unique id is assigned
  • settings input and labels are linked via id so label can be clicked
  • description can be provided for settings inputs that is shown as title attribute
  • widgets can have a custom display name
  • CSS: settings table does no longer expand should a value be wider than the table cell

Testing

Use the new command to deploy MC:

asadmin> set-monitoring-console-configuration enabled=true

Open MC at: http://localhost:8080/monitoring-console/

I tested all features manually.
In particular this should now allow to add a series manually that has the name ns: system. cpu.load
without this causing the UI to show all widgets as "No Connection". Instead the added widget shows "No Data".

@jbee jbee self-assigned this Nov 11, 2019
@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Nov 11, 2019

jenkins test please

.collect(range.getName(), range.getCurrent())
.group(range.getName())
.collect("LowWaterMark", range.getLowWaterMark())
.collect("HighWaterMark", range.getHighWaterMark());

This comment has been minimized.

Copy link
@jbee

jbee Nov 13, 2019

Author Contributor

I was adding low and high watermark from the beginning just because they existed but soon realised that this is the same as all time min/max (of current) already provided for any metric so this just clutters the list of metrics for no good reason wherefore it now got removed again.

@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Nov 14, 2019

jenkins test please

@@ -192,6 +208,20 @@ MonitoringConsole.View.Units = (function() {
return number.toString().padStart(2, '0');
}

const DECIMAL_NUMBER_PATTERN = '([0-9]+\.)?[0-9]+';

This comment has been minimized.

Copy link
@dmatej

dmatej Nov 21, 2019

Contributor

Wouldn't be better to declate constants on one place like we do in Java? It's only a question, not request.

This comment has been minimized.

Copy link
@jbee

jbee Nov 21, 2019

Author Contributor

The JS code uses the "directly called function module" pattern to do effective scoping. Constants are located in the "module" that uses them. Same as in java where we hide most constants in private fields of a class.

@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Nov 21, 2019

@dmatej addressed your comments on Java exception handling with added validation, logging and handling potential exceptions in different code paths.

For JS formatting and code organisation I am not changing the style in the PR. I so far used what works good for me but that is also very much a work in progress. Surely we should consolidate this at some point. Maybe its good idea to have a look when more people get involved in MC.

I am also aware that the merging of JS files isn't ideal but I wanted to keep it simple for us Java programmers not introducing JS stack just to do a simple file merge. We could look into having this treated as generated source file.

@jbee

This comment has been minimized.

Copy link
Contributor Author

jbee commented Nov 21, 2019

jenkins test please

@dmatej
dmatej approved these changes Nov 21, 2019
@jbee jbee merged commit 6592187 into payara:master Nov 21, 2019
58 checks passed
58 checks passed
Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details

@Test(expected = IllegalArgumentException.class)
public void nullArgument() {
assertNotNull(new Series(null));

This comment has been minimized.

Copy link
@dmatej

dmatej Nov 21, 2019

Contributor

here is correct to add fail(message] after the new Series and remove the assertNotNull which will never be called

This comment has been minimized.

Copy link
@jbee

jbee Nov 21, 2019

Author Contributor

I tend to use the assertNotNull as a way to do something with the outcome should it not throw an exception so static analysis does not throw out the statement and the exception never occurs for that reason. Maybe a message could make that clear.


@Test(expected = IllegalArgumentException.class)
public void emptyArgument() {
assertNotNull(new Series(""));

This comment has been minimized.

Copy link
@dmatej

dmatej Nov 21, 2019

Contributor

Same as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.