Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable SSL, make reporter a daemon thread #118

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ there are two ways to specify the host in the jolokia URL so this URL will be re
**Optional environment variables**

- GRAPHITE_PORT: Protocol port of graphite. Defaults to 2004.
- GRAPHITE_SSL: Boolean to enable SSL sockets. Standard `javax.net.ssl` system properties are used for keystores, etc.
- SERVICE_HOST: By default the host is taken from Jolokia URL and serves as the service host, unless you use this variable.
- INTERVAL_IN_SEC: By default 30 seconds unless you use this variable.
- LOG_LEVEL: Configure Log Level [any of OFF, FATAL, ERROR, WARN, INFO, DEBUG, TRACE, ALL]
- WHITE_LIST_REGEX: filter out unwanted metrics with whitelist regex.
- BLACK_LIST_REGEX: filter out unwanted metrics with blacklist regex.


### Docker with config file
Create a .conf file, set the input parameter and provide it to the docker.

Expand Down
12 changes: 7 additions & 5 deletions src/main/java/io/logz/jmx2graphite/GraphiteClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.slf4j.LoggerFactory;

import javax.net.SocketFactory;
import javax.net.ssl.SSLSocketFactory;
import java.io.Closeable;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
Expand All @@ -37,7 +38,7 @@ public class GraphiteClient implements Closeable {
private int failuresAtLastWrite = 0;

public GraphiteClient(String serviceHost, String serviceName, String graphiteHostname, int graphitePort,
int connectTimeout, int socketTimeout, int writeTimeoutMs, GraphiteProtocol protocol) {
boolean ssl, int connectTimeout, int socketTimeout, int writeTimeoutMs, GraphiteProtocol protocol) {
List<String> prefixElements = Lists.newArrayList();
if (serviceName != null && !serviceName.isEmpty()) {
prefixElements.add(sanitizeMetricName(serviceName));
Expand All @@ -54,7 +55,7 @@ public GraphiteClient(String serviceHost, String serviceName, String graphiteHos
logger.info("Graphite metrics prefix: {}", metricsPrefix);
logger.info("Graphite Client: using writeTimeoutMs of {} [ms]. Establishing connection...", writeTimeoutMs);

SocketFactory socketFactory = new SocketFactoryWithTimeouts(connectTimeout, socketTimeout);
SocketFactory socketFactory = new SocketFactoryWithTimeouts(connectTimeout, socketTimeout, ssl);
if (protocol == UDP) {
graphite = new GraphiteUDP(new InetSocketAddress(graphiteHostname, graphitePort));
} else if (protocol == TCP) {
Expand Down Expand Up @@ -157,19 +158,20 @@ public GraphiteWriteFailed(String message, Throwable cause) {
}

private static class SocketFactoryWithTimeouts extends SocketFactory {
private SocketFactory socketFactory = SocketFactory.getDefault();
private final SocketFactory socketFactory;

// FIXME make it use this connect timeout (I can't find a way to do it
private int connectTimeout;
private int socketTimeout;

public SocketFactoryWithTimeouts(int connectTimeout, int socketTimeout) {
public SocketFactoryWithTimeouts(int connectTimeout, int socketTimeout, boolean ssl) {
this.connectTimeout = connectTimeout;
this.socketTimeout = socketTimeout;
this.socketFactory = ssl ? SSLSocketFactory.getDefault() : SocketFactory.getDefault();
}

public static SocketFactory getDefault() {
return new SocketFactoryWithTimeouts(5, 10);
return new SocketFactoryWithTimeouts(5, 10, false);
}

@Override
Expand Down
10 changes: 10 additions & 0 deletions src/main/java/io/logz/jmx2graphite/Jmx2GraphiteConfiguration.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public enum MetricClientType {
private class Graphite {
public String hostname;
public int port;
public boolean ssl;
}

public Jmx2GraphiteConfiguration(Config config) throws IllegalConfiguration {
Expand Down Expand Up @@ -104,6 +105,7 @@ else if (config.hasPath("service.poller.mbean-direct")) {
graphite = new Graphite();
graphite.hostname = config.getString("graphite.hostname");
graphite.port = config.getInt("graphite.port");
graphite.ssl = config.getBoolean("graphite.ssl");
metricsPollingIntervalInSeconds = config.getInt("metricsPollingIntervalInSeconds");

serviceName = config.getString("service.name");
Expand Down Expand Up @@ -176,6 +178,14 @@ public void setGraphitePort(int graphitePort) {
this.graphite.port = graphitePort;
}

public boolean getGraphiteSsl() {
return graphite.ssl;
}

public void setGraphiteSsl(boolean ssl) {
this.graphite.ssl = ssl;
}

public String getServiceName() {
return serviceName;
}
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/io/logz/jmx2graphite/Jmx2GraphiteJavaAgent.java
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ private static String getArgumentConfigurationRepresentation(String key) throws
return "graphite.hostname";
case "GRAPHITE_PORT":
return "graphite.port";
case "GRAPHITE_SSL":
return "graphite.ssl";
case "SERVICE_NAME":
return "service.name";
case "SERVICE_HOST":
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/io/logz/jmx2graphite/MetricsPipeline.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class MetricsPipeline {
public MetricsPipeline(Jmx2GraphiteConfiguration conf, MBeanClient client) {

this.graphiteClient = new GraphiteClient(conf.getServiceHost(), conf.getServiceName(), conf.getGraphiteHostname(),
conf.getGraphitePort(), conf.getGraphiteConnectTimeout(),
conf.getGraphitePort(), conf.getGraphiteSsl(), conf.getGraphiteConnectTimeout(),
conf.getGraphiteSocketTimeout(), conf.getGraphiteWriteTimeoutMs(),
conf.getGraphiteProtocol());
this.client = client;
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/javaagent.conf
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ graphite {
hostname = ${?GRAPHITE_HOST}
port = ${?GRAPHITE_PORT}
protocol = ${?GRAPHITE_PROTOCOL}
ssl = ${?GRAPHITE_SSL}
}

filter {
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/reference.conf
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ graphite {
port = 2004
connectTimeout = 10
socketTimeout = 5
ssl = false
}
2 changes: 1 addition & 1 deletion src/test/java/io/logz/jmx2graphite/GraphiteClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public void testOnServerRestart() throws InterruptedException {
int connectTimeout = 1000;
int socketTimeout = 1000;
GraphiteClient client = new GraphiteClient("bla-host.com", "bla-service", "localhost",
port, connectTimeout, socketTimeout, 20000,
port, false, connectTimeout, socketTimeout, 20000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add an integration test to see that the functionality works?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, setting up an ssl-enabled graphite is fairly complicated, and doing it in an integration test more so, and I don't have the time right now to get it working. I understand if you don't want to merge without a test though. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elahrvivaz Even if you use testcontainers? If you already did once for manual testing, it's only a matter of writing the configuration file for Graphite no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, well I didn't set up the instance I've been testing on, and I'm not familiar with testcontainers :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elahrvivaz Testcontainers is fairly easy to work with. You have a great example at GraphiteITest. Take a few minutes to review it - I'll happy to assist with any questions you may have.

null);

ArrayList<MetricValue> dummyMetrics = Lists.newArrayList(new MetricValue("dice", 4, TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis())));
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/io/logz/jmx2graphite/GraphiteITest.java
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ private GraphiteClient createGraphiteClient() {
int socketTimeout = (int) Duration.ofSeconds(5).toMillis();
int writeTimeoutMs = (int) Duration.ofSeconds(5).toMillis();

return new GraphiteClient(TEST_SERVICE_HOST, TEST_SERVICE_NAME, "localhost", 2003,
return new GraphiteClient(TEST_SERVICE_HOST, TEST_SERVICE_NAME, "localhost", 2003, false,
connectTimeout, socketTimeout, writeTimeoutMs, GraphiteProtocol.TCP);
}

Expand Down