7231: JMC createReport for JMC8 Automated Analysis fails to evaluate several rules #311

aptmac · 2021-09-24T21:32:18Z

This PR addresses JMC-7231 [0], in which using JfrHtmlRulesReport throws a number of exceptions and fails to evaluate a handful of rules.

I used some of the review comments [1] that I made in the other open PR to address the same issue.

As far as I can tell, there are two main issues here.

The first is some rules now (after the Rules 2.0 update) have dependencies, these are (4) GcLockerRule, GcStallRule, HeapInspectionRule, and SystemGcRule which depend on GarbageCollectionInfoRule. The problem here is that if GarbageCollectionInfoRule isn't evaluated then the other four rules end up throwing an NPE while trying to evaluate.

The second issue is that as of Rules 2.0, on the jmc.ui Automated Analysis page we use RulesToolkit.matchesEventAvailabilityMap() to verify is an event is is available and if the rule should be evaluated. This check wasn't replicated on the core implementation, so I've added it in. What's interesting here is that in the test jfr I was using, GarbageCollectionInfoRule was filtered out by matchesEventAvailabilityMap(), which was causing the above 4 Gc rules to explode.

The solution I've put together uses two lists to track 1). rules that are unavailable due to being filtered out by matchesEventAvailabilityMap(), and 2). rules that have dependencies. It later loops through the list of rules with dependencies, and checks to see if they exist in the list with unavailable rules. If a rule is not able to be evaluated by either 1). not being available or 2). by having a dependency that is not available, it returns a completed future IResult containing "not available", similar to how on this is handled on the jmc.ui side. Then the rest of the rules report proceeds as usual.

I think the easiest way to verify/try this fix is by creating a run configuration for JfrHtmlRulesReport in Eclipse, and feeding it a jfr file.

[0] https://bugs.openjdk.java.net/browse/JMC-7231
[1] #302 (review)

Progress

Commit message must refer to an issue
Change must be properly reviewed

Issue

JMC-7231: JMC createReport for JMC8 Automated Analysis fails to evaluate several rules

Reviewers

