Skip to content

Commit f847561

Browse files
robsundayrobsunday
committed
Code review changes:
Argument parsing moved to the main class. Argument validation now throw exception. More tests added for config factory. Created unit tests for JmxScraper class.
1 parent 164679f commit f847561

File tree

7 files changed

+217
-104
lines changed

7 files changed

+217
-104
lines changed

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.contrib.jmxscraper;
7+
8+
public class ArgumentsParsingException extends Exception {
9+
private static final long serialVersionUID = 0L;
10+
}

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

Lines changed: 100 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,101 @@
99
import io.opentelemetry.contrib.jmxscraper.config.JmxScraperConfig;
1010
import io.opentelemetry.contrib.jmxscraper.config.JmxScraperConfigFactory;
1111
import io.opentelemetry.contrib.jmxscraper.jmx.JmxClient;
12+
import java.io.DataInputStream;
13+
import java.io.IOException;
14+
import java.io.InputStream;
1215
import java.net.MalformedURLException;
16+
import java.nio.file.Files;
17+
import java.nio.file.Paths;
1318
import java.util.Arrays;
19+
import java.util.List;
20+
import java.util.Properties;
1421
import java.util.concurrent.Executors;
1522
import java.util.concurrent.ScheduledExecutorService;
1623
import java.util.concurrent.TimeUnit;
1724
import java.util.logging.Logger;
1825

