8244: Turn off scientific notation in JMC for attributes with long data type #572

Suchitainf · 2024-07-28T19:24:56Z

This PR resolves the issue of scientific notation display for attributes with long datatype. This was the common pain-point of several customers and we were getting repeated requests of fixing this issue as they were not able to analyze the data due to this format of display. Since there is very less difference between the values of each row, after converting it to scientific notation all values look-alike and its difficult for customers to analyze the data. We have consulted the JFR team and there was a suggestion of skipping long attributes for this format display as they have followed in JFR-tools also. Same approach I have followed in JMC too. Please provide your valuable feedbacks.

Attaching few screenshots for better reference:

Before:

After:

Before:

After:

Before:

After:

Progress

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

Issue

JMC-8244: Turn off scientific notation in JMC for attributes with long data type (Enhancement - P3)

Reviewers

Brice Dutheil (@bric3 - Committer)
Marcus Hirt (@thegreystone - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 572

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-28T19:25:38Z

👋 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.

openjdk · 2024-07-28T19:26:46Z

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

8244: Turn off scientific notation in JMC for attributes with long data type

Reviewed-by: bdutheil, hirt

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:

4c9d9e4: 7992: Add API to easily write annotated Java JFR events
5016cc1: 8245: Improve XML processing
8a04432: 8236: Add support for Jolokia discovery

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.

mlbridge · 2024-07-28T19:30:03Z

Webrevs

00: Full (0d1726e2)

aptmac

Looks good, although the issue title and description make reference to an option in the preferences that would control this, is this currently in the works or should the issue be updated?

Also not sure if you had seen it, but egahlin left a comment mentioning an approach that jmc 4 and jfr-tool use, which would be to only use scientific notation for floats & doubles, if that's something you'd be interested in as well.

Suchitainf · 2024-07-30T21:12:10Z

Looks good, although the issue title and description make reference to an option in the preferences that would control this, is this currently in the works or should the issue be updated?

Also not sure if you had seen it, but egahlin left a comment mentioning an approach that jmc 4 and jfr-tool use, which would be to only use scientific notation for floats & doubles, if that's something you'd be interested in as well.

Hi @aptmac , Thanks for taking a look at it. Actually the discussion started with giving an option (like in preference), then we had a detailed discussion with customer as well as Erik. Erik suggested to skip scientific notation for long attributes and that should solve the issue. Same concept I have followed in the fix provided as part of this PR. I agree the subject of the issue was bit confusing, I have updated the same now.

bric3

This PR looks good to me for the immediate problem, but I wonder if other predefined "attributes" could benefit from that approach.

bric3 · 2024-08-09T12:34:44Z

...enjdk.jmc.flightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/jdk/JdkAttributes.java

+					};
+				}
+			});
+
 	public static final IAttribute<IQuantity> CLASSLOADER_UNLOADED_COUNT = attr("unloadedClassCount", //$NON-NLS-1$


question: Could this be useful to have that for similar attributes like unloadedClassCount.

yes it resolves issue for all such attributes.

Here is the example of a JFR where unloadedClassCount data is present.

Before the change:

After the change:

bric3 · 2024-08-09T12:44:45Z

...enjdk.jmc.flightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/jdk/JdkAttributes.java

+					return accessor == null ? null : new IMemberAccessor<IQuantity, U>() {
+						@Override
+						public IQuantity getMember(U i) {
+							Number countNumber = accessor.getMember(i);
+							return countNumber == null ? null : UnitLookup.NUMBER_UNITY.quantity(countNumber);
+						}
+					};


question: For readability I wonder of these attributes might be converted to lambda, e.g. :

Suggested change

return accessor == null ? null : new IMemberAccessor<IQuantity, U>() {

@Override

public IQuantity getMember(U i) {

Number countNumber = accessor.getMember(i);

return countNumber == null ? null : UnitLookup.NUMBER_UNITY.quantity(countNumber);

}

};

return accessor == null ? null : i -> {

Number countNumber = accessor.getMember(i);

return countNumber == null ? null : UnitLookup.NUMBER_UNITY.quantity(countNumber);

};

thegreystone · 2024-08-20T01:55:33Z

...enjdk.jmc.flightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/jdk/JdkAttributes.java

@@ -792,9 +793,27 @@ public <U> IMemberAccessor<IMCThread, U> customAccessor(IType<U> type) {
 	public static final IAttribute<String> SHUTDOWN_REASON = attr("reason", //$NON-NLS-1$
 			Messages.getString(Messages.ATTR_SHUTDOWN_REASON), Messages.getString(Messages.ATTR_SHUTDOWN_REASON_DESC),
 			PLAIN_TEXT);
-	public static final IAttribute<IQuantity> CLASSLOADER_LOADED_COUNT = attr("loadedClassCount", //$NON-NLS-1$
+	public static final IAttribute<Number> CLASSLOADER_LOADED_COUNT_NUMBER = attr("loadedClassCount", //$NON-NLS-1$


Note that this is changing the generic type for publicly defined constants, and that we would be breaking people depending on them.

Thanks for the review @thegreystone . I understand this concern, thats why I have retained the attribute with same datatype i.e. IQuantity on line number 800. So basically the internal value is number but I have written a custom accessor to retain the uniformity of IQuantity data type. So that it doesn;t break anything. Please let me know if I am missing something here.

Suchitainf · 2024-08-24T10:33:20Z

/integrate

openjdk · 2024-08-24T10:33:54Z

Going to push as commit ab0f9a6.
Since your change was applied there have been 3 commits pushed to the master branch:

4c9d9e4: 7992: Add API to easily write annotated Java JFR events
5016cc1: 8245: Improve XML processing
8a04432: 8236: Add support for Jolokia discovery

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-24T10:34:01Z

@Suchitainf Pushed as commit ab0f9a6.

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

8244: Option that allows you to turn off scientific notation in JMC

0d1726e

openjdk bot added the rfr label Jul 28, 2024

aptmac reviewed Jul 30, 2024

View reviewed changes

Suchitainf changed the title ~~8244: Option that allows you to turn off scientific notation in JMC~~ 8244: Turn off scientific notation in JMC for attributes with long data type Jul 30, 2024

Suchitainf requested a review from aptmac July 30, 2024 21:12

bric3 approved these changes Aug 9, 2024

View reviewed changes

openjdk bot added the ready label Aug 9, 2024

thegreystone reviewed Aug 20, 2024

View reviewed changes

thegreystone approved these changes Aug 23, 2024

View reviewed changes

openjdk bot added the integrated label Aug 24, 2024

openjdk bot closed this Aug 24, 2024

openjdk bot removed ready rfr labels Aug 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8244: Turn off scientific notation in JMC for attributes with long data type #572

8244: Turn off scientific notation in JMC for attributes with long data type #572

Suchitainf commented Jul 28, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 28, 2024

openjdk bot commented Jul 28, 2024 •

edited

Loading

mlbridge bot commented Jul 28, 2024

aptmac left a comment

Suchitainf commented Jul 30, 2024

bric3 left a comment

bric3 Aug 9, 2024

Suchitainf Aug 13, 2024

bric3 Aug 9, 2024

thegreystone Aug 20, 2024

Suchitainf Aug 20, 2024

Suchitainf commented Aug 24, 2024

openjdk bot commented Aug 24, 2024

openjdk bot commented Aug 24, 2024

8244: Turn off scientific notation in JMC for attributes with long data type #572

8244: Turn off scientific notation in JMC for attributes with long data type #572

Conversation

Suchitainf commented Jul 28, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 28, 2024

openjdk bot commented Jul 28, 2024 • edited Loading

mlbridge bot commented Jul 28, 2024

Webrevs

aptmac left a comment

Choose a reason for hiding this comment

Suchitainf commented Jul 30, 2024

bric3 left a comment

Choose a reason for hiding this comment

bric3 Aug 9, 2024

Choose a reason for hiding this comment

Suchitainf Aug 13, 2024

Choose a reason for hiding this comment

bric3 Aug 9, 2024

Choose a reason for hiding this comment

thegreystone Aug 20, 2024

Choose a reason for hiding this comment

Suchitainf Aug 20, 2024

Choose a reason for hiding this comment

Suchitainf commented Aug 24, 2024

openjdk bot commented Aug 24, 2024

openjdk bot commented Aug 24, 2024

Suchitainf commented Jul 28, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 28, 2024 •

edited

Loading