jmc icon indicating copy to clipboard operation
jmc copied to clipboard

7506: Incorrect numeric formatting of PID by JMC

Open Suchitainf opened this issue 1 year ago • 6 comments

This PR is to fix the formatting issue of the JVM PID on JVM information screen. JVM PID attribute is received as NUMBER from JFR and hence the JMC application is treating is as number. When the number is smaller , the PID is displayed using commas and when the number is larger, its converted to exponential format. This is a long pending bug in product.

As part of this PR we have introduced a new aggregator JVM_PID_ID which will contain the value of Identifier in plain text format instead of number format. This new aggregator is displayed on JVM Information page so that formatting is not applied as per NUMBER formatting rules.

JVM PID before the change: image

JVM PID after the change: image

The issue is more prominent when the identifier value is converted to exponential.


Progress

  • [x] Commit message must refer to an issue
  • [ ] Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

  • JMC-7506: Incorrect numeric formatting of PID by JMC (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jmc.git pull/557/head:pull/557
$ git checkout pull/557

Update a local copy of the PR:
$ git checkout pull/557
$ git pull https://git.openjdk.org/jmc.git pull/557/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 557

View PR using the GUI difftool:
$ git pr show -t 557

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jmc/pull/557.diff

Webrev

Link to Webrev Comment

Suchitainf avatar Mar 15 '24 14:03 Suchitainf

:wave: Welcome back schaturvedi! 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.

bridgekeeper[bot] avatar Mar 15 '24 14:03 bridgekeeper[bot]

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

7506: Incorrect numeric formatting of PID by JMC

Reviewed-by: bdutheil

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

  • 92750098a2deac54b8165ec5c6d368be00707856: 8220: Make LocalJVMToolkit reflective
  • 57e386adb82b81d228a5f5ab114ef02c28cc8930: 7948: Update ASM version in the Agent
  • 12dba597abd009f41e97f47ac4197f0c5ee9fc0e: 8212: Update trunk to 9.1.0

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. 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.

openjdk[bot] avatar Mar 15 '24 14:03 openjdk[bot]

For the JVM PID 2177: image

Before the Change: JVM Internal Page Screenshot 2024-03-25 103611

Event Browser Page Screenshot 2024-03-25 104001

After the Change: JVM Internal Page Screenshot 2024-03-25 104207

Event Browser Page Screenshot 2024-03-25 104343

Suchitainf avatar Mar 25 '24 05:03 Suchitainf

I posted a diff for an alternative solution for this in the JMC slack. @Suchitainf is looking at it.

thegreystone avatar Apr 04 '24 16:04 thegreystone

@Suchitainf This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar May 02 '24 19:05 bridgekeeper[bot]

I posted a diff for an alternative solution for this in the JMC slack. @Suchitainf is looking at it.

Oh I missed that message.

bric3 avatar May 17 '24 11:05 bric3

MIght want to check so that there isn't any use (except for the attribute in the SyntheticAttributeExtension) of JVM_PID (attribute or aggregator) anywhere else in JMC. For example in the JVMInformationPage@237.

thegreystone avatar May 22 '24 18:05 thegreystone

MIght want to check so that there isn't any use (except for the attribute in the SyntheticAttributeExtension) of JVM_PID (attribute or aggregator) anywhere else in JMC. For example in the JVMInformationPage@237.

There is only one usage of JVM_PID which is in JVMInformationPage@237. I have searched entire workspace on eclipse.

Suchitainf avatar May 22 '24 20:05 Suchitainf

MIght want to check so that there isn't any use (except for the attribute in the SyntheticAttributeExtension) of JVM_PID (attribute or aggregator) anywhere else in JMC. For example in the JVMInformationPage@237.

There is only one usage of JVM_PID which is in JVMInformationPage@237. I have searched entire workspace on eclipse.

One could argue that there should be a PID aggregator, and that the JVM_PID one should be deprecated.

thegreystone avatar May 23 '24 20:05 thegreystone

New PR https://github.com/openjdk/jmc/pull/565

Suchitainf avatar May 25 '24 21:05 Suchitainf