jmx scraper with sdk autoconfig #1651

SylvainJuge · 2025-01-17T15:35:16Z

Refactor jmx scraper configuration to use autoconfiguration.

This provides the following benefits:

allow using environment variables in addition to java system properties
override the default configuration using SDK API : config provider and config customizer

Now only the the otel.metric.export.interval SDK configuration should be used to define the metric sampling and export interval, compatibility is provided with existing otel.jmx.interval.milliseconds when needed.

When otel.metric.export.interval is not set, the default of 1 minute is applied which remains consistent with the SDK implementation and specification.

The ability to provide configuration through standard input, external properties and the JVM system properties have been preserved.

To be discussed

Should we align the default exporter to be otlp to align with the SDK default value, or should we keep the logging as we have currently when it's not set. Setting logging here by default mostly benefits onboarding as users can see what is being captured, but requires to explicitly include otlp which is set by default for the SDK autoconfiguration.
- Answer: let's align

Follow-up tasks

In instrumentation (JMX insights), the otel.jmx.config option provides the path to a list of custom YAML metrics definitions, we currently use otel.jmx.custom.scraping.config which does not allow to use a list. Dealing with this was left outside of this PR and will be handled by this #1662.

…ontrib into sdk-autoconfig

SylvainJuge · 2025-01-17T15:35:51Z

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

