8257424: RecordingStream does not specify the recording name

[YaSuenag]

RecordingStream will start new flight recording to gather events. It has some of methods which are equivalent to Recording, but we cannot set recording name.

If we can specify the name for RecordingStream, it is useful to distinguish recordings by JFR.check dcmd.

---

Progress

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

Issue

• JDK-8257424: RecordingStream does not specify the recording name

Reviewers

• Erik Gahlin (@egahlin - Reviewer)

Download

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

[bridgekeeper]

👋 Welcome back ysuenaga! 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.

[openjdk]

@YaSuenag The following label will be automatically applied to this pull request:

• hotspot-jfr

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (35d85af)
• 00: Full (4155bf4)

[egahlin]

The underlying recording is an implementation detail, not sure it should show up in jcmd at all. Also, RemoteRecordingStream and RecordingStream should be interchangeable and setting the name on RemoteRecordingStream will not work with the current implementation.

We have deliberately not added methods in Recording to RecordingStream and kept the API to the bare minimum Many of the methods are trivial to implement, but we don't want to commit on them yet. Instead we like to get the overall architecture correct and there still things we need to figure out.

[YaSuenag]

RemoteRecordingStream would set the specified recording name.
https://github.com/openjdk/jdk/blob/master/src/jdk.management.jfr/share/classes/jdk/management/jfr/RemoteRecordingStream.java#L278

The underlying recording is an implementation detail, not sure it should show up in jcmd at all.

If so, why RemoteRecordingStream will set the name?

Also, RemoteRecordingStream and RecordingStream should be interchangeable and setting the name on RemoteRecordingStream will not work with the current implementation.

I think we can add setName() to both RecordingStream and RemoteRecordingStream, and also it is nature because RemoteRecordingStream will set specified name in current behavior.

[egahlin]

If so, why RemoteRecordingStream will set the name?

There is no API to set the name.

[egahlin]

RemoteRecordingStream would set the specified recording name.
https://github.com/openjdk/jdk/blob/master/src/jdk.management.jfr/share/classes/jdk/management/jfr/RemoteRecordingStream.java#L278

The underlying recording is an implementation detail, not sure it should show up in jcmd at all.

If so, why RemoteRecordingStream will set the name?

Also, RemoteRecordingStream and RecordingStream should be interchangeable and setting the name on RemoteRecordingStream will not work with the current implementation.

I think we can add setName() to both RecordingStream and RemoteRecordingStream, and also it is nature because RemoteRecordingStream will set specified name in current behavior.

The name is used to identify if it is a streaming recording, so it is closed properly. See OngoingStream. This is not an ideal solution, but it works for now.

[egahlin]

If you want to make the name of a RecordingStream "Recording Stream" + creationTime.toString(), for consistency and debuggability, I'm fine with that, but I rather not add an API method, at least not now.

[YaSuenag]

If you want to make the name of a RecordingStream "Recording Stream" + creationTime.toString(), for consistency and debuggability, I'm fine with that, but I rather not add an API method, at least not now.

Agree, should I change issue title? or should I file to JBS with new title?

[YaSuenag]

I updated PR. I will update both subject and description if you are ok.

[egahlin]

No problem, update PR subject and bug description. Have you run the tests in jdk/jdk/jfr?

[YaSuenag]

@egahlin I updated subject and description on this PR and on JBS.

I tested jdk/jdk/jfr on my Linux x64 with this change, Only jdk/jfr/api/recording/event/TestReEnableName.java was failed, but it does not seem to cause by this change.

test result: Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: Too few events found: expected 0 >= 5

[openjdk]

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

8257424: RecordingStream does not specify the recording name

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

• aa2d36f: 8256807: C2: Not marking stores correctly as mismatched in string opts
• 3d460bd: 8256655: rework long counted loop handling
• 13bd2e8: 8257396: AArch64 Zero build is broken after JDK-8252684
• 353e791: 8257398: Enhance debug output in Type::check_symmetrical
• f48737c: 8256254: Convert vmIntrinsics::ID to enum class
• b5ce8af: 8256373: [Windows/HiDPI] The Frame#setBounds does not work in a minimized state
• 0eaf0bb: 8257420: Zero VM build broken with clang after JDK-8256726 due to strlen() is not a constexpr
• 822ee47: 8257242: [macOS] Java app crashes while switching input methods
• 7d89852: 8244154: Update SunPKCS11 provider with PKCS11 v3.0 header files
• 4356469: 8230501: Class data support for hidden classes
• ... and 13 more: https://git.openjdk.java.net/jdk/compare/4db05e991bd8ac6959d02e7993ed8c0e1e479e4f...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.

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

[YaSuenag]

/integrate

[openjdk]

@YaSuenag Since your change was applied there have been 25 commits pushed to the master branch:

• 60f2ba9: 8257487: Include configuration name in summary
• 021dced: 8257415: ZGC: Fix barrier_data types
• aa2d36f: 8256807: C2: Not marking stores correctly as mismatched in string opts
• 3d460bd: 8256655: rework long counted loop handling
• 13bd2e8: 8257396: AArch64 Zero build is broken after JDK-8252684
• 353e791: 8257398: Enhance debug output in Type::check_symmetrical
• f48737c: 8256254: Convert vmIntrinsics::ID to enum class
• b5ce8af: 8256373: [Windows/HiDPI] The Frame#setBounds does not work in a minimized state
• 0eaf0bb: 8257420: Zero VM build broken with clang after JDK-8256726 due to strlen() is not a constexpr
• 822ee47: 8257242: [macOS] Java app crashes while switching input methods
• ... and 15 more: https://git.openjdk.java.net/jdk/compare/4db05e991bd8ac6959d02e7993ed8c0e1e479e4f...master

Your commit was automatically rebased without conflicts.

Pushed as commit e0de28c.

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