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

8256156: JFR: Allow 'jfr' tool to show metadata without a recording #1904

Closed
wants to merge 8 commits into from

Conversation

y1yang0
Copy link
Member

@y1yang0 y1yang0 commented Dec 29, 2020

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

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8256156: JFR: Allow 'jfr' tool to show metadata without a recording

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1904/head:pull/1904
$ git checkout pull/1904

@bridgekeeper bridgekeeper bot added the oca Needs verification of OCA signatory status label Dec 29, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Dec 29, 2020

Hi @kelthuzadx, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user kelthuzadx" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@y1yang0
Copy link
Member Author

y1yang0 commented Dec 29, 2020

/covered

@bridgekeeper bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Dec 29, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Dec 29, 2020

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@y1yang0 y1yang0 closed this Jan 5, 2021
@y1yang0 y1yang0 reopened this Jan 5, 2021
@y1yang0
Copy link
Member Author

y1yang0 commented Jan 5, 2021

/signed

@bridgekeeper bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Jan 5, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 5, 2021
@y1yang0 y1yang0 closed this Jan 5, 2021
@y1yang0 y1yang0 reopened this Jan 5, 2021
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 5, 2021

You are already a known contributor!

@y1yang0
Copy link
Member Author

y1yang0 commented Jan 5, 2021

/label add hotspot-jfr

@openjdk openjdk bot added the hotspot-jfr hotspot-jfr-dev@openjdk.org label Jan 5, 2021
@openjdk
Copy link

openjdk bot commented Jan 5, 2021

@kelthuzadx
The hotspot-jfr label was successfully added.

@y1yang0
Copy link
Member Author

y1yang0 commented Jan 5, 2021

/label add ready

@mlbridge
Copy link

mlbridge bot commented Jan 5, 2021

Webrevs

@openjdk
Copy link

openjdk bot commented Jan 5, 2021

@kelthuzadx The label ready is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt

@mlbridge
Copy link

mlbridge bot commented Jan 8, 2021

Mailing list message from Yang Yi on hotspot-jfr-dev:

Happy New Year.

Can anyone help reviewing this patch? ;)

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

------------------------------------------------------------------
From:Yang Yi <github.com+5010047+kelthuzadx at openjdk.java.net>
Send Time:2021 Jan. 5 (Tue.) 10:55
To:hotspot-jfr-dev <hotspot-jfr-dev at openjdk.java.net>
Subject:RFR: 8256156: JFR: Allow 'jfr' tool to show metadata without a recording

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

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

Commit messages:
- Allow jfr tool to show metadata without a recording

Changes: https://git.openjdk.java.net/jdk/pull/1904/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1904&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8256156
Stats: 151 lines in 5 files changed: 131 ins; 8 del; 12 mod
Patch: https://git.openjdk.java.net/jdk/pull/1904.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/1904/head:pull/1904

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

} catch (IOException ioe) {
couldNotReadError(file, ioe);
}
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error handling seems inadequate. If there is IOException, it still makes sense to get proper message, and not a stack trace

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