@@ -24,7 +24,6 @@ public class JmxScraperContainer extends GenericContainer<JmxScraperContainer> {
  private final String endpoint;
  private final Set<String> targetSystems;
  private String serviceUrl;
-  private int intervalMillis;


[for reviewer] this is never changed for tests, so we can use the minimal hard-coded value of 1s.

SylvainJuge · 2025-01-17T15:39:12Z

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

+  static void propagateToSystemProperties(Properties properties) {
+    for (Map.Entry<Object, Object> entry : properties.entrySet()) {
+      String key = entry.getKey().toString();
+      String value = entry.getValue().toString();
+      if (key.startsWith("javax.net.ssl.keyStore") || key.startsWith("javax.net.ssl.trustStore")) {
+        if (System.getProperty(key) == null) {
+          System.setProperty(key, value);
+        }
+      }
+    }
+  }


[for reviewer] propagation of system properties is only used to set the java keystore/truststore options from the program arguments/standard input to the global JVM settings, so it was simpler to move it here. In practice this will likely be tested when we add tests with custom keystore/truststore by setting those configuration options from the standard input or the properties file.

SylvainJuge · 2025-01-17T15:40:12Z

...scraper/src/main/java/io/opentelemetry/contrib/jmxscraper/config/ConfigurationException.java

-
-package io.opentelemetry.contrib.jmxscraper.config;
-
-public class ConfigurationException extends Exception {


[for reviewer] there is already a similarly named runtime exception in the SDK autoconfiguration, so reusing it makes more sense and avoid ambiguity.

SylvainJuge · 2025-01-17T15:43:46Z

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

+    if (exportInterval == null || exportInterval.isNegative() || exportInterval.isZero()) {
+      // if not explicitly set, use default value of 1 minute as defined in specification
+      scraperConfig.samplingInterval = Duration.ofMinutes(1);
+    } else {
+      scraperConfig.samplingInterval = exportInterval;


[for reviewer] we have to do this because we don't have access to the default value that will be used by the SDK when an explicit value is not present.

SylvainJuge · 2025-01-17T15:46:19Z

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

+
+  @Override
+  public Map<String, String> apply(ConfigProperties config) {
+    Map<String, String> result = new HashMap<>();


[for reviewer] the returned map contains the configuration items that will be explicitly set/overriden so that they can be later read by the SDK initialization. In addition to that we use this to build an instance of JmxScraperConfig from the provided ConfigProperties that will later be used to configure the jmx scraper.

breedx-splk · 2025-01-17T20:09:48Z

Regarding the question of default logger -- I think we should continue to be consistent with the sdk and default to exporting otlp to localhost. 👍🏻 If a user needs to see details/logging they should be able to opt-into that I think.

SylvainJuge · 2025-01-20T09:00:27Z

Regarding the question of default logger -- I think we should continue to be consistent with the sdk and default to exporting otlp to localhost. 👍🏻 If a user needs to see details/logging they should be able to opt-into that I think.

I agree here, staying consistent with the SDK defaults would provide a more consistent experience.

Also:

using "logging" mostly benefits the onboarding/setup phase, which is relatively short compared to the time it's being used with a real exporter, so we should make the most common use-case the easiest to use. Logging here is mostly relevant to investigate/debug.
this makes the jmx scraper "do nothing by default", which makes it appear "broken by default unless you configure it", in particular when the logs/standard outpout is not directly visible, it would be easy to not notice that nothing is being sent to OTLP.
if it was "otlp,logging" by default it would have at least do something useful by default, but then having to explicitly disable logging for production use is an extra burden.

Thus I'll align with the SDK unless we have a clear use-case that justifies it.

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

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

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

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

breedx-splk · 2025-01-23T23:39:05Z

jmx-scraper/README.md

+| `otel.jmx.username`               | user name for JMX connection                                                                 |
+| `otel.jmx.password`               | password for JMX connection                                                                  |


Doesn't have to be in this PR of course, but it might be nice to have an indication of which configs are required/optional and/or what the default values are.

I tried to do that with dedicated column but this wasn't looking great because

there is only a single mandatory option for the remote URL

other arguments are mandatory depending on the context (authentication enabled on remote JVM, if there is already a target system set or a custom yaml file used).

This is why I decided to document this mostly in the config option descriptions, I've clarified that for jmx authentication in d237a56

breedx-splk · 2025-01-23T23:40:40Z

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

+ * Exception indicating something is wrong with the provided arguments or reading the configuration
+ * from them
+ */
+public class InvalidArgumentException extends Exception {


Is this really providing any benefit over the built-in jdk IllegalArgumentException?

The main benefit is that it's a checked exception and that it forces the caller to take care of it. However, given the very limited scope this has, we could definitely replace it with the IllegalArgumentException which is a runtime exception if you think that makes things less confusing.

breedx-splk · 2025-01-23T23:49:27Z

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

+    targetSystem.forEach(
+        s -> {
+          if (!AVAILABLE_TARGET_SYSTEMS.contains(s)) {
+            throw new ConfigurationException("unsupported target system: '" + s + "'");


Doesn't this get hidden because it's buried within a closure within forEach?

Could consider switching to if(!targetSystem.stream().allMatch(AVAILABLE_TARGET_SYSTEMS.contains(s)) ... or similar? Maybe a filter if the detailed logging of which system is necessary.

The exception does not get hidden here, this case is covered by the JmxScraperConfigTest#shouldFailValidation_invalidTargetSystem test case.

breedx-splk

Had a few minor comments but LGTM! Stoked to have this autoconfig power.

SylvainJuge added 14 commits January 8, 2025 15:33

add properties supplier

e7747ee

add config customizer

d3afdcd

add test util

3f17316

arg exception with cause

252e554

wip properties customizer

3fbf71d

wip

45ce9be

add some test coverage

96c6250

simplify a bit

1a4ab3e

fix global sdk registration

6793bbe

reuse metric sdk export interval by default + test

6924d9c

log configuration corner cases

ce43846

refactor to use logger for text output

1271bba

log export interval and simplify a bit config further

62d41a5

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

44e8f6e

…ontrib into sdk-autoconfig

SylvainJuge requested a review from a team as a code owner January 17, 2025 15:35

github-actions bot assigned breedx-splk, robsunday and SylvainJuge Jan 17, 2025

github-actions bot requested review from breedx-splk and robsunday January 17, 2025 15:35

SylvainJuge commented Jan 17, 2025

View reviewed changes

SylvainJuge added 3 commits January 21, 2025 11:08

update documentation

0f25c60

add TODO

20c5cc9

set default exporter to otlp

7288ef3

SylvainJuge mentioned this pull request Jan 21, 2025

[jmx-scraper] align custom configuration with instrumentation #1662

Open

SylvainJuge added 2 commits January 21, 2025 11:47

markdown lint

55a1e0e

fix typo

dc064db

robsunday reviewed Jan 22, 2025

View reviewed changes

SylvainJuge added 2 commits January 23, 2025 09:58

post-review changes

857ac4d

post-review changes

646e156

breedx-splk reviewed Jan 23, 2025

View reviewed changes

breedx-splk approved these changes Jan 23, 2025

View reviewed changes

add when jmx auth config are mandatory

d237a56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jmx scraper with sdk autoconfig #1651

jmx scraper with sdk autoconfig #1651

SylvainJuge commented Jan 17, 2025 •

edited

Loading

SylvainJuge Jan 17, 2025

SylvainJuge Jan 17, 2025

SylvainJuge Jan 17, 2025

SylvainJuge Jan 17, 2025

SylvainJuge Jan 17, 2025

breedx-splk commented Jan 17, 2025

SylvainJuge commented Jan 20, 2025

breedx-splk Jan 23, 2025

SylvainJuge Jan 24, 2025

breedx-splk Jan 23, 2025

SylvainJuge Jan 24, 2025

breedx-splk Jan 23, 2025

SylvainJuge Jan 24, 2025

breedx-splk left a comment


		package io.opentelemetry.contrib.jmxscraper.config;

		public class ConfigurationException extends Exception {

		\| `otel.jmx.username` \| user name for JMX connection \|
		\| `otel.jmx.password` \| password for JMX connection \|

jmx scraper with sdk autoconfig #1651

Are you sure you want to change the base?

jmx scraper with sdk autoconfig #1651

Conversation

SylvainJuge commented Jan 17, 2025 • edited Loading

To be discussed

Follow-up tasks

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

breedx-splk commented Jan 17, 2025

SylvainJuge commented Jan 20, 2025

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

breedx-splk left a comment

Choose a reason for hiding this comment

SylvainJuge commented Jan 17, 2025 •

edited

Loading