Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for JMX TabularData that uses a CompositeData key #814

Merged

Conversation

adamretter
Copy link
Contributor

Previously the implementation for JMX TabularData did not support TabularData that used composite keys. This PR adds support for that. It also adds a test that should show that this now works.

In eXist-db, we often export data via JMX that uses composite keys for Tabular Data. There is nothing in this PR that is specific to eXist-db, and I suspect the improved functionality in this PR will be be generally useful for all JMX users.

@adamretter adamretter force-pushed the feature/tabular-data-composite-key branch from 7e29ad6 to b747b53 Compare June 19, 2023 16:19
@dhoard
Copy link
Collaborator

dhoard commented Jun 23, 2023

@adamretter Thanks for the PR!

I'm expecting that we will have a release within the week, so I'll have to defer the review until after the release.

@adamretter
Copy link
Contributor Author

adamretter commented Jun 23, 2023

@dhoard Thanks. I don't suppose there might be a chance of getting it into the next release? (cheeky request)

@dhoard
Copy link
Collaborator

dhoard commented Jun 23, 2023

I'll see what I can do.

Can you rebase/retarget the PR based on the development branch?

As part of the upcoming release, the project is moving to a simplified Gitflow branching strategy.

cc @fstab

@dhoard dhoard changed the base branch from main to development June 23, 2023 23:42
@dhoard
Copy link
Collaborator

dhoard commented Jun 24, 2023

@adamretter I changed the PR target and requested a few minor code changes.

We also need an integration test for the change. If you rebase your changes on development, you should be able to create an integration test.

Add a copy of your MBean source to (We don't want a dependency on the collector project.)

https://github.com/prometheus/jmx_exporter/blob/development/integration_test_suite/jmx_example_application/src/main/java/io/prometheus/jmx

... and add it to ...

https://github.com/prometheus/jmx_exporter/blob/development/integration_test_suite/jmx_example_application/src/main/java/io/prometheus/jmx/JmxExampleApplication.java

Copy the MinmalTest class to a CompositeKeyDataTest class and change as required ...

https://github.com/prometheus/jmx_exporter/blob/development/integration_test_suite/integration_tests/src/test/java/io/prometheus/jmx/test/MinimalTest.java

Copy the MinimalTest resources directory to CompositeKeyDataTest directory ...

https://github.com/prometheus/jmx_exporter/tree/development/integration_test_suite/integration_tests/src/test/resources/io/prometheus/jmx/test/MinimalTest

Test ...

mvn clean verify

Once an initial build is done via mvn, you can change the test and re-run via IntelliJ as part of development.

@adamretter adamretter force-pushed the feature/tabular-data-composite-key branch 2 times, most recently from f37e6e2 to 8c04c3d Compare June 26, 2023 10:50
Signed-off-by: Adam Retter <adam.retter@googlemail.com>
@adamretter adamretter force-pushed the feature/tabular-data-composite-key branch from 8c04c3d to acf541a Compare June 26, 2023 11:57
@adamretter
Copy link
Contributor Author

@dhoard I have now added the Integration Tests as requested.

When I run mvn clean verify I see 13 Test Failures, but they appear unrelated to my changes, all seem to be related to BasicAuthentication* tests.

@dhoard
Copy link
Collaborator

dhoard commented Jun 26, 2023

I just checked out your repo/branch and everything passes...

[INFO] ------------------------------------------------------------------------
[INFO] AntuBLUE Test Engine v4.2.2 Summary
[INFO] ------------------------------------------------------------------------
[INFO] Test Classes   :  23, PASSED :  23, FAILED : 0, SKIPPED : 0
[INFO] Test Methods   : 664, PASSED : 664, FAILED : 0, SKIPPED : 0
[INFO] ------------------------------------------------------------------------
[INFO] PASSED
[INFO] ------------------------------------------------------------------------
[INFO] Total Test Time : 2 minutes, 2 seconds, 123 ms (122123 ms)
[INFO] Finished At     : 2023-06-26T08:53:08.565
[INFO] ------------------------------------------------------------------------

... so I suspect it's an environment issue of some sort.

Can you provide your OS version, Java information, and mvn build output?

@adamretter
Copy link
Contributor Author

Can you provide your OS version, Java information, and mvn build output?

@dhoard Sure, here you go:

  • macOS 13.3.1
  • openjdk version "11.0.17" 2022-10-18 LTS
  • Maven 3.8.1

mvn.log

… a CompositeData key

Signed-off-by: Adam Retter <adam.retter@googlemail.com>
@adamretter adamretter force-pushed the feature/tabular-data-composite-key branch from acf541a to f541c93 Compare June 26, 2023 13:56
@dhoard
Copy link
Collaborator

dhoard commented Jun 26, 2023

@adamretter The issue appears to be machine overload, resulting in socket timeouts.

The AntuBLUE Test Engine used for integration testing creates a thread for each processor.

Runtime.getRuntime().availableProcessors()

You can constrain the thread count by using an environment variable.

export ANTUBLUE_TEST_ENGINE_THREAD_COUNT=2
./mvnw clean verify

You should be able to use ANTUBLUE_TEST_ENGINE_THREAD_COUNT=1, but there appears to be a bug in the test engine which needs to be fixed.

@dhoard dhoard merged commit 29b8039 into prometheus:development Jun 26, 2023
1 check passed
@adamretter
Copy link
Contributor Author

Thanks @dhoard I will give that a try tomorrow and report back. Also many thanks for your help and support in getting this merged.

@dhoard
Copy link
Collaborator

dhoard commented Jun 26, 2023

My testing of the PR was successful, so I merged the PR. The test engine issue was fixed and a new version was released.

Note: You still may need to define ANTUBLUE_TEST_ENGINE_THREAD_COUNT=1 or some low thread count if you encounter exceptions caused by timeout issues.

@dhoard dhoard mentioned this pull request Jun 30, 2023
dhoard pushed a commit that referenced this pull request Jun 30, 2023
* [feature] Add support for JMX TabularData that uses a CompositeData key

Signed-off-by: Adam Retter <adam.retter@googlemail.com>

* [test] Add Integration Test for support for JMX TabularData that uses a CompositeData key

Signed-off-by: Adam Retter <adam.retter@googlemail.com>

---------

Signed-off-by: Adam Retter <adam.retter@googlemail.com>
Signed-off-by: Doug Hoard <doug.hoard@gmail.com>
dhoard pushed a commit that referenced this pull request Jun 30, 2023
* [feature] Add support for JMX TabularData that uses a CompositeData key

Signed-off-by: Adam Retter <adam.retter@googlemail.com>

* [test] Add Integration Test for support for JMX TabularData that uses a CompositeData key

Signed-off-by: Adam Retter <adam.retter@googlemail.com>

---------

Signed-off-by: Adam Retter <adam.retter@googlemail.com>
Signed-off-by: Doug Hoard <doug.hoard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants