add wildfly.yaml to JMX scraper #1531
Conversation
…ontrib into jmx-wildfly
| @@ -25,11 +25,12 @@ protected GenericContainer<?> createTargetContainer(int jmxPort) { | |||
| builder -> builder.from("apache/activemq-classic:5.18.6").build())) | |||
| .withEnv("JAVA_TOOL_OPTIONS", genericJmxJvmArguments(jmxPort)) | |||
| .withStartupTimeout(Duration.ofMinutes(2)) | |||
| .waitingFor(Wait.forListeningPort()); | |||
| .waitingFor(Wait.forLogMessage(".*Apache ActiveMQ.*started.*", 1)); | |||
There was a problem hiding this comment.
[for reviewer] waiting for open port seems to sometimes not be accurate enough to detect proper startup, which makes the scraper container start when the JMX port is not yet available. Waiting on log message is an attempt to make this a bit less flaky (time will tell if that's a good idea or not here).
There was a problem hiding this comment.
Wait.forListeningPort() requires withExposedPorts(jmxPort) to be called to work properly. I think Tomcat test works by a chance. I added withExposedPorts to all tests using forListeningPort on my branch, but you can add it in your PR instead
There was a problem hiding this comment.
While this sounds brittle, I think it's slightly better to wait for the target application to be properly started (here we use the log message as a proxy), because while the JMX port might be open early, the MBeans are likely not yet published so while we can connect to it the MBeans that we are trying to access are likely not yet published.
Otherwise do you suggest to add to every container something like this for Tomcat:
protected GenericContainer<?> createTargetContainer(int jmxPort) {
return new GenericContainer<>(
new ImageFromDockerfile()
// ...
.withStartupTimeout(Duration.ofMinutes(2))
.withExposedPorts(8080, jmxPort)
.waitingFor(Wait.forListeningPorts(8080, jmxPort));
}
There was a problem hiding this comment.
As discussed today I've implemented it for all other containers in 3ab2f28.
|
|
||
| - command line arguments: `java -jar scraper.jar --config otel.jmx.target.system=...` | ||
| - system properties `java -jar scraper.jar` | ||
| - java properties file: `java -jar config.properties` |
There was a problem hiding this comment.
I think for now it could be:
- command line arguments:
java -Dotel.jmx.service.url=service:jmx:rmi:///jndi/rmi://tomcat:9010/jmxrmi -Dotel.jmx.target.system=tomcat -jar scraper.jar - java properties file:
java -jar scraper.jar -config config.properties - config provided from stdin:
java -jar scraper.jar -config -then type:otel.jmx.target.system=tomcatand:otel.jmx.service.url=service:jmx:rmi:///jndi/rmi://tomcat:9010/jmxrmi
| @@ -25,11 +25,13 @@ protected GenericContainer<?> createTargetContainer(int jmxPort) { | |||
| builder -> builder.from("apache/activemq-classic:5.18.6").build())) | |||
| .withEnv("JAVA_TOOL_OPTIONS", genericJmxJvmArguments(jmxPort)) | |||
| .withStartupTimeout(Duration.ofMinutes(2)) | |||
| .waitingFor(Wait.forListeningPort()); | |||
| .withExposedPorts(61616, jmxPort) | |||
There was a problem hiding this comment.
It would be more maintainable if some named constant is used instead of just 61616. The same applies to other tests below
| public class WildflyIntegrationTest extends TargetSystemIntegrationTest { | ||
|
|
||
| @SuppressWarnings("NonFinalStaticField") | ||
| private static Path tempJbossClient = null; |
There was a problem hiding this comment.
Maybe we do not need static and SuppressWarnings here? JUnit is managing lifecycle of the test suite. It is creating single instance of the test suite anyway
| private static Path tempJbossClient = null; | ||
|
|
||
| @AfterAll | ||
| public static void cleanup() throws IOException { |
There was a problem hiding this comment.
I see here some room for improvement for our integration tests. Containers are created in @Test in base class. Also temp file is created there, while it is removed in @AfterAll.
In theory we could add more @Test functions that creates more containers, more temp files, etc., so it would be safer to do cleanup in @AfterEach.
As alternative we can consider moving container creation to @BeforeAll
For me current implementation is somehow risky.
| mapping: | ||
| sessionsCreated: | ||
| metric: count | ||
| type: counter |
There was a problem hiding this comment.
minor - could be defined as a common type
| desc: The number of requests that have resulted in a 5xx response. | ||
| bytesSent: | ||
| metric: network.io | ||
| unit: "by" |
There was a problem hiding this comment.
"By" seems to be unit recommended by semconv. Can we just use "By" here?
| type: counter | ||
| prefix: wildfly.jdbc. | ||
| mapping: | ||
| ActiveCount: |
There was a problem hiding this comment.
ActiveCount and IdleCount could use YAML references since they share a lot. The same applies to few props in the bean below. What do you think about it?
Description:
jboss-client.jarfor using HTTP management APIwildfly.groovyto fit the stable convention for metric units.Part of #1362
Notes:
In the original groovy implementation session metrics are not tested, this requires deploying and application to the container, this is not properly tested in the YAML implementation.
In the original groovy implementation the gatherer was run directly from the same docker container probably for simplicity as
jboss-client.jaris directly available.When using two containers, the jar needs to be copied from wildfly container to scraper container and it complicates authentication: login/pwd can be skipped if both processes run on the same host and can access the same filesystem.
Testing here is done using two distinct containers as this setup is a bit more complex and thus likely to fail (doing so here helped me find that we did not handle it properly).