RFR: 8256156: JFR: Allow 'jfr' tool to show metadata without a recording [v12]

[Erik Gahlin]

On Thu, 18 Feb 2021 02:18:57 GMT, Yi Yang <github.com+5010047+kelthuzadx at openjdk.org> wrote:

>> Hi all,
>> 
>> May I please have a review for this minor patch?
>> 
>> It simplifies the use of subcommand `metadata` of jfr tool. Recording 
>> file is no longer mandatory, users can directly use `jfr metadata` to 
>> output JDK builtin meta information. As JDK-8256156 mentioned, it also 
>> supports events and categories filters:
>> $ jfr metadata 
>> $ jfr metadata recording.jfr # compatible
>> $ jfr metadata --events jdk.ThreadStart,jdk.ThreadEnd
>> $ jfr metadata --events jdk.ThreadStart,jdk.ThreadEnd recording.jfr
>> $ jfr metadata --categories GC,Detailed
>> $ jfr metadata --categories GC,Detailed recording.jfr
>
> Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix

I think it is easier to grasp what the code does without the continue statement.

test/jdk/jdk/jfr/tool/TestMetadata.java line 106:

> 104:             expectedNames.add(eventType.getName());
> 105:         }
> 106:         Asserts.assertGTE(eventNames.size(), expectedNames.size());

Seems something like this would be sufficient:

    public void testNumberOfEventTypes() {
      int count = 0;
      for (String line : output.asLines()) {
      if (line.contains("extends jdk.jfr.Event")) {
        count++;
      }
      Assert.assertEquals(count, FlightRecorder.getFlightRecorder().getEventTypes().size());
    }
≈

test/jdk/jdk/jfr/tool/TestMetadata.java line 57:

> 55:     }
> 56: 
> 57:     static void testBasic() throws Throwable {

Perhaps change the name of the method to testUnfiltered() to make it more clear what the test does and create a separate method for the --wrongOption, i.e. testIllegalOption()

test/jdk/jdk/jfr/tool/TestMetadata.java line 109:

> 107:     }
> 108: 
> 109:     static void testDeterministic() throws Throwable {

Change name of the method to testEventFilter().

test/jdk/jdk/jfr/tool/TestMetadata.java line 115:

> 113: 
> 114:         for (String line : lines) {
> 115:             if (line.startsWith("@Name(\"")) {

Use contains "extends jdk.jfr.Event" as it is more stable

test/jdk/jdk/jfr/tool/TestMetadata.java line 116:

> 114:         for (String line : lines) {
> 115:             if (line.startsWith("@Name(\"")) {
> 116:                 eventNames.add(line.substring(7, line.indexOf("\"", 7)));

Should be easy to check if the event is correct, not just the number.

    if (line.contains("XXZQZYY.") {
      event1 = true;
    }
    if (line.contains("ZQZPP") {
      event2 = true;
    }
    
    Regarding events names, see following comment.

test/jdk/jdk/jfr/tool/TestMetadata.java line 123:

> 121: 
> 122:     static void testWildcardAndAcronym() throws Throwable {
> 123:         OutputAnalyzer output = ExecuteHelper.jfr("metadata", "--events", "Thread*");

Instead of depending on name of existing JVM events (which may change, be removed or interfere with wildcard expansion done by the OS) it would be better to create two custom Java events with an unlikely name pattern, i.e. com.example.XXZQZYY and com.stuff.ZQZPP, and then filter on "ZQZ".  

To make sure they are registered, you can do:

    FlightRecorder.register(XXZQZYY.class); 
    FlightRecorder.register(ZQZPP.class); 

in static initializer so it is executed before calling ExecuteHelper.jfr("metadata", "--events", "ZQZ*");

test/jdk/jdk/jfr/tool/TestMetadata.java line 122:

> 120:     }
> 121: 
> 122:     static void testWildcardAndAcronym() throws Throwable {

The name of the seems misleading since it doesn't test acronyms, testWildcard() shuld be sufficient.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1904