RFR: 8354508: JFR: Strengthen metadata checks for labels

[Aleksey Shipilev]

On Mon, 14 Apr 2025 08:02:34 GMT, Erik Gahlin <egahlin at 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