continue;
}
if (foundEventFilter) {
if (acceptedEvents.contains(type.getName())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect filtering for the metadata command to work like the 'print' command (accepting acronyms, wildcards etc.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

if (categoryAnno != null) {
String[] categories = categoryAnno.value();
for (String category : categories) {
if (categoryNames.contains(category)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect filtering for the metadata command to work like the 'print' command (wildcards etc.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

options.removeLast();
return tmp;
} catch (Exception e) {
// ignored since recording file for jfr metadata is optional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inadequate error handling. If there is something wrong with the file (permission etc) I like to know it, and not get the event types for the current JVM.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would not swallow the exception now.

eventNames.add(line.substring(7, line.indexOf("\"", 7)));
}
}
List<Type> eventTypes = TypeLibrary.getInstance().getTypes();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to use public API (FlightRecorder.getEventTypes()) instead of calling internals.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

@@ -99,7 +106,11 @@ public String getName() {

@Override
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ine would like to get similar help as for the ''print' command when it comes to filtering.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@egahlin
Copy link
Member

egahlin commented Jan 8, 2021

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Jan 8, 2021
@openjdk
Copy link

openjdk bot commented Jan 8, 2021

@egahlin has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@kelthuzadx please create a CSR request and add link to it in JDK-8256156. This pull request cannot be integrated until the CSR request is approved.

@openjdk openjdk bot removed the rfr Pull request is ready for review label Jan 8, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 8, 2021
@y1yang0 y1yang0 marked this pull request as draft January 8, 2021 09:31
@egahlin
Copy link
Member

egahlin commented Feb 16, 2021

It still doesn't work the same way as when you specify a file.

Here is a patch I think it will fix the functionality. Not tested or formatted.

diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/tool/Metadata.java    
b/src/jdk.jfr/share/classes/jdk/jfr/internal/tool/Metadata.java
index 7a4b49703cf..1b6c50df441 100644
--- a/src/jdk.jfr/share/classes/jdk/jfr/internal/tool/Metadata.java
+++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/tool/Metadata.java
@@ -39,6 +39,7 @@ import java.util.List;
import java.util.function.Predicate;

import jdk.jfr.EventType;
+import jdk.jfr.FlightRecorder;
import jdk.jfr.consumer.RecordingFile;
import jdk.jfr.internal.PlatformEventType;
import jdk.jfr.internal.PrivateAccess;
@@ -202,38 +203,40 @@ final class Metadata extends Command {
         if (filter != null) {
             filter = addCache(filter, type -> type.getId());
         }
-
-            List<Type> types = getAllTypes(file);
+           List<Type> types = findTypes(file);
             Collections.sort(types, new TypeComparator());
             for (Type type : types) {
-                // if it's an event type, apply filter on it
-                if (Type.SUPER_TYPE_EVENT.equals(type.getSuperType())) {
-                    EventType et = PrivateAccess.getInstance().newEventType((PlatformEventType) type);
-                    if (filter != null && !filter.test(et)) {
-                        continue;
+                if (filter != null) { 
+                    // If --events or --categories, only operate on events
+                    if (Type.SUPER_TYPE_EVENT.equals(type.getSuperType())) {
+                        EventType et = PrivateAccess.getInstance().newEventType((PlatformEventType) type);
+                        if (filter.test(et)) {
+                            prettyWriter.printType(type);
+                        }
                 }
+                } else {
+                    prettyWriter.printType(type);
             }
-
-                prettyWriter.printType(type);
         }
         prettyWriter.flush(true);
         pw.flush();
     }
 }

-    private List<Type> getAllTypes(Path file) throws UserDataException {
+    private List<Type> findTypes(Path file) throws UserDataException {
          // Determine whether reading from recording file or reading from the JDK where
          // the jfr tool is located will be used
          if (file == null) {
+            // Force initialization
+            FlightRecorder.getFlightRecorder().getEventTypes();
             return TypeLibrary.getInstance().getTypes();
          }
-        List<Type> types = null;
         try (RecordingFile rf = new RecordingFile(file)) {
-            types = PRIVATE_ACCESS.readTypes(rf);
+            return PRIVATE_ACCESS.readTypes(rf);
         } catch (IOException ioe) {
             couldNotReadError(file, ioe);
         }
-        return types;
+        return null; // Can't reach
     }

 private Path getOptionalJFRInputFile(Deque<String> options) throws UserDataException {

@y1yang0
Copy link
Member Author

y1yang0 commented Feb 18, 2021

Hi Erik, I have updated this patch according to your diff content.

I'm fine with changes about force initialization. But it looks like the changes in execute() method does the same thing? If no filter is given, they both print all types. If filter applied, they print only event type.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Feb 24, 2021
Copy link
Member

@egahlin egahlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for (EventType eventType : eventTypes) {
expectedNames.add(eventType.getName());
}
Asserts.assertGTE(eventNames.size(), expectedNames.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
}

testWildcardAndAcronym();
}

static void testBasic() throws Throwable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Asserts.assertGTE(eventNames.size(), expectedNames.size());
}

static void testDeterministic() throws Throwable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change name of the method to testEventFilter().

List<String> lines = output.asLines();

for (String line : lines) {
if (line.startsWith("@Name(\"")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


for (String line : lines) {
if (line.startsWith("@Name(\"")) {
eventNames.add(line.substring(7, line.indexOf("\"", 7)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

static void testWildcardAndAcronym() throws Throwable {
OutputAnalyzer output = ExecuteHelper.jfr("metadata", "--events", "Thread*");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, I re-implement the test, using customized events.

Asserts.assertGTE(eventNames.size(), 2);
}

static void testWildcardAndAcronym() throws Throwable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@egahlin egahlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

I still think the name of the test method should be called testWildcard() instead of testWildcardAndAcronym() since it is not testing acronyms, i.e using "GC" instead of "GarbageCollection", which is fine, but the name of the method should be appropriate.

@openjdk
Copy link

openjdk bot commented Mar 11, 2021

@kelthuzadx 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:

8256156: JFR: Allow 'jfr' tool to show metadata without a recording

Reviewed-by: egahlin

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 996 new commits pushed to the master branch:

  • 43524cc: 8243455: Many SA tests can fail due to trying to get the stack trace of an active method
  • e834f99: 8263412: ClassFileInstaller can't be used by classes outside of default package
  • bf9b5fa: 8263501: compiler/oracle/TestInvalidCompileCommand.java fails with release VMs
  • 0c8350e: 8263460: DynamicArchiveRelocationTest.java fails in product VM
  • b2f7c58: 8263055: hsdb Command Line Debugger does not properly direct output for some commands
  • ecfa712: 8263326: Remove ReceiverTypeData check from serviceability/sa/TestPrintMdo.java
  • b932a62: 8263470: Consolidate copies of getClassBytes in various tests
  • 0ea48d9: 8194129: Regression automated Test '/open/test/jdk/java/awt/Window/ShapedAndTranslucentWindows/TranslucentChoice.java' fails
  • 4b5c664: 8178348: left_n_bits(0) invokes undefined behavior
  • 0b10c6b: 8263017: Read barriers are missing in nmethod printing code
  • ... and 986 more: https://git.openjdk.java.net/jdk/compare/779ee1104ca7f506bd1113004a84745824a507d4...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@egahlin) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 11, 2021
@y1yang0
Copy link
Member Author

y1yang0 commented Mar 12, 2021

Thank Erik for your patient and reviews. Your naming suggestion makes sense, I forgot to rename that method in the previous commit. Now it has been corrected.

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 12, 2021
@openjdk
Copy link

openjdk bot commented Mar 12, 2021

@kelthuzadx
Your change (at version a2cedbf9841a60222e727006e24309fd84da2a9c) is now ready to be sponsored by a Committer.

@openjdk openjdk bot removed the sponsor Pull request is ready to be sponsored label Mar 12, 2021
@egahlin
Copy link
Member

egahlin commented Mar 12, 2021

/sponsor

@openjdk
Copy link

openjdk bot commented Mar 12, 2021

@egahlin The PR has been updated since the change author (@kelthuzadx) issued the integrate command - the author must perform this command again.

@y1yang0
Copy link
Member Author

y1yang0 commented Mar 13, 2021

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 13, 2021
@openjdk
Copy link

openjdk bot commented Mar 13, 2021

@kelthuzadx
Your change (at version c401df4) is now ready to be sponsored by a Committer.

@egahlin
Copy link
Member

egahlin commented Mar 13, 2021

/sponsor

@openjdk openjdk bot closed this Mar 13, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 13, 2021
@openjdk
Copy link

openjdk bot commented Mar 13, 2021

@egahlin @kelthuzadx Since your change was applied there have been 997 commits pushed to the master branch:

  • 0b68ced: 8263548: runtime/cds/appcds/SharedRegionAlignmentTest.java fails to compile after JDK-8263412
  • 43524cc: 8243455: Many SA tests can fail due to trying to get the stack trace of an active method
  • e834f99: 8263412: ClassFileInstaller can't be used by classes outside of default package
  • bf9b5fa: 8263501: compiler/oracle/TestInvalidCompileCommand.java fails with release VMs
  • 0c8350e: 8263460: DynamicArchiveRelocationTest.java fails in product VM
  • b2f7c58: 8263055: hsdb Command Line Debugger does not properly direct output for some commands
  • ecfa712: 8263326: Remove ReceiverTypeData check from serviceability/sa/TestPrintMdo.java
  • b932a62: 8263470: Consolidate copies of getClassBytes in various tests
  • 0ea48d9: 8194129: Regression automated Test '/open/test/jdk/java/awt/Window/ShapedAndTranslucentWindows/TranslucentChoice.java' fails
  • 4b5c664: 8178348: left_n_bits(0) invokes undefined behavior
  • ... and 987 more: https://git.openjdk.java.net/jdk/compare/779ee1104ca7f506bd1113004a84745824a507d4...master

Your commit was automatically rebased without conflicts.

Pushed as commit 86e4c75.

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

@y1yang0 y1yang0 deleted the JDK_8256156_impl branch March 14, 2021 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-jfr hotspot-jfr-dev@openjdk.org integrated Pull request has been integrated
2 participants