1926
public class JmxScraper {
2027
private static final Logger logger = Logger.getLogger(JmxScraper.class.getName());
28+
private static final int EXECUTOR_TERMINATION_TIMEOUT_MS = 5000;
2129
private final ScheduledExecutorService exec = Executors.newSingleThreadScheduledExecutor();
2230
private final JmxScraperConfig config;
2331

24-
JmxScraper(JmxScraperConfig config) {
32+
/**
33+
* Main method to create and run a {@link JmxScraper} instance.
34+
*
35+
* @param args - must be of the form "-config {jmx_config_path,'-'}"
36+
*/
37+
@SuppressWarnings({"SystemOut", "SystemExitOutsideMain"})
38+
public static void main(String[] args) {
39+
try {
40+
JmxScraperConfigFactory factory = new JmxScraperConfigFactory();
41+
JmxScraperConfig config = JmxScraper.createConfigFromArgs(Arrays.asList(args), factory);
42+
43+
JmxScraper jmxScraper = new JmxScraper(config);
44+
jmxScraper.start();
45+
46+
Runtime.getRuntime()
47+
.addShutdownHook(
48+
new Thread() {
49+
@Override
50+
public void run() {
51+
jmxScraper.shutdown();
52+
}
53+
});
54+
} catch (ArgumentsParsingException e) {
55+
System.err.println(
56+
"Usage: java -jar <path_to_jmxscraper.jar> "
57+
+ "-config <path_to_config.properties or - for stdin>");
58+
System.exit(1);
59+
} catch (ConfigurationException e) {
60+
System.err.println(e.getMessage());
61+
System.exit(1);
62+
}
63+
}
64+
65+
/**
66+
* Create {@link JmxScraperConfig} object basing on command line options
67+
*
68+
* @param args application commandline arguments
69+
*/
70+
static JmxScraperConfig createConfigFromArgs(List<String> args, JmxScraperConfigFactory factory)
71+
throws ArgumentsParsingException, ConfigurationException {
72+
if (!args.isEmpty() && (args.size() != 2 || !args.get(0).equalsIgnoreCase("-config"))) {
73+
throw new ArgumentsParsingException();
74+
}
75+
76+
Properties loadedProperties = new Properties();
77+
if (args.size() == 2) {
78+
String path = args.get(1);
79+
if (path.trim().equals("-")) {
80+
loadPropertiesFromStdin(loadedProperties);
81+
} else {
82+
loadPropertiesFromPath(loadedProperties, path);
83+
}
84+
}
85+
86+
return factory.createConfig(loadedProperties);
87+
}
88+
89+
private static void loadPropertiesFromStdin(Properties props) throws ConfigurationException {
90+
try (InputStream is = new DataInputStream(System.in)) {
91+
props.load(is);
92+
} catch (IOException e) {
93+
throw new ConfigurationException("Failed to read config properties from stdin", e);
94+
}
95+
}
96+
97+
private static void loadPropertiesFromPath(Properties props, String path)
98+
throws ConfigurationException {
99+
try (InputStream is = Files.newInputStream(Paths.get(path))) {
100+
props.load(is);
101+
} catch (IOException e) {
102+
throw new ConfigurationException("Failed to read config properties file: '" + path + "'", e);
103+
}
104+
}
105+
106+
JmxScraper(JmxScraperConfig config) throws ConfigurationException {
25107
this.config = config;
26108

27109
try {
@@ -51,28 +133,23 @@ private void start() {
51133

52134
private void shutdown() {
53135
logger.info("Shutting down JmxScraper and exporting final metrics.");
136+
// Prevent new tasks to be submitted
54137
exec.shutdown();
55-
}
56-
57-
/**
58-
* Main method to create and run a {@link JmxScraper} instance.
59-
*
60-
* @param args - must be of the form "-config {jmx_config_path,'-'}"
61-
*/
62-
public static void main(String[] args) {
63-
JmxScraperConfigFactory factory = new JmxScraperConfigFactory();
64-
JmxScraperConfig config = factory.createConfigFromArgs(Arrays.asList(args));
65-
66-
JmxScraper jmxScraper = new JmxScraper(config);
67-
jmxScraper.start();
68-
69-
Runtime.getRuntime()
70-
.addShutdownHook(
71-
new Thread() {
72-
@Override
73-
public void run() {
74-
jmxScraper.shutdown();
75-
}
76-
});
138+
try {
139+
// Wait a while for existing tasks to terminate
140+
if (!exec.awaitTermination(EXECUTOR_TERMINATION_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
141+
// Cancel currently executing tasks
142+
exec.shutdownNow();
143+
// Wait a while for tasks to respond to being cancelled
144+
if (!exec.awaitTermination(EXECUTOR_TERMINATION_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
145+
logger.warning("Thread pool did not terminate in time: " + exec);
146+
}
147+
}
148+
} catch (InterruptedException e) {
149+
// (Re-)Cancel if current thread also interrupted
150+
exec.shutdownNow();
151+
// Preserve interrupt status
152+
Thread.currentThread().interrupt();
153+
}
77154
}
78155
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
package io.opentelemetry.contrib.jmxscraper.config;
77

8-
public class ConfigurationException extends RuntimeException {
8+
public class ConfigurationException extends Exception {
99
private static final long serialVersionUID = 0L;
1010

1111
public ConfigurationException(String message, Throwable cause) {

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

Lines changed: 8 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,12 @@
77

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

10-
import java.io.DataInputStream;
11-
import java.io.IOException;
12-
import java.io.InputStream;
13-
import java.nio.file.Files;
14-
import java.nio.file.Paths;
1510
import java.util.Arrays;
1611
import java.util.List;
1712
import java.util.Locale;
1813
import java.util.Properties;
1914
import java.util.stream.Collectors;
2015

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

6559
private Properties properties = new Properties();
6660

67-
/**
68-
* Create {@link JmxScraperConfig} object basing on command line options
69-
*
70-
* @param args application commandline arguments
71-
*/
72-
public JmxScraperConfig createConfigFromArgs(List<String> args) {
73-
if (!args.isEmpty() && (args.size() != 2 || !args.get(0).equalsIgnoreCase("-config"))) {
74-
System.out.println(
75-
"Usage: java io.opentelemetry.contrib.jmxscraper.JmxScraper "
76-
+ "-config <path_to_config.properties or - for stdin>");
77-
System.exit(1);
78-
}
79-
80-
Properties loadedProperties = new Properties();
81-
if (args.size() == 2) {
82-
String path = args.get(1);
83-
if (path.trim().equals("-")) {
84-
loadPropertiesFromStdin(loadedProperties);
85-
} else {
86-
loadPropertiesFromPath(loadedProperties, path);
87-
}
88-
}
89-
90-
JmxScraperConfig config = createConfig(loadedProperties);
91-
validateConfig(config);
92-
populateJmxSystemProperties();
93-
94-
return config;
95-
}
96-
97-
private static void loadPropertiesFromStdin(Properties props) {
98-
try (InputStream is = new DataInputStream(System.in)) {
99-
props.load(is);
100-
} catch (IOException e) {
101-
System.out.println("Failed to read config properties from stdin: " + e.getMessage());
102-
System.exit(1);
103-
}
104-
}
105-
106-
private static void loadPropertiesFromPath(Properties props, String path) {
107-
try (InputStream is = Files.newInputStream(Paths.get(path))) {
108-
props.load(is);
109-
} catch (IOException e) {
110-
System.out.println(
111-
"Failed to read config properties file at '" + path + "': " + e.getMessage());
112-
System.exit(1);
113-
}
114-
}
115-
116-
JmxScraperConfig createConfig(Properties props) {
61+
public JmxScraperConfig createConfig(Properties props) throws ConfigurationException {
11762
properties = new Properties();
11863
// putAll() instead of using constructor defaults
11964
// to ensure they will be recorded to underlying map
@@ -148,6 +93,9 @@ JmxScraperConfig createConfig(Properties props) {
14893

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

96+
validateConfig(config);
97+
populateJmxSystemProperties();
98+
15199
return config;
152100
}
153101

@@ -168,7 +116,7 @@ private void populateJmxSystemProperties() {
168116
});
169117
}
170118

171-
private int getProperty(String key, int defaultValue) {
119+
private int getProperty(String key, int defaultValue) throws ConfigurationException {
172120
String propVal = properties.getProperty(key);
173121
if (propVal == null) {
174122
return defaultValue;
@@ -191,7 +139,8 @@ private String getAndSetPropertyIfUndefined(String key, String defaultValue) {
191139
return propVal;
192140
}
193141

194-
private int getAndSetPropertyIfUndefined(String key, int defaultValue) {
142+
private int getAndSetPropertyIfUndefined(String key, int defaultValue)
143+
throws ConfigurationException {
195144
int propVal = getProperty(key, defaultValue);
196145
if (propVal == defaultValue) {
197146
properties.setProperty(key, String.valueOf(defaultValue));
@@ -200,7 +149,7 @@ private int getAndSetPropertyIfUndefined(String key, int defaultValue) {
200149
}
201150

202151
/** Will determine if parsed config is complete, setting any applicable values and defaults. */
203-
void validateConfig(JmxScraperConfig config) {
152+
private static void validateConfig(JmxScraperConfig config) throws ConfigurationException {
204153
if (isBlank(config.serviceUrl)) {
205154
throw new ConfigurationException(SERVICE_URL + " must be specified.");
206155
}

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ public class JmxClient {
3838

3939
public JmxClient(JmxScraperConfig config) throws MalformedURLException {
4040
this.url = new JMXServiceURL(config.getServiceUrl());
41-
;
4241
this.username = config.getUsername();
4342
this.password = config.getPassword();
4443
this.realm = config.getRealm();

‎jmx-scraper/src/test/java/io/opentelemetry/contrib/jmxscraper/JmxScraperTest.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
/*
2+
* Copyright The OpenTelemetry Authors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package io.opentelemetry.contrib.jmxscraper;
7+
8+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
9+
import static org.mockito.Mockito.mock;
10+
11+
import io.opentelemetry.contrib.jmxscraper.config.JmxScraperConfigFactory;
12+
import java.util.Arrays;
13+
import java.util.Collections;
14+
import java.util.List;
15+
import org.junit.jupiter.api.Test;
16+
17+
class JmxScraperTest {
18+
@Test
19+
void shouldThrowExceptionWhenInvalidCommandLineArgsProvided() {
20+
// Given
21+
List<String> emptyArgs = Collections.singletonList("-inexistingOption");
22+
JmxScraperConfigFactory configFactoryMock = mock(JmxScraperConfigFactory.class);
23+
24+
// When and Then
25+
assertThatThrownBy(() -> JmxScraper.createConfigFromArgs(emptyArgs, configFactoryMock))
26+
.isInstanceOf(ArgumentsParsingException.class);
27+
}
28+
29+
@Test
30+
void shouldThrowExceptionWhenTooManyCommandLineArgsProvided() {
31+
// Given
32+
List<String> emptyArgs = Arrays.asList("-config", "path", "-inexistingOption");
33+
JmxScraperConfigFactory configFactoryMock = mock(JmxScraperConfigFactory.class);
34+
35+
// When and Then
36+
assertThatThrownBy(() -> JmxScraper.createConfigFromArgs(emptyArgs, configFactoryMock))
37+
.isInstanceOf(ArgumentsParsingException.class);
38+
}
39+
}

0 commit comments

Comments
 (0)