Skip to content

Commit

Permalink
Code review changes:
Browse files Browse the repository at this point in the history
Argument parsing moved to the main class.
Argument validation now throw exception.
More tests added for config factory.
Created unit tests for JmxScraper class.
  • Loading branch information
robsunday committed Sep 17, 2024
1 parent 164679f commit f847561
Show file tree
Hide file tree
Showing 7 changed files with 217 additions and 104 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.jmxscraper;

public class ArgumentsParsingException extends Exception {
private static final long serialVersionUID = 0L;
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,101 @@
import io.opentelemetry.contrib.jmxscraper.config.JmxScraperConfig;
import io.opentelemetry.contrib.jmxscraper.config.JmxScraperConfigFactory;
import io.opentelemetry.contrib.jmxscraper.jmx.JmxClient;
import java.io.DataInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.MalformedURLException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.List;
import java.util.Properties;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.logging.Logger;

public class JmxScraper {
private static final Logger logger = Logger.getLogger(JmxScraper.class.getName());
private static final int EXECUTOR_TERMINATION_TIMEOUT_MS = 5000;
private final ScheduledExecutorService exec = Executors.newSingleThreadScheduledExecutor();
private final JmxScraperConfig config;

JmxScraper(JmxScraperConfig config) {
/**
* Main method to create and run a {@link JmxScraper} instance.
*
* @param args - must be of the form "-config {jmx_config_path,'-'}"
*/
@SuppressWarnings({"SystemOut", "SystemExitOutsideMain"})
public static void main(String[] args) {
try {
JmxScraperConfigFactory factory = new JmxScraperConfigFactory();
JmxScraperConfig config = JmxScraper.createConfigFromArgs(Arrays.asList(args), factory);

JmxScraper jmxScraper = new JmxScraper(config);
jmxScraper.start();

Runtime.getRuntime()
.addShutdownHook(
new Thread() {
@Override
public void run() {
jmxScraper.shutdown();
}
});
} catch (ArgumentsParsingException e) {
System.err.println(
"Usage: java -jar <path_to_jmxscraper.jar> "
+ "-config <path_to_config.properties or - for stdin>");
System.exit(1);
} catch (ConfigurationException e) {
System.err.println(e.getMessage());
System.exit(1);
}
}

/**
* Create {@link JmxScraperConfig} object basing on command line options
*
* @param args application commandline arguments
*/
static JmxScraperConfig createConfigFromArgs(List<String> args, JmxScraperConfigFactory factory)
throws ArgumentsParsingException, ConfigurationException {
if (!args.isEmpty() && (args.size() != 2 || !args.get(0).equalsIgnoreCase("-config"))) {
throw new ArgumentsParsingException();
}

Properties loadedProperties = new Properties();
if (args.size() == 2) {
String path = args.get(1);
if (path.trim().equals("-")) {
loadPropertiesFromStdin(loadedProperties);
} else {
loadPropertiesFromPath(loadedProperties, path);
}
}

return factory.createConfig(loadedProperties);
}

private static void loadPropertiesFromStdin(Properties props) throws ConfigurationException {
try (InputStream is = new DataInputStream(System.in)) {
props.load(is);
} catch (IOException e) {
throw new ConfigurationException("Failed to read config properties from stdin", e);
}
}

private static void loadPropertiesFromPath(Properties props, String path)
throws ConfigurationException {
try (InputStream is = Files.newInputStream(Paths.get(path))) {
props.load(is);
} catch (IOException e) {
throw new ConfigurationException("Failed to read config properties file: '" + path + "'", e);
}
}

JmxScraper(JmxScraperConfig config) throws ConfigurationException {
this.config = config;

try {
Expand Down Expand Up @@ -51,28 +133,23 @@ private void start() {

private void shutdown() {
logger.info("Shutting down JmxScraper and exporting final metrics.");
// Prevent new tasks to be submitted
exec.shutdown();
}

/**
* Main method to create and run a {@link JmxScraper} instance.
*
* @param args - must be of the form "-config {jmx_config_path,'-'}"
*/
public static void main(String[] args) {
JmxScraperConfigFactory factory = new JmxScraperConfigFactory();
JmxScraperConfig config = factory.createConfigFromArgs(Arrays.asList(args));

JmxScraper jmxScraper = new JmxScraper(config);
jmxScraper.start();

Runtime.getRuntime()
.addShutdownHook(
new Thread() {
@Override
public void run() {
jmxScraper.shutdown();
}
});
try {
// Wait a while for existing tasks to terminate
if (!exec.awaitTermination(EXECUTOR_TERMINATION_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
// Cancel currently executing tasks
exec.shutdownNow();
// Wait a while for tasks to respond to being cancelled
if (!exec.awaitTermination(EXECUTOR_TERMINATION_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
logger.warning("Thread pool did not terminate in time: " + exec);
}
}
} catch (InterruptedException e) {
// (Re-)Cancel if current thread also interrupted
exec.shutdownNow();
// Preserve interrupt status
Thread.currentThread().interrupt();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

package io.opentelemetry.contrib.jmxscraper.config;

public class ConfigurationException extends RuntimeException {
public class ConfigurationException extends Exception {
private static final long serialVersionUID = 0L;

public ConfigurationException(String message, Throwable cause) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,12 @@

import static io.opentelemetry.contrib.jmxscraper.util.StringUtils.isBlank;

import java.io.DataInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.List;
import java.util.Locale;
import java.util.Properties;
import java.util.stream.Collectors;

@SuppressWarnings({"SystemOut", "SystemExitOutsideMain"})
public class JmxScraperConfigFactory {
private static final String PREFIX = "otel.";
static final String SERVICE_URL = PREFIX + "jmx.service.url";
Expand Down Expand Up @@ -64,56 +58,7 @@ public class JmxScraperConfigFactory {

private Properties properties = new Properties();

/**
* Create {@link JmxScraperConfig} object basing on command line options
*
* @param args application commandline arguments
*/
public JmxScraperConfig createConfigFromArgs(List<String> args) {
if (!args.isEmpty() && (args.size() != 2 || !args.get(0).equalsIgnoreCase("-config"))) {
System.out.println(
"Usage: java io.opentelemetry.contrib.jmxscraper.JmxScraper "
+ "-config <path_to_config.properties or - for stdin>");
System.exit(1);
}

Properties loadedProperties = new Properties();
if (args.size() == 2) {
String path = args.get(1);
if (path.trim().equals("-")) {
loadPropertiesFromStdin(loadedProperties);
} else {
loadPropertiesFromPath(loadedProperties, path);
}
}

JmxScraperConfig config = createConfig(loadedProperties);
validateConfig(config);
populateJmxSystemProperties();

return config;
}

private static void loadPropertiesFromStdin(Properties props) {
try (InputStream is = new DataInputStream(System.in)) {
props.load(is);
} catch (IOException e) {
System.out.println("Failed to read config properties from stdin: " + e.getMessage());
System.exit(1);
}
}

private static void loadPropertiesFromPath(Properties props, String path) {
try (InputStream is = Files.newInputStream(Paths.get(path))) {
props.load(is);
} catch (IOException e) {
System.out.println(
"Failed to read config properties file at '" + path + "': " + e.getMessage());
System.exit(1);
}
}

JmxScraperConfig createConfig(Properties props) {
public JmxScraperConfig createConfig(Properties props) throws ConfigurationException {
properties = new Properties();
// putAll() instead of using constructor defaults
// to ensure they will be recorded to underlying map
Expand Down Expand Up @@ -148,6 +93,9 @@ JmxScraperConfig createConfig(Properties props) {

config.registrySsl = Boolean.parseBoolean(properties.getProperty(REGISTRY_SSL));

validateConfig(config);
populateJmxSystemProperties();

return config;
}

Expand All @@ -168,7 +116,7 @@ private void populateJmxSystemProperties() {
});
}

private int getProperty(String key, int defaultValue) {
private int getProperty(String key, int defaultValue) throws ConfigurationException {
String propVal = properties.getProperty(key);
if (propVal == null) {
return defaultValue;
Expand All @@ -191,7 +139,8 @@ private String getAndSetPropertyIfUndefined(String key, String defaultValue) {
return propVal;
}

private int getAndSetPropertyIfUndefined(String key, int defaultValue) {
private int getAndSetPropertyIfUndefined(String key, int defaultValue)
throws ConfigurationException {
int propVal = getProperty(key, defaultValue);
if (propVal == defaultValue) {
properties.setProperty(key, String.valueOf(defaultValue));
Expand All @@ -200,7 +149,7 @@ private int getAndSetPropertyIfUndefined(String key, int defaultValue) {
}

/** Will determine if parsed config is complete, setting any applicable values and defaults. */
void validateConfig(JmxScraperConfig config) {
private static void validateConfig(JmxScraperConfig config) throws ConfigurationException {
if (isBlank(config.serviceUrl)) {
throw new ConfigurationException(SERVICE_URL + " must be specified.");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ public class JmxClient {

public JmxClient(JmxScraperConfig config) throws MalformedURLException {
this.url = new JMXServiceURL(config.getServiceUrl());
;
this.username = config.getUsername();
this.password = config.getPassword();
this.realm = config.getRealm();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.jmxscraper;

import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.mock;

import io.opentelemetry.contrib.jmxscraper.config.JmxScraperConfigFactory;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import org.junit.jupiter.api.Test;

class JmxScraperTest {
@Test
void shouldThrowExceptionWhenInvalidCommandLineArgsProvided() {
// Given
List<String> emptyArgs = Collections.singletonList("-inexistingOption");
JmxScraperConfigFactory configFactoryMock = mock(JmxScraperConfigFactory.class);

// When and Then
assertThatThrownBy(() -> JmxScraper.createConfigFromArgs(emptyArgs, configFactoryMock))
.isInstanceOf(ArgumentsParsingException.class);
}

@Test
void shouldThrowExceptionWhenTooManyCommandLineArgsProvided() {
// Given
List<String> emptyArgs = Arrays.asList("-config", "path", "-inexistingOption");
JmxScraperConfigFactory configFactoryMock = mock(JmxScraperConfigFactory.class);

// When and Then
assertThatThrownBy(() -> JmxScraper.createConfigFromArgs(emptyArgs, configFactoryMock))
.isInstanceOf(ArgumentsParsingException.class);
}
}
Loading

0 comments on commit f847561

Please sign in to comment.