Reuse JMX Insights in Scraper + Tomcat support #1485

SylvainJuge · 2024-10-02T13:44:01Z

add snapshot dependency
first working poc with tomcat
add comments for future config enhancement
port tomcat YAML definiton from groovy
port jvm YAML definiton from groovy

Description:

Part of #1362

Testing:

Integration tests from JMX Gatherer almost copied as-is for jvm and tomcat systems.

Manual testing on a local tomcat server started with following JVM options in bin/setenv.sh

#!/bin/bash
opts=''
opts="${opts} -Dcom.sun.management.jmxremote.port=12345"
opts="${opts} -Dcom.sun.management.jmxremote.local.only=false"
opts="${opts} -Dcom.sun.management.jmxremote.authenticate=false"
opts="${opts} -Dcom.sun.management.jmxremote.ssl=false"
export CATALINA_OPTS="${opts:-}"

Scraper started with command line (in the ./jmx-scraper subfolder as working directory).

java \
-Dotel.metrics.exporter=otlp,logging \
-Dotel.exporter.otlp.endpoint=${OTEL_EXPORTER_OTLP_ENDPOINT} \
-Dotel.jmx.service.url=service:jmx:rmi:///jndi/rmi://localhost:12345/jmxrmi \
-Dotel.jmx.target.system=tomcat \
-jar ./build/libs/opentelemetry-jmx-scraper-1.40.0-alpha-SNAPSHOT.jar