Marcus Hirt (@thegreystone - Reviewer)
Henrik Dafgård (@Gunde - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/311/head:pull/311
$ git checkout pull/311

Update a local copy of the PR:
$ git checkout pull/311
$ git pull https://git.openjdk.java.net/jmc pull/311/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 311

View PR using the GUI difftool:
$ git pr show -t 311

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/311.diff

bridgekeeper · 2021-09-24T21:33:44Z

👋 Welcome back aptmac! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2021-09-24T21:38:03Z

Webrevs

thegreystone

LGTM, but I'd prefer @Gunde to review this.

thegreystone · 2021-09-29T18:57:18Z

It could be worth getting this particular fix into an 8.1.1 release.

...der.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/messages/internal/Messages.java

...ightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java

...rc/main/resources/org/openjdk/jmc/flightrecorder/rules/messages/internal/messages.properties

core/tests/org.openjdk.jmc.flightrecorder.rules.test/pom.xml

...recorder.rules.test/src/test/java/org/openjdk/jmc/flightrecorder/rules/RulesToolkitTest.java

…several rules

…nd on it

thegreystone · 2021-10-04T17:52:35Z

/reviewers 2

openjdk · 2021-10-04T17:52:55Z

@thegreystone
The number of required reviews for this PR is now set to 2 (with at least 1 of role committers).

Gunde · 2021-10-04T19:03:32Z

...ightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java

+	 *            provider of configuration values used by the ResultBuilder
+	 * @return a completed future containing an evaluation error result
+	 */
+	private static Future<IResult> evaluationErrorResult(IRule rule, IPreferenceValueProvider preferences) {


I think it might be slightly confusing that this is labeled with "error" if the rule evaluation did not fail, just did not take place. It could probably just be a N/A result instead, which we should already have methods for in RulesToolkit.

Gunde · 2021-10-04T19:05:14Z

...ightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java

+			boolean shouldEvaluate = true;
+			for (IRule unavailableRule : unavailableRules) {
+				if (dependencyName.equals(unavailableRule.getId())) {
+					shouldEvaluate = false;
+					resultFutures.put(rule, evaluationErrorResult(rule, preferences));
+					break;
+				}
+			}


This is not enough to determine whether or not a rule should be evaluated. The @dependsOn annotation specifies both a class and an optional minimum severity that the prior rule must return for the current rule to evaluate. If we disregard this we risk running into NPEs for later rules as they attempt to access results that might not be present in the RulesProvider.

Right, thanks for pointing that out. I'll put a severity check into there similar to how it's done in RuleManager: https://github.com/openjdk/jmc/blob/master/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java#L114

aptmac · 2021-10-06T15:18:00Z

I've incorporated Henrik's review comments into this latest commit.

The first addressed change is making use of RulesToolkit.getNotApplicableResult() for rules that are not evaluated. I tried to make it such that the messages associated with these NA results match what is used in RuleManager.

The second addressed change is to take the dependency's result severity value into account when evaluating a rule. On the UI side, this is done by RuleManager.shouldEvaluate(), so I've created a similar function at RulesToolkit.shouldEvaluate(), that takes a rule and the result of its dependency rule.

Lastly I tried to reorganize the code a bit so it should be easier to keep track of the rules (with dependencies), their dependencies, and the computed result of these dependencies.

openjdk · 2021-10-14T14:23:16Z

@aptmac This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

7231: JMC createReport for JMC8 Automated Analysis fails to evaluate several rules

Reviewed-by: hirt, hdafgard

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

90197d6: 7419: Update XML parsing
3424e72: 7409: Update the spotless maven plugin

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

aptmac · 2021-10-14T15:03:01Z

/integrate

openjdk · 2021-10-14T15:04:29Z

Going to push as commit c725078.
Since your change was applied there have been 2 commits pushed to the master branch:

90197d6: 7419: Update XML parsing
3424e72: 7409: Update the spotless maven plugin

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-14T15:04:44Z

@aptmac Pushed as commit c725078.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr label Sep 24, 2021

thegreystone requested a review from Gunde September 26, 2021 23:37

aptmac force-pushed the 7231 branch from 86159cb to ca98a1d Compare September 29, 2021 16:41

thegreystone reviewed Sep 29, 2021

View reviewed changes

thegreystone added the hacktoberfest-accepted label Oct 1, 2021

thegreystone suggested changes Oct 1, 2021

View reviewed changes

aptmac added 5 commits October 4, 2021 11:10

7231: JMC createReport for JMC8 Automated Analysis fails to evaluate …

8af8297

…several rules

ensure rule dependency is evaluated before evaluating rules that depe…

8740dc9

…nd on it

clean up if statement

1510838

run mvn spotless:apply

a8efbf5

update copyright year and javadoc for RulesToolkit.evaluateErrorResult()

892ed43

aptmac force-pushed the 7231 branch from 6a97078 to 892ed43 Compare October 4, 2021 15:10

thegreystone approved these changes Oct 4, 2021

View reviewed changes

Gunde suggested changes Oct 4, 2021

View reviewed changes

verify dependency result severity satisfies rule severity requirement

66452da

thegreystone self-requested a review October 6, 2021 16:27

thegreystone approved these changes Oct 6, 2021

View reviewed changes

thegreystone requested a review from Gunde October 8, 2021 16:47

Gunde approved these changes Oct 14, 2021

View reviewed changes

openjdk bot added the ready label Oct 14, 2021

openjdk bot closed this Oct 14, 2021

openjdk bot added integrated and removed ready rfr labels Oct 14, 2021

aptmac deleted the 7231 branch October 14, 2021 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7231: JMC createReport for JMC8 Automated Analysis fails to evaluate several rules #311

7231: JMC createReport for JMC8 Automated Analysis fails to evaluate several rules #311

aptmac commented Sep 24, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 24, 2021

mlbridge bot commented Sep 24, 2021 •

edited

Loading

thegreystone left a comment

thegreystone commented Sep 29, 2021

thegreystone commented Oct 4, 2021

openjdk bot commented Oct 4, 2021

Gunde Oct 4, 2021

Gunde Oct 4, 2021

aptmac Oct 4, 2021

aptmac commented Oct 6, 2021

openjdk bot commented Oct 14, 2021

aptmac commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

7231: JMC createReport for JMC8 Automated Analysis fails to evaluate several rules #311

7231: JMC createReport for JMC8 Automated Analysis fails to evaluate several rules #311

Conversation

aptmac commented Sep 24, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 24, 2021

mlbridge bot commented Sep 24, 2021 • edited Loading

Webrevs

thegreystone left a comment

Choose a reason for hiding this comment

thegreystone commented Sep 29, 2021

thegreystone commented Oct 4, 2021

openjdk bot commented Oct 4, 2021

Gunde Oct 4, 2021

Choose a reason for hiding this comment

Gunde Oct 4, 2021

Choose a reason for hiding this comment

aptmac Oct 4, 2021

Choose a reason for hiding this comment

aptmac commented Oct 6, 2021

openjdk bot commented Oct 14, 2021

aptmac commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

aptmac commented Sep 24, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 24, 2021 •

edited

Loading