RFR: 8354508: JFR: Strengthen metadata checks for labels
Could I have a review of a PR that improves the checks for label metadata? When running TestEventMetadata, I found some fields with incorrect capitalization. I fixed those, as well as some typos, and updated the filename from trace.xml to metadata.xml. Testing: jdk/jdk/jfr Thanks Erik ------------- Commit messages: - Fixes - Initial Changes: https://git.openjdk.org/jdk/pull/24616/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24616&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8354508 Stats: 77 lines in 2 files changed: 46 ins; 5 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/24616.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24616/head:pull/24616 PR: https://git.openjdk.org/jdk/pull/24616
On Mon, 14 Apr 2025 08:02:34 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of a PR that improves the checks for label metadata? When running TestEventMetadata, I found some fields with incorrect capitalization. I fixed those, as well as some typos, and updated the filename from trace.xml to metadata.xml.
Testing: jdk/jdk/jfr
Thanks Erik
Makes sense. Only stylistic nits: Looks reasonable to me, thanks. test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 168:
166: Asserts.assertNotEquals(label, null, "Label not allowed to be null"); 167: Asserts.assertTrue(label.length() > 1, "Label must be at least two characters"); 168: Asserts.assertTrue(label.length() < 45, "Label should not exceed 45 characters, use description to explain");
Message suggests the condition should be `<= 45`? test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 177:
175: Asserts.assertTrue(isCapitalized(firstWord), "Label should capitalize first word"); 176: if (!isNumeric(lastWord)) { 177: Asserts.assertTrue(isCapitalized(lastWord), "Label should capitalize last word");
This looks like a heuristic that last word is very likely a noun, and should be capitalized then? Just curious. test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 181:
179: for (String word : words) { 180: Asserts.assertFalse(word.endsWith("-") || word.startsWith("-"), "Word in label should not start or end with hyphen"); 181: Asserts.assertTrue(word.length() != 0, "Label should not contain superflous whitespace");
"superfluous"? test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 194:
192: } 193: for (char c : label.toCharArray()) { 194: Asserts.assertTrue(isAllowedCharacter(c), "Label should only consist of letters, numbers, hyphens, parantheses or whitespace, found '" + c + "'");
"parentheses"? test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 212:
210: 211: private static boolean isShortCommonPreposition(String word) { 212: String[] prepositions = { "in", "on", "at", "by", "to", "of"};
Suggestion: String[] prepositions = { "in", "on", "at", "by", "to", "of" }; ------------- PR Review: https://git.openjdk.org/jdk/pull/24616#pullrequestreview-2763542035 Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24616#pullrequestreview-2764010341 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041660467 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041676636 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041679340 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041679801 PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041680301
On Mon, 14 Apr 2025 08:37:32 GMT, Aleksey Shipilev <shade@openjdk.org> wrote:
Could I have a review of a PR that improves the checks for label metadata? When running TestEventMetadata, I found some fields with incorrect capitalization. I fixed those, as well as some typos, and updated the filename from trace.xml to metadata.xml.
Testing: jdk/jdk/jfr
Thanks Erik
test/jdk/jdk/jfr/event/metadata/TestEventMetadata.java line 177:
175: Asserts.assertTrue(isCapitalized(firstWord), "Label should capitalize first word"); 176: if (!isNumeric(lastWord)) { 177: Asserts.assertTrue(isCapitalized(lastWord), "Label should capitalize last word");
This looks like a heuristic that last word is very likely a noun, and should be capitalized then? Just curious.
The last word should always be capitalized, but Character.isUpperCase in isCapitalized() returns false for numbers. As a result, a label like "GC Phase Level 1" fails. The isNumeric check avoids that. I will add a comment. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24616#discussion_r2041732863
On Mon, 14 Apr 2025 08:02:34 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of a PR that improves the checks for label metadata? When running TestEventMetadata, I found some fields with incorrect capitalization. I fixed those, as well as some typos, and updated the filename from trace.xml to metadata.xml.
Testing: jdk/jdk/jfr
Thanks Erik
This pull request has now been integrated. Changeset: d7676c39 Author: Erik Gahlin <egahlin@openjdk.org> URL: https://git.openjdk.org/jdk/commit/d7676c39b648bd55f72a50494432b02862a4e111 Stats: 77 lines in 2 files changed: 46 ins; 5 del; 26 mod 8354508: JFR: Strengthen metadata checks for labels Reviewed-by: shade ------------- PR: https://git.openjdk.org/jdk/pull/24616
participants (2)
-
Aleksey Shipilev
-
Erik Gahlin