The explicit otel.exporter.otlp.endpoint and otel.metrics.exporter Java system properties are needed due to a check in the configuration parsing (and they aren't yet read from the env variables), but this will later be refactored to use auto-configuration in a later PR to solve this.

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

robsunday · 2024-10-04T08:57:38Z

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

+        throw new IllegalStateException("no support for " + system);
+      }
+    } catch (Exception e) {
+      throw new IllegalStateException("error while loading rules for system " + system, e);


Other exception would be better here as well

I'll leave this one as-is for now, as we don't have dedicated runtime exceptions, but this could be an improvement later.

robsunday · 2024-10-04T08:58:38Z

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

-  private int intervalMilliseconds;
-  private String metricsExporterType = "";
-  private String otlpExporterEndpoint = "";
+  private int intervalMilliseconds; // TODO only used to set 'otel.metric.export.interval' from SDK


What this TODO means here? Is there anything aditional to be implemented?

Yes, basically this configuration option is only used to configure the metrics polling interval and there is already a dedicated configuration option in the SDK for this, so if someone is used to configure the SDK using this one we should reuse the existing config option (we'll deal with that when adding auto-configuration here).

…ontrib into scraper-connect

...craper/src/integrationTest/java/io/opentelemetry/contrib/jmxscraper/JmxScraperContainer.java

...est/java/io/opentelemetry/contrib/jmxscraper/target_systems/TargetSystemIntegrationTest.java

SylvainJuge · 2024-10-04T15:45:54Z

@robsunday I have just added support for both the JVM metrics and the Tomcat metrics with tests from Gatherer, so we have end-to-end testing now 🎉.

If you have some time for another review that would be great !

...craper/src/integrationTest/java/io/opentelemetry/contrib/jmxscraper/JmxScraperContainer.java

...est/java/io/opentelemetry/contrib/jmxscraper/target_systems/TargetSystemIntegrationTest.java

SylvainJuge · 2024-10-11T13:43:26Z

In the context of integration tests, we always want to send data to otlp as otherwise we don't have a way to validate what it's captured, this is not meant to be a reusable test container outside of those integration tests.

For the filtering on the metrics, there was a filter on the metrics that are sent with a specific sdk version, now that we reuse code from instrumentation we do not get the project version as we used to have, but the version of the instrumentation project which is used as dependency.

trask · 2024-10-16T02:55:19Z

sorry @robsunday, I hit the wrong button and re-requested a review from you, which also cleared your approval, can you re-approve? and let me know if there are any outstanding discussions, and if not I'll merge, thanks!

trask · 2024-10-16T02:58:24Z

@SylvainJuge sorry, can you fix up the merge conflicts from #1480? thanks!

…ontrib into scraper-connect

trask · 2024-10-16T15:09:04Z

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

@@ -35,15 +44,25 @@ public class JmxScraper {
   */
  @SuppressWarnings({"SystemOut", "SystemExitOutsideMain"})
  public static void main(String[] args) {
+
+    // enable SDK auto-configure if not explicitly set by user
+    // TODO: refactor this to use AutoConfiguredOpenTelemetrySdk


trask · 2024-10-16T15:10:58Z

jmx-scraper/build.gradle.kts

+  // TODO: remove snapshot repository once 2.9.0 is released
+  maven {
+    setUrl("https://oss.sonatype.org/content/repositories/snapshots")
+  }


I'm merging anyways since

the jmx-scraper artifact isn't published yet so no worries about relying on snapshot repo from maven central

merging will unblock other work

we expect to update this to 2.9.0 within a week

trask · 2024-10-16T15:12:35Z

...craper/src/integrationTest/java/io/opentelemetry/contrib/jmxscraper/JmxScraperContainer.java

@@ -76,6 +76,7 @@ public void start() {
    // for now only configure through JVM args
    List<String> arguments = new ArrayList<>();
    arguments.add("java");
+    arguments.add("-Dotel.metrics.exporter=otlp");


I think it makes sense to align (long-term) if possible with the autoconfigure SDK defaults

SylvainJuge added 3 commits October 1, 2024 10:38

add snapshot dependency

5948f57

first working poc with tomcat

7f7681b

add comments for future config enhancement

93bf770

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

SylvainJuge added 3 commits October 2, 2024 17:26

fix typo

ac67d0f

add tomcat yaml config

97038df

fix test logging + fail on unsupported system

67a50a0

robsunday approved these changes Oct 4, 2024

View reviewed changes

SylvainJuge added 2 commits October 4, 2024 17:33

plug things together + test JVM & Tomcat

edb3d6e

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

3a36466

…ontrib into scraper-connect

github-actions bot requested a review from breedx-splk October 4, 2024 15:34

SylvainJuge commented Oct 4, 2024

View reviewed changes

...craper/src/integrationTest/java/io/opentelemetry/contrib/jmxscraper/JmxScraperContainer.java Show resolved Hide resolved

SylvainJuge commented Oct 4, 2024

View reviewed changes

...est/java/io/opentelemetry/contrib/jmxscraper/target_systems/TargetSystemIntegrationTest.java Show resolved Hide resolved

SylvainJuge marked this pull request as ready for review October 4, 2024 15:44

SylvainJuge requested a review from a team as a code owner October 4, 2024 15:44

SylvainJuge mentioned this pull request Oct 4, 2024

Refactor & merge JMX gatherer & insight implementations #1362

Open

22 tasks

change case of exceptions msg

70d0bb0

SylvainJuge mentioned this pull request Oct 11, 2024

Integration tests for JMX Scraper #1480

Merged

robsunday approved these changes Oct 11, 2024

View reviewed changes

...craper/src/integrationTest/java/io/opentelemetry/contrib/jmxscraper/JmxScraperContainer.java Show resolved Hide resolved

...est/java/io/opentelemetry/contrib/jmxscraper/target_systems/TargetSystemIntegrationTest.java Show resolved Hide resolved

robsunday mentioned this pull request Oct 15, 2024

Integration test activemq #1497

Merged

trask requested a review from robsunday October 16, 2024 02:53

SylvainJuge added 3 commits October 16, 2024 09:39

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

95c0b59

…ontrib into scraper-connect

log received metrics at debug level

69f0fd1

mapped port not mapped anymore

3362166

robsunday approved these changes Oct 16, 2024

View reviewed changes

SylvainJuge mentioned this pull request Oct 16, 2024

JMX: Tomcat sessions is always exported as 0 #1360

Closed

robsunday mentioned this pull request Oct 16, 2024

REQUEST: New membership for robsunday open-telemetry/community#2401

Closed

6 tasks

trask approved these changes Oct 16, 2024

View reviewed changes

trask merged commit dc66266 into open-telemetry:main Oct 16, 2024
14 checks passed

trask mentioned this pull request Oct 16, 2024

fix(deps): update dependency io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha to v2.10.0-alpha-snapshot - autoclosed #1500

Closed

1 task

SylvainJuge deleted the scraper-connect branch October 16, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse JMX Insights in Scraper + Tomcat support #1485

Reuse JMX Insights in Scraper + Tomcat support #1485

SylvainJuge commented Oct 2, 2024 •

edited

Loading

robsunday Oct 4, 2024

SylvainJuge Oct 4, 2024

robsunday Oct 4, 2024

SylvainJuge Oct 4, 2024

SylvainJuge commented Oct 4, 2024

SylvainJuge commented Oct 11, 2024

trask commented Oct 16, 2024

trask commented Oct 16, 2024

trask Oct 16, 2024

trask Oct 16, 2024

trask Oct 16, 2024

Reuse JMX Insights in Scraper + Tomcat support #1485

Reuse JMX Insights in Scraper + Tomcat support #1485

Conversation

SylvainJuge commented Oct 2, 2024 • edited Loading

robsunday Oct 4, 2024

Choose a reason for hiding this comment

SylvainJuge Oct 4, 2024

Choose a reason for hiding this comment

robsunday Oct 4, 2024

Choose a reason for hiding this comment

SylvainJuge Oct 4, 2024

Choose a reason for hiding this comment

SylvainJuge commented Oct 4, 2024

SylvainJuge commented Oct 11, 2024

trask commented Oct 16, 2024

trask commented Oct 16, 2024

trask Oct 16, 2024

Choose a reason for hiding this comment

trask Oct 16, 2024

Choose a reason for hiding this comment

trask Oct 16, 2024

Choose a reason for hiding this comment

SylvainJuge commented Oct 2, 2024 •

edited

Loading