jfrunit icon indicating copy to clipboard operation
jfrunit copied to clipboard

Add automatic analysis from JMC

Open johnaohara opened this issue 3 years ago • 11 comments

Allow assertions against JMC automatic analysis rules based on whether rule has fired and severity.

johnaohara avatar Aug 19 '21 09:08 johnaohara

@gunnarmorling I have finally found some time to open a PR for JMC analysis. This PR will load rules from the published JMC artefacts (org.openjdk.jmc.flightrecorder.rules and org.openjdk.jmc.flightrecorder.rules.jdk). There is still more that can be done around custom rules, configuring severity and expanding on assertions, but initially demonstrates the scope and type of assertions that could be made against automated analysis

johnaohara avatar Aug 19 '21 09:08 johnaohara

@johnaohara This looks great! I just added couple of suggestions.

phejl avatar Aug 19 '21 10:08 phejl

@johnaohara If I understand the code correctly it dumps the events to the same file which is later rewritten at the end of the test (or sonner by another analysis if we will allow that). Wouldn't it be better having it isolated? So the file used for analysis would be suffixed with something like -analysis-1 and would be kept for possible later check? WDYT @gunnarmorling ?

phejl avatar Aug 19 '21 17:08 phejl

@johnaohara If I understand the code correctly it dumps the events to the same file which is later rewritten at the end of the test (or sonner by another analysis if we will allow that). Wouldn't it be better having it isolated? So the file used for analysis would be suffixed with something like -analysis-1 and would be kept for possible later check? WDYT @gunnarmorling ?

@phejl I had assumed that all historic events would be preserved in between writing to disk, i.e. an new events would be appended to the existing jfr file, although I need to validate this assumption.

I understand the concern if the output file was overwritten and events that had previously been written to disk were lost. I also can see the concern if a user was to run multiple automatic analysis during a test and want to be able to inspect events after the test has completed. If the JFR recording is overwritten multiple times with each automatic analysis, a user would not be able to reload a JFR recording exactly as it was at the point the analysis was performed.

I did investigate converting the JfrEvents.events from a Queue<RecordedEvent> to a IItemCollection to pass directly into org.openjdk.jmc.flightrecorder.rules.util.RulesToolkit.evaluateParallel() but this at first appeared to be overly convoluted and it was much easier to write recorded events to disk and reload from disk for analysis.

I can dump the recording used for analysis to a separate file for clarity, but will double check that dumping the recording does not reset the recording and the JFR events generated by a single test are not split over multiple recordings.

johnaohara avatar Aug 20 '21 10:08 johnaohara

@gunnarmorling an example of using the analysis over individual events could be a generic catch all, "I want to assert that there are no WARNINGS in from the automated analysis", e.g.

    @EnableConfiguration("profile")
    public void automatedExceptionAnalysis() throws Exception {

        //simulate some user code that silently discards a lot of exceptions
        for (int i = 0; i < 20_000; i++) {
            try{
                throw new MethodNotFoundException();
            } catch (MethodNotFoundException methodNotFoundException){
                //silently swallow exception
            }
        }

        jfrEvents.awaitEvents();

        List<IResult> analysisResults = jfrEvents.automaticAnalysis();

        assertThat(analysisResults.size()).isGreaterThan(0);

        JmcAutomaticAnalysisAssert.assertThat(analysisResults)
                .haveSeverityLessThan(Severity.WARNING);

    }

In this example, the test would fail as the org.openjdk.jmc.flightrecorder.rules.jdk.exceptions.ExceptionRule would generate a WARNING analysis result.

(N.B. the haveSeverityLessThan() method is not currently in the JfrAnalysisAssert api, but I have a working proyotype in a new JmcAutomaticAnalysis api that I will push shortly)

johnaohara avatar Aug 20 '21 10:08 johnaohara

@johnaohara If I understand the code correctly it dumps the events to the same file which is later rewritten at the end of the test...

@phejl I checked the behaviour of JFR when events are dumped to file multiple times, and jfr is writing all events out to disk with every dump and duplicating events. I will separate the analysis dump files so events are not duplicated in the final jfr's

johnaohara avatar Aug 20 '21 12:08 johnaohara

an example of using the analysis over individual events could be a generic catch all, "I want to assert that there are no WARNINGS in from the automated analysis"

Ok, that could be useful, yes. On the exception case in particular, couldn't the same be achieved though like so:

assertThat(jfrEvents).contains(
    event("jdk.ExceptionStatistics").with("throwables", 0);

?

Note I don't mean to push back, I think integration with the rules API can be useful. I just would love to see some really compelling example or use case.

gunnarmorling avatar Aug 20 '21 13:08 gunnarmorling

There's a failure in the new test on CI btw:

JmcAutomaticAnalysisTest.automatedExceptionAnalysis:88 Expected to contain severity greater than: Warning

gunnarmorling avatar Aug 20 '21 13:08 gunnarmorling

There's a failure in the new test on CI btw:

JmcAutomaticAnalysisTest.automatedExceptionAnalysis:88 Expected to contain severity greater than: Warning

Yes, that is ironic; I added that test as an example, and it "passed on my machine"™

johnaohara avatar Aug 20 '21 13:08 johnaohara

Yes, that is ironic; I added that test as an example, and it "passed on my machine"™

LOL 🤣 .

gunnarmorling avatar Aug 20 '21 13:08 gunnarmorling

Hey @johnaohara, any further insight on the test failure here?

gunnarmorling avatar Sep 03 '21 19:09 gunnarmorling