chore(jmc-core): Upstream updates #201
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ebaron
reviewed
Mar 28, 2023
There was a problem hiding this comment.
Hey Josh, thanks for doing this! A couple of general comments:
- Were there any packages/files moved to core by 8.2 that can now be removed from this embedded source, or are they all still needed?
- It seems like GitHub is complaining about no newline at the end of several files. I'm not sure if it's like this in the JMC source too.
|
@Josh-Matsuoka Also, do you know how difficult it will be to update the InterruptibleReportsGenerator code for 8.2? We'd really like to have jmc-core updated to 8.2 as well for Cryostat 2.3. |
|
@ebaron I'll take a look in a bit more detail and see if it's realistic to do in time for code freeze. Otherwise, the code we're currently embedding is still in the process of being moved upstream, so it wouldn't be in 8.2. I'll take a look at fixing the whitespace issue as well. |
|
Updated the PR to bump jmc-core up to 8.2 I've updated InterruptibleReportsGenerator to account for the rules 2.0 changes and added an extra few sanity checks to the rule evaluation code to check for dependencies (some rules have dependencies in rules 2.0, e.g. the GC rules rely on the GC Information Rule to be run) and check that the required events exist as well. This should fix the newline issue as well. |
src/main/java/io/cryostat/core/reports/InterruptibleReportGenerator.java
Outdated
Show resolved
Hide resolved
|
Very simple looking integration test failures when running this update in a Cryostat image: quite a promising result! Actually running it in a smoketest now to see how things look. The fact that the itest suite otherwise all passed should mean that the basic functionalities are all still working however. |
|
I'll make a PR in cryostat main to fix those itests |
src/main/java/io/cryostat/core/reports/InterruptibleReportGenerator.java
Outdated
Show resolved
Hide resolved
|
It looks like the "explanation" field is missing some string interpolations:
I'm not sure how the JMC Rules 2.0 is meant to do this, though. |
|
The |
I think this is it: |
|
After my last commit ^ |
|
It seems like |
|
It buckets them in by the looks Rules call out to Severity.get() when setting it and that method then clamps it to the nearest Severity score |
|
Yea, I guess it does. The Severity constructor is private and the only score/limit values are those 0/25/75 thresholds. But, individual rule evaluations do still have a And this looks like a common pattern: double score = RulesToolkit.mapExp100(longestDuration.doubleValueIn(UnitLookup.SECOND),
infoLimit.doubleValueIn(UnitLookup.SECOND), warningLimit.doubleValueIn(UnitLookup.SECOND));
...
return ResultBuilder.createFor(this, vp).setSeverity(severity)
.setSummary(Messages.getString(Messages.FileReadRuleFactory_TEXT_WARN))
.setExplanation(Messages.getString(Messages.FileReadRuleFactory_TEXT_WARN_LONG))
.addResult(TypedResult.SCORE, UnitLookup.NUMBER_UNITY.quantity(score))
.addResult(LONGEST_READ_AMOUNT, amountRead).addResult(LONGEST_READ_TIME, longestDuration)
.addResult(AVERAGE_FILE_READ, avgDuration).addResult(TOTAL_FILE_READ, totalDuration)
.addResult(LONGEST_TOTAL_READ, totalLongestIOPath).addResult(LONGEST_READ_PATH, fileName).build();I think we can extract it back with |
|
Nailed it:
|
|
I'm having some strange flakey test issues with |
|
I think the problem was some kind of caching or threading issue with @ebaron any other feedback on this PR? I think it's looking good. |
|
I spoke too soon: This is a pretty typical example. The stack trace where some |
|
Ah, maybe it has to do with that new inter-rule dependency stuff you were talking about, @Josh-Matsuoka . Could it be that this Rule A is failing because it depends on a result from Rule B, but A is calculated on Thread 1 and B is on Thread 2? I see you already have some logic to try to ensure this kind of thing doesn't happen, but perhaps that's not threadsafe? |
The embedded JMC sources look fine to me. Once we figure out the report generation test issue, this is good to go for me. |
|
Looking at the heap inspection rule, it has a dependency on the GarbageCollectionInfo rule to be run first, and it's results being added to the ResultsProvider: So if the GarbageCollectionInfo rule is never run the aggregate would be null causing the error. This kind of thing should be caught by the dependency check, it looks like there might be a race here where the evaluatedRules map was updated, causing the dependency check to pass but the rule was run before the ResultProvider was updated. |
|
~Also this whole thing can be cached, because the dependency relations between rules are static, so this can all be done on startup by checking the RuleRegistry and computing these dependency subgraphs for all of the rules, and then just making a map keyed on a rule to be evaluated and mapping to its set of direct dependencies. When we service a request and need to evaluate a rule we can reference this cache to determine which rules need to be evaluated first and prepend them to the work queue.~~ After a cursory glance at the JMC sources for RuleRegistry... it already does exactly this! |
|
That looks like it's basically what I was describing. Let's port that over but replace the |
|
@Josh-Matsuoka are you able to take on porting over that implementation or should I give it a stab? |
|
I'll take a look at it |
|
Updated to bring over evaluateParallel |
|
This is looking all good now, repeated testing now seems stable and the code looks like it's doing what I was trying to explain. Great work @Josh-Matsuoka , thanks so much! |
andrewazores
previously approved these changes
Apr 14, 2023
|
@Josh-Matsuoka oh, sign your commits please. |
ebaron
previously approved these changes
Apr 14, 2023
Josh-Matsuoka
force-pushed
the
upstream-updates
branch
from
786780c
to
05dcfe1
Compare
|
hmm that brought in quite the commit history |
…r rules changes in core, fixing whitespace
andrewazores
force-pushed
the
upstream-updates
branch
from
05dcfe1
to
cdebf4d
Compare
|
Fixed it :-) |
andrewazores
approved these changes
Apr 14, 2023
This was referenced Apr 14, 2023
Closed
This was referenced May 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
This PR updates the embedded JMC sources to bring them in line with upstream, there's work onging upstream to refactor jmc-core and bring in most of the classes we're using, but it's not going to be resolved in time so updating the embedded sources makes the most sense for the moment.
Addresses #95
TODO for a separate PR or perhaps for this one, the InterruptibleReportsGenerator relies on old rules code that has been updated between 7.1.1 and 8.2.0 and doesn't build when updated to 8.2.0 so updating the maven dependency on jmc-7.1.1 will need a rework of InterruptibleReportsGenerator