jmx scraper with sdk autoconfig #1651
Conversation
…ontrib into sdk-autoconfig
| @@ -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; | |||
There was a problem hiding this comment.
[for reviewer] this is never changed for tests, so we can use the minimal hard-coded value of 1s.
| 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); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[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.
|
|
||
| package io.opentelemetry.contrib.jmxscraper.config; | ||
|
|
||
| public class ConfigurationException extends Exception { |
There was a problem hiding this comment.
[for reviewer] there is already a similarly named runtime exception in the SDK autoconfiguration, so reusing it makes more sense and avoid ambiguity.
| 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; |
There was a problem hiding this comment.
[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.
|
|
||
| @Override | ||
| public Map<String, String> apply(ConfigProperties config) { | ||
| Map<String, String> result = new HashMap<>(); |
There was a problem hiding this comment.
[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.
|
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. |
Refactor jmx scraper configuration to use autoconfiguration.
This provides the following benefits:
Now only the the
otel.metric.export.intervalSDK configuration should be used to define the metric sampling and export interval, compatibility is provided with existingotel.jmx.interval.millisecondswhen needed.When
otel.metric.export.intervalis 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
otlpto align with the SDK default value, or should we keep theloggingas we have currently when it's not set. Settinglogginghere by default mostly benefits onboarding as users can see what is being captured, but requires to explicitly includeotlpwhich is set by default for the SDK autoconfiguration.