Integration test activemq #1497

robsunday · 2024-10-15T13:44:08Z

Primary goal of this PR is to use JMX Metric Insight to gather ActiveMQ JMX properties while preserving compatibility with JMX Gatherer.

At the moment there are two lines of ActiveMQ product:

Classic
Artemis

This test covers Classic line. Artemis line have different set of JMX attributes, so it should have separate yaml and test (nice summary of differences is here: https://www.datadoghq.com/blog/activemq-architecture-and-metrics/). Artemis support may be included in the future.

Assertions in the ActiveMQ integration test are copied from opentelemetry-java/contrib/jmx-metrics while activemq.yaml is taken from opentelemetry-java-instrumentation/jmx-metrics (JMX Metric Insight)
Minor changes were appiled to both of them. These changes are described as comments in the files.

…ontrib into scraper-connect

refactor in a single method + reuse available port detect

use fixed port for container-to-container

…tivemq

robsunday · 2024-10-15T13:46:13Z

...ionTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/ActiveMqIntegrationTest.java

+                "activemq.consumer.count",
+                "The number of consumers currently reading from the broker.",
+                "consumers",
+                /* isMonotonic= */ false,


[for reviewer] isMonotonic=false was added due to updatedDefinition of asserSumWithAttributes.

For all of the 3 metrics that are non-monotonic here I think it was probably a bug in the original test of the groovy version. The fact that they all have current in their description means that they should represent a value that can go up or down, and thus can't be monotonic.

robsunday · 2024-10-15T13:46:27Z

...ionTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/ActiveMqIntegrationTest.java

+                "activemq.producer.count",
+                "The number of producers currently attached to the broker.",
+                "producers",
+                /* isMonotonic= */ false,


[for reviewer] isMonotonic=false was added due to updatedDefinition of asserSumWithAttributes.

robsunday · 2024-10-15T13:46:34Z

...ionTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/ActiveMqIntegrationTest.java

+                "activemq.connection.count",
+                "The total number of current connections.",
+                "connections",
+                /* isMonotonic= */ false,


[for reviewer] isMonotonic=false was added due to updatedDefinition of asserSumWithAttributes.

robsunday · 2024-10-15T13:51:41Z

jmx-scraper/src/main/java/io/opentelemetry/contrib/jmxscraper/JmxScraper.java

@@ -58,6 +77,9 @@ public static void main(String[] args) {
    } catch (IOException e) {
      System.err.println("Unable to connect " + e.getMessage());
      System.exit(2);
+    } catch (RuntimeException e) {
+      e.printStackTrace(System.err);


Simple printing Exception message is not enough here. We lose a lot of information regarding the issue. Stacktrace is better

jmx-scraper/src/main/resources/activemq.yaml

robsunday · 2024-10-15T14:13:56Z

jmx-scraper/src/main/resources/activemq/env

Once we merge it will be good to update resources folder structure. I did not want to do too much in this PR.

robsunday · 2024-10-16T09:55:41Z

...ionTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/ActiveMqIntegrationTest.java

+    return new GenericContainer<>(
+            new ImageFromDockerfile()
+                .withDockerfileFromBuilder(
+                    builder -> builder.from("apache/activemq-classic:5.18.6").build()))


[for reviewer] Updated activemq docker image to the most recent ActiveMQ Classic version (from rmohr/activemq:5.15.9 used in JMX Metric Gatherer)

robsunday · 2024-10-16T09:58:04Z

...ntegrationTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/MetricAssertions.java

+  }
+
+  @SafeVarargs
+  static void assertSumWithAttributes(


[for reviewer] This override method is added to support 'monotonic' flag and metric attributes at he same time.

trask · 2024-10-17T15:04:25Z

@robsunday @SylvainJuge all good to merge here? thanks

SylvainJuge · 2024-10-17T15:53:26Z

@trask yes, given there is no user of this yet there can't be any regression :-).

trask · 2024-10-17T19:38:12Z

thanks, @robsunday mentioned on slack that he may push one more update, will wait for his confirmation

trask · 2024-10-18T04:13:25Z

...ionTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/ActiveMqIntegrationTest.java

+        .withCopyFileToContainer(
+            MountableFile.forClasspathResource("activemq/env"), "/opt/apache-activemq/bin/env")


can you add a comment why we need to customize activemq/env? thanks

Finally I got rid of this file at all

Co-authored-by: SylvainJuge <[email protected]>

SylvainJuge · 2024-10-18T08:25:25Z

jmx-scraper/src/main/resources/activemq.yaml

@@ -10,11 +10,15 @@ rules:
    mapping:
      ProducerCount:
        metric: producer.count
+        # Unit name inherited from activemq.groovy file.


[minor] for those adding a TODO to make sure we don't forget about this could be nice (we are more likely to look for TODOs than read current definitions YAML files).

Eventually we are going to use original *.yaml files from JMX Insights, right? I think we may not need to update this file and added these comments for informational purposes only. But if you have different opinion then I can add TODOs :-), not a problem.

Yes, those are likely to remain "as is", and regarding their location having them in a dedicated folder might also make sense when we reorganize but it's another topic.

I'm just trying to not to forget about this, but a TODO here is probably not the best choice, it's probably better to add this to the issue where we listed all the supported systems for feature parity: open-telemetry/opentelemetry-java-instrumentation#12158 so I've updated it to ensure we don't forget about it.

SylvainJuge · 2024-10-18T08:29:06Z

...ionTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/ActiveMqIntegrationTest.java

+            // Overwrite default ActiveMQ configuration in order to let ActiveMQ use JMX options
+            // stored in $ACTIVEMQ_JMX_OPTS env variable defined below


Maybe not for this PR, but do you know if there is a way to avoid adding the whole env file as an alternative ? For example could an env variable be used instead ? Or maybe as we are in a containers the JAVA_TOOL_OPTIONS might work as well.

Ideally if we don't have extra files to manage for those containers, the easier it gets to maintain them, even if I agree that this one does not seem very complicated.

This is actually a very good point. Replacing file with JAVA_TOOL_OPTIONS was trivial and worked. '''env''' file was used in jmx-metrics and ported here, but really was not needed.
Great suggestion!

Removed env file and use JAVA_TOOL_OPTIONS instead to pass JMX options to ActiveMQ

robsunday · 2024-10-18T09:45:51Z

...ationTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/TomcatIntegrationTest.java

@@ -30,7 +30,6 @@ protected GenericContainer<?> createTargetContainer(int jmxPort) {
                                "https://tomcat.apache.org/tomcat-9.0-doc/appdev/sample/sample.war",
                                "/usr/local/tomcat/webapps/ROOT.war")
                            .build()))
