Add jetty.yaml to JMX scraper #1517

SylvainJuge · 2024-10-24T13:35:37Z

Adds support for jetty system by providing an jetty.yaml and related tests.

Part of #1362

Tests had to be slightly adapted from their groovy counterparts to make it work as expected, but captured metrics are compatible (some of them only have a few extra attributes).

Adds a genericJmxJvmArguments method to generate common JMX JVM arguments to remove some documentation.

Bonus:

making cassandra startup faster as it's a single isolated node.
fixing some minor inconsistencies with plural units and align with stable conventions: https://opentelemetry.io/docs/specs/semconv/general/metrics/#instrument-units

…ontrib into jmx-scraper-jetty

SylvainJuge · 2024-10-24T13:37:41Z

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

@@ -136,4 +153,19 @@ private static void assertAttributedPoints(
                            KeyValue::getKey, keyValue -> keyValue.getValue().getStringValue())))
        .satisfiesExactlyInAnyOrder(assertions);
  }
+
+  @SuppressWarnings("unchecked")
+  private static void assertAttributedMultiplePoints(


[for reviewer] the difference with assertAttributedPoints is that we check all the provided assertions to all the data points, whereas assertAttributedPoints expects to have exactly one assertion per point.

...rationTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/JettyIntegrationTest.java

robsunday · 2024-10-28T16:16:19Z

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

@@ -196,4 +198,27 @@ public void export(
      sb.http(0);
    }
  }
+
+  protected static String genericJmxJvmArguments(int port) {


Good change! However it is a bit over-complicated in my opinion. Simple StringBuilder and additional private method would be enough, no need for maps and streams. It is just my opinion :-)

Yeah, I think I agree...I would have just hard coded a bunch of string concats...but I don't think it's a huge problem.

robsunday · 2024-10-28T16:40:37Z

...rationTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/JettyIntegrationTest.java

+                .withDockerfileFromBuilder(
+                    builder ->
+                        builder
+                            .from("jetty:11")


Did you consider upgrade to jetty 12?

No, ideally we should test all those metrics on multiple versions but it's not the case for now, but it would be a nice improvement for later :-).

breedx-splk

Looks good to me, but offered a couple discussion points.

breedx-splk · 2024-10-30T22:08:01Z

...onTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/CassandraIntegrationTest.java

-                + " -Dcom.sun.management.jmxremote.ssl=false"
-                + " -Dcom.sun.management.jmxremote.authenticate=false")
+            genericJmxJvmArguments(jmxPort)
+                // making cassandra startup faster for single node, from ~1min to ~15s


nice 👍🏻

breedx-splk · 2024-10-30T22:12:07Z

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

@@ -196,4 +198,27 @@ public void export(
      sb.http(0);
    }
  }
+
+  protected static String genericJmxJvmArguments(int port) {


Yeah, I think I agree...I would have just hard coded a bunch of string concats...but I don't think it's a huge problem.

breedx-splk · 2024-10-30T22:15:51Z

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

+        # Unit should not use a plural form, left as-is for compatibility with jetty.groovy
+        # https://opentelemetry.io/docs/specs/semconv/general/metrics/#instrument-units
+        unit: "{operations}"


Is there a reason that we're trying to keep this compatibility instead of fixing it?

Yes, I think this is better as a first step to stay very close to the original implementation (even if it's buggy).
The unit tests are almost copied as-is which helps us prevent any regressions and to ensure that we have the same behavior between the two implementations.

However, I think that it would be a good idea to remove this (and the other ones) as soon as possible with a separate PR that fixes the two implementations (this one in yaml and groovy) at once. This should not be a breaking change as the units are very unlikely to be relied on beyond documentation, the captured values remain the same.

Giving it a second thought on this, the "we'll never be able to fix this if we perpetuate it" changed my mind and I've fixed that directly in this PR with 187ca89.

I was thinking more about the process of replicating existing tests with some fidelity than fixing small inconsistencies while we are at it when we can do it in a non-breaking way.

...rationTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/JettyIntegrationTest.java

…ontrib into jmx-scraper-jetty

SylvainJuge added 10 commits October 17, 2024 16:07

add method to generate generic jmx args

bac90ea

jetty sessions

8b87c8f

jetty sessions duration

57a552f

session max duration

c2ace1a

jetty select count

eacaeab

jetty threads

9378dcf

wip, failing are commented

03b6536

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

8a735ef

…ontrib into jmx-scraper-jetty

finally making it work !!

643618b

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

5df0512

…ontrib into jmx-scraper-jetty

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

github-actions bot requested review from breedx-splk and robsunday October 24, 2024 13:35

SylvainJuge commented Oct 24, 2024

View reviewed changes

SylvainJuge added 3 commits October 24, 2024 16:23

use generic JVM params + faster cassandra startup

4d1989a

spotless

bf569df

remove useless env variable

3aba726

SylvainJuge marked this pull request as ready for review October 24, 2024 14:34

SylvainJuge requested a review from a team as a code owner October 24, 2024 14:34

SylvainJuge mentioned this pull request Oct 24, 2024

Refactor & merge JMX gatherer & insight implementations #1362

Open

22 tasks

robsunday approved these changes Oct 28, 2024

View reviewed changes

add comment about incorrect metrics

a2a1c96

breedx-splk approved these changes Oct 30, 2024

View reviewed changes

SylvainJuge added 4 commits November 4, 2024 11:48

fix minor metrics units

187ca89

simplify JVM params generation

5421f09

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

7861f83

…ontrib into jmx-scraper-jetty

fix spotless

ff8a540

trask merged commit df4960b into open-telemetry:main Nov 4, 2024
14 checks passed

SylvainJuge mentioned this pull request Nov 25, 2024

JMX Scraper - YAML config and integration test for HBase #1538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jetty.yaml to JMX scraper #1517

Add jetty.yaml to JMX scraper #1517

SylvainJuge commented Oct 24, 2024 •

edited

Loading

SylvainJuge Oct 24, 2024

robsunday Oct 28, 2024 •

edited

Loading

breedx-splk Oct 30, 2024

robsunday Oct 28, 2024

SylvainJuge Oct 29, 2024

breedx-splk left a comment

breedx-splk Oct 30, 2024

breedx-splk Oct 30, 2024

breedx-splk Oct 30, 2024

SylvainJuge Nov 4, 2024

SylvainJuge Nov 4, 2024

Add jetty.yaml to JMX scraper #1517

Add jetty.yaml to JMX scraper #1517

Conversation

SylvainJuge commented Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

robsunday Oct 28, 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

breedx-splk left a 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

SylvainJuge commented Oct 24, 2024 •

edited

Loading

robsunday Oct 28, 2024 •

edited

Loading