Implement getAttributes() method of MBean, and update Number-based MBean attributes to return Number objects instead of Strings

cgossett commented 1 week ago

User description

Thanks for contributing to Selenium! A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines. Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This change implements the getAttributes() method on the JMX MBean object(s), returning an AttributeList with the current attributes of the MBean.

It also updates the return type of the MBean attributes which are Number based to their appropriate Number data type instead of String objects.

Motivation and Context

Motivation for this change is driven by trying to export the custom JMX metrics out of the Grid and into Prometheus, with the end goal being that those metrics once in Prometheus in an OpenShift environment can drive autoscaling of Node pods for a Grid instance. The JMX exporter in question is the Prometheus JMX Exporter, which leverages the getAttributes() method of any DynamicMBeans in order to determine if any attributes should be exported.

If implemented, this addition will allow those running Selenium Grid to more easily export the custom Grid metrics into a Prometheus environment.

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x] I have read the contributing document.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

PR Type

Enhancement, Tests

Description

Implemented the getAttributes() method in the MBean class to return an AttributeList with current attributes.
Updated the getAttribute method to correctly handle and parse Number types.
Enhanced error handling in the getAttribute method to include ParseException.
Added comprehensive unit tests for the getAttributes method.
Updated existing tests to ensure MBean attributes return Number types instead of String.
Introduced a helper method getAttributeList in tests to streamline attribute retrieval.

Changes walkthrough 📝

Relevant files

Enhancement

MBean.java

Implement `getAttributes` method and update `getAttribute` handling

java/src/org/openqa/selenium/grid/jmx/MBean.java

Implemented getAttributes() method to return an AttributeList with
current MBean attributes.

Updated getAttribute method to handle Number types and parse them
correctly.

Added error handling for ParseException.

+22/-2

Tests

JmxTest.java

Add unit tests for `getAttributes` and update attribute type checks

java/test/org/openqa/selenium/grid/router/JmxTest.java

Added unit tests for the getAttributes method.

Updated existing tests to validate that MBean attributes return Number
types instead of String.

Introduced helper method getAttributeList to streamline attribute
retrieval in tests.

+76/-24

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

CLAassistant commented 1 week ago

All committers have signed the CLA.

codiumai-pr-agent-pro[bot] commented 1 week ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	3
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The use of `NumberFormat.getInstance().parse(res.toString())` in `getAttribute` method might not be the most efficient or error-free way to convert numbers. Consider using more direct methods like `Integer.valueOf()` or `Double.valueOf()` depending on the expected type.
⚡ Key issues to review	Exception Handling: The broad catch of `Exception` in the `getAttributes` method might mask other unexpected runtime issues. It's generally better to catch more specific exceptions if possible.

codiumai-pr-agent-pro[bot] commented 1 week ago

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Catch specific exceptions instead of a generic `Exception` in the `getAttributes` method ___ Instead of catching a generic `Exception` in the `getAttributes` method, catch specific exceptions to avoid masking other potential issues. [java/src/org/openqa/selenium/grid/jmx/MBean.java [259-261]](https://github.com/SeleniumHQ/selenium/pull/14153/files#diff-8b6d54f14ba5ddec40d9d68412689a031f18d1583478aa78007f3740510e0709R259-R261) ```diff -} catch (Exception e) { +} catch (AttributeNotFoundException \| MBeanException \| ReflectionException e) { LOG.severe("Error during execution: " + e.getMessage()); } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: Catching specific exceptions instead of a generic `Exception` is a best practice as it prevents masking other potential issues and helps in handling exceptions more accurately.	8
Performance	Return `Number` directly instead of parsing its string representation back to a number ___ Use `Number` directly instead of parsing the string representation back to a number in the `getAttribute` method. [java/src/org/openqa/selenium/grid/jmx/MBean.java [228]](https://github.com/SeleniumHQ/selenium/pull/14153/files#diff-8b6d54f14ba5ddec40d9d68412689a031f18d1583478aa78007f3740510e0709R228-R228) ```diff -return NumberFormat.getInstance().parse(res.toString()); +return res; ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: Returning the `Number` object directly is more efficient and cleaner than parsing its string representation back to a number, thus improving performance.	7
Maintainability	Extract repeated parsing and assertion logic into a helper method ___ Extract the repeated parsing and assertion logic into a helper method to reduce code duplication and improve readability. [java/test/org/openqa/selenium/grid/router/JmxTest.java [148-152]](https://github.com/SeleniumHQ/selenium/pull/14153/files#diff-89598c0635d3d9b749c357e36e67a49f5f6d979e90d8231b058659bcd453d3dfR148-R152) ```diff -assertThat(Integer.parseInt(currentSessions.toString())).isZero(); -assertThat(Integer.parseInt(maxSessions.toString())).isEqualTo(1); +assertNumberAttribute(currentSessions, 0); +assertNumberAttribute(maxSessions, 1); +private void assertNumberAttribute(Object attribute, int expectedValue) { + assertThat(attribute).isNotNull(); + assertThat(Integer.parseInt(attribute.toString())).isEqualTo(expectedValue); +} + ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: Extracting repeated logic into a helper method reduces code duplication and improves readability, which enhances maintainability.	7
Possible bug	Add null checks before parsing attributes to avoid potential `NullPointerException` ___ Add assertions to check for null values before parsing the attributes to avoid potential `NullPointerException`. [java/test/org/openqa/selenium/grid/router/JmxTest.java [148]](https://github.com/SeleniumHQ/selenium/pull/14153/files#diff-89598c0635d3d9b749c357e36e67a49f5f6d979e90d8231b058659bcd453d3dfR148-R148) ```diff +assertThat(currentSessions).isNotNull(); assertThat(Integer.parseInt(currentSessions.toString())).isZero(); ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding null checks before parsing attributes is a crucial safety measure to prevent `NullPointerException`, which can cause runtime errors.	6

SeleniumHQ / selenium