-        .withEnv("LOCAL_JMX", "no")


This is not needed for Tomcat.

robsunday and others added 22 commits September 30, 2024 15:46

Additional logger call

49cfb99

Test exception thrown

a8f8ace

Spotless fix

e1043d1

add snapshot dependency

5948f57

first working poc with tomcat

7f7681b

add comments for future config enhancement

93bf770

fix typo

ac67d0f

add tomcat yaml config

97038df

fix test logging + fail on unsupported system

67a50a0

Integration tests are now working on macOS

26efa72

plug things together + test JVM & Tomcat

edb3d6e

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-c…

3a36466

…ontrib into scraper-connect

change case of exceptions msg

70d0bb0

refactor in a single method

12e6533

cleanup and reuse free port impl.

172846c

revert port selector class

898c3c8

Merge pull request #1 from SylvainJuge/jmx-scraper-integration-tests

da8f4d8

refactor in a single method + reuse available port detect

use fixed port for container-to-container

2260258

Merge pull request #2 from SylvainJuge/fixed-port-for-cross-container

59e1e6f

use fixed port for container-to-container

Spotless fix

df4e149

Merge branch 'jmx-scraper-integration-tests' into integration-test-ac…

80f5e88

…tivemq

ActiveMQ Classic integration test

055813c

github-actions bot assigned breedx-splk, robsunday and SylvainJuge Oct 15, 2024

github-actions bot requested review from breedx-splk and SylvainJuge October 15, 2024 13:44

robsunday commented Oct 15, 2024

View reviewed changes

jmx-scraper/src/main/resources/activemq.yaml Show resolved Hide resolved

robsunday commented Oct 15, 2024

View reviewed changes

jmx-scraper/src/main/resources/activemq.yaml Show resolved Hide resolved

robsunday commented Oct 15, 2024

View reviewed changes

robsunday commented Oct 16, 2024

View reviewed changes

Merge branch 'main' into integration-test-activemq

9d6fd15

robsunday marked this pull request as ready for review October 17, 2024 09:20

robsunday requested a review from a team as a code owner October 17, 2024 09:20

SylvainJuge approved these changes Oct 17, 2024

View reviewed changes

trask reviewed Oct 18, 2024

View reviewed changes

Update jmx-scraper/src/main/resources/activemq.yaml

5b046cb

Co-authored-by: SylvainJuge <[email protected]>

github-actions bot requested a review from SylvainJuge October 18, 2024 07:24

Code review changes

36bc5aa

robsunday requested a review from trask October 18, 2024 08:25

SylvainJuge approved these changes Oct 18, 2024

View reviewed changes

Cleanup unnecessary env variables.

3f7d02f

Removed env file and use JAVA_TOOL_OPTIONS instead to pass JMX options to ActiveMQ

github-actions bot requested a review from SylvainJuge October 18, 2024 09:37

robsunday commented Oct 18, 2024

View reviewed changes

SylvainJuge approved these changes Oct 18, 2024

View reviewed changes

trask approved these changes Oct 18, 2024

View reviewed changes

trask merged commit f8c7cb4 into open-telemetry:main Oct 18, 2024
14 checks passed

SylvainJuge mentioned this pull request Oct 24, 2024

Refactor & merge JMX gatherer & insight implementations #1362

Open

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration test activemq #1497

Integration test activemq #1497

robsunday commented Oct 15, 2024 •

edited

Loading

robsunday Oct 15, 2024

SylvainJuge Oct 17, 2024

robsunday Oct 15, 2024

robsunday Oct 15, 2024

robsunday Oct 15, 2024

robsunday Oct 15, 2024

robsunday Oct 16, 2024 •

edited

Loading

robsunday Oct 16, 2024

trask commented Oct 17, 2024

SylvainJuge commented Oct 17, 2024

trask commented Oct 17, 2024

trask Oct 18, 2024

robsunday Oct 18, 2024

SylvainJuge Oct 18, 2024

robsunday Oct 18, 2024

SylvainJuge Oct 18, 2024

SylvainJuge Oct 18, 2024

robsunday Oct 18, 2024

robsunday Oct 18, 2024 •

edited

Loading

		.withCopyFileToContainer(
		MountableFile.forClasspathResource("activemq/env"), "/opt/apache-activemq/bin/env")

		// Overwrite default ActiveMQ configuration in order to let ActiveMQ use JMX options
		// stored in $ACTIVEMQ_JMX_OPTS env variable defined below

Integration test activemq #1497

Integration test activemq #1497

Conversation

robsunday commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robsunday Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask commented Oct 17, 2024

SylvainJuge commented Oct 17, 2024

trask commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robsunday Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

robsunday commented Oct 15, 2024 •

edited

Loading

robsunday Oct 16, 2024 •

edited

Loading

robsunday Oct 18, 2024 •

edited

Loading