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.

Sign up for GitHub

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

Allow metrics configs to be loaded via resources #205

Merged
merged 3 commits into from
Jan 16, 2019
Merged

Allow metrics configs to be loaded via resources #205

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bcb08ba
Remove unneeded dependency and use Charset instead of String
tylerbenson Dec 28, 2018
2125937
Allow metrics configs to be loaded via resources
tylerbenson Dec 22, 2018
0f43dc7
Add some tests for loading metric configs.
tylerbenson Jan 3, 2019
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
7 changes: 1 addition & 6 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
</developers>

<properties>
<maven.compiler.source>1.6</maven.compiler.source>
<maven.compiler.target>1.6</maven.compiler.target>

<commons-io.version>2.4</commons-io.version>
Expand Down Expand Up @@ -92,12 +93,6 @@
<artifactId>commons-lang</artifactId>
<version>${commons-lang.version}</version>
</dependency>
<dependency>
<!-- https://mvnrepository.com/artifact/org.apache.commons/commons-lang3 -->
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>${apache-commons-lang3.version}</version>
</dependency>
<dependency>
<groupId>com.datadoghq</groupId>
<artifactId>java-dogstatsd-client</artifactId>
Expand Down
45 changes: 18 additions & 27 deletions src/main/java/org/datadog/jmxfetch/App.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import java.io.FileNotFoundException;
import java.io.InputStream;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.nio.charset.Charset;
import java.util.Arrays;
import java.util.ArrayList;
import java.util.Enumeration;
Expand All @@ -27,7 +27,6 @@
import org.apache.log4j.Appender;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;
import org.apache.commons.lang3.CharEncoding;
import org.apache.commons.io.IOUtils;
import org.datadog.jmxfetch.reporter.Reporter;
import org.datadog.jmxfetch.util.CustomLogger;
Expand All @@ -51,6 +50,7 @@ public class App {
private static final String AD_LEGACY_CONFIG_TERM = "#### SERVICE-DISCOVERY TERM ####";
private static final int AD_MAX_NAME_LEN = 80;
private static final int AD_MAX_MAG_INSTANCES = 4; // 1000 instances ought to be enough for anyone
private static final Charset UTF_8 = Charset.forName("UTF-8");
Copy link

Choose a reason for hiding this comment

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

Nice refactoring! 👍


private static int loopCounter;
private int lastJSONConfigTS;
Expand Down Expand Up @@ -219,38 +219,29 @@ public boolean processAutoDiscovery(byte[] buffer) {
boolean reinit = false;
String[] discovered;

try {
Copy link
Member

Choose a reason for hiding this comment

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

Indeed it looks like we were try...catching an Exception that didn't even come into play here.

String configs = new String(buffer, CharEncoding.UTF_8);
String separator = App.AD_CONFIG_SEP;
String configs = new String(buffer, UTF_8);
String separator = App.AD_CONFIG_SEP;

if (configs.indexOf(App.AD_LEGACY_CONFIG_SEP) != -1) {
separator = App.AD_LEGACY_CONFIG_SEP;
}
discovered = configs.split(separator + System.getProperty("line.separator"));
} catch(UnsupportedEncodingException e) {
LOGGER.debug("Unable to parse byte buffer to UTF-8 String.");
return false;
if (configs.indexOf(App.AD_LEGACY_CONFIG_SEP) != -1) {
separator = App.AD_LEGACY_CONFIG_SEP;
}
discovered = configs.split(separator + System.getProperty("line.separator"));

for (String config : discovered) {
if (config == null || config.isEmpty()) {
continue;
}

try{
String name = getAutoDiscoveryName(config);
LOGGER.debug("Attempting to apply config. Name: " + name + "\nconfig: \n" + config);
InputStream stream = new ByteArrayInputStream(config.getBytes(CharEncoding.UTF_8));
YamlParser yaml = new YamlParser(stream);

if (this.addConfig(name, yaml)){
reinit = true;
LOGGER.debug("Configuration added succesfully reinit in order");
} else {
LOGGER.debug("Unable to apply configuration.");
}
} catch(UnsupportedEncodingException e) {
LOGGER.debug("Unable to parse byte buffer to UTF-8 String.");
String name = getAutoDiscoveryName(config);
LOGGER.debug("Attempting to apply config. Name: " + name + "\nconfig: \n" + config);
InputStream stream = new ByteArrayInputStream(config.getBytes(UTF_8));
YamlParser yaml = new YamlParser(stream);

if (this.addConfig(name, yaml)){
reinit = true;
LOGGER.debug("Configuration added succesfully reinit in order");
} else {
LOGGER.debug("Unable to apply configuration.");
}
}

Expand Down Expand Up @@ -587,7 +578,7 @@ private boolean getJSONConfigs() {

LOGGER.debug("Received the following JSON configs: " + response.getResponseBody());

InputStream jsonInputStream = IOUtils.toInputStream(response.getResponseBody(), "UTF-8");
InputStream jsonInputStream = IOUtils.toInputStream(response.getResponseBody(), UTF_8);
JsonParser parser = new JsonParser(jsonInputStream);
int timestamp = ((Integer) parser.getJsonTimestamp()).intValue();
if (timestamp > lastJSONConfigTS) {
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/org/datadog/jmxfetch/AppConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ public class AppConfig {

// This is used by things like APM agent to provide configuration from resources
private List<String> instanceConfigResources;
// This is used by things like APM agent to provide metric configuration from resources
private List<String> metricConfigResources;
// This is used by things like APM agent to provide metric configuration from files
private List<String> metricConfigFiles;
// This is used by things like APM agent to provide global override for bean refresh period
Expand Down Expand Up @@ -229,6 +231,10 @@ public List<String> getInstanceConfigResources() {
return instanceConfigResources;
}

public List<String> getMetricConfigResources() {
return metricConfigResources;
}

public List<String> getMetricConfigFiles() {
return metricConfigFiles;
}
Expand All @@ -246,6 +252,7 @@ public Map<String, String> getGlobalTags() {
*/
public static AppConfig create(
List<String> instanceConfigResources,
List<String> metricConfigResources,
List<String> metricConfigFiles,
Integer checkPeriod,
Integer refreshBeansPeriod,
Expand All @@ -256,6 +263,7 @@ public static AppConfig create(
AppConfig config = new AppConfig();
config.action = ImmutableList.of(ACTION_COLLECT);
config.instanceConfigResources = ImmutableList.copyOf(instanceConfigResources);
config.metricConfigResources = ImmutableList.copyOf(metricConfigResources);
config.metricConfigFiles = ImmutableList.copyOf(metricConfigFiles);
if (checkPeriod != null) {
config.checkPeriod = checkPeriod;
Expand Down
47 changes: 45 additions & 2 deletions src/main/java/org/datadog/jmxfetch/Instance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.lang.ClassCastException;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -38,6 +39,13 @@ public class Instance {
public static final String PROCESS_NAME_REGEX = "process_name_regex";
public static final String ATTRIBUTE = "Attribute: ";

private static final ThreadLocal<Yaml> YAML = new ThreadLocal<Yaml>() {
Copy link

Choose a reason for hiding this comment

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

Why does it need to be a ThreadLocal object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There seemed to be non-trivial amount of initialization happening in the constructor, so I wanted to avoid it. The problem is (I believe) Yaml is not thread safe, so I used the ThreadLocal to ensure that safety.

Copy link

Choose a reason for hiding this comment

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

ok I trust you judgement here

Copy link
Member

Choose a reason for hiding this comment

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

This is accurate. We weren't thread-unsafe due to how we deal with instances, but this is safer. Thank you for this cleanup.

@Override
public Yaml initialValue() {
return new Yaml();
}
};

private Set<ObjectName> beans;
private LinkedList<String> beanScopes;
private LinkedList<Configuration> configurationList = new LinkedList<Configuration>();
Expand Down Expand Up @@ -149,6 +157,7 @@ public Instance(LinkedHashMap<String, Object> instanceMap, LinkedHashMap<String,
}

loadMetricConfigFiles(appConfig, configurationList);
loadMetricConfigResources(appConfig, configurationList);

String gcMetricConfig = "old-gc-default-jmx-metrics.yaml";

Expand All @@ -164,7 +173,7 @@ public Instance(LinkedHashMap<String, Object> instanceMap, LinkedHashMap<String,
}

private void loadDefaultConfig(String configResourcePath) {
ArrayList<LinkedHashMap<String, Object>> defaultConf = (ArrayList<LinkedHashMap<String, Object>>) new Yaml().load(this.getClass().getResourceAsStream(configResourcePath));
ArrayList<LinkedHashMap<String, Object>> defaultConf = (ArrayList<LinkedHashMap<String, Object>>) YAML.get().load(this.getClass().getResourceAsStream(configResourcePath));
for (LinkedHashMap<String, Object> conf : defaultConf) {
configurationList.add(new Configuration(conf));
}
Expand All @@ -178,7 +187,7 @@ private void loadMetricConfigFiles(AppConfig appConfig, LinkedList<Configuration
LOGGER.info("Reading metric config file " + yamlPath);
try {
yamlInputStream = new FileInputStream(yamlPath);
ArrayList<LinkedHashMap<String, Object>> confs = (ArrayList<LinkedHashMap<String, Object>>) new Yaml().load(yamlInputStream);
ArrayList<LinkedHashMap<String, Object>> confs = (ArrayList<LinkedHashMap<String, Object>>) YAML.get().load(yamlInputStream);
for (LinkedHashMap<String, Object> conf : confs) {
configurationList.add(new Configuration(conf));
}
Expand All @@ -199,6 +208,40 @@ private void loadMetricConfigFiles(AppConfig appConfig, LinkedList<Configuration
}
}

private void loadMetricConfigResources(AppConfig config, LinkedList<Configuration> configurationList) {
Copy link

Choose a reason for hiding this comment

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

Is it legit to add a unit tests for this method, unless it is covered already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was doing manual testing since the application is not structured well for easy testing. Do you have any suggestions for better ways to validate this in a unit test?

Copy link

Choose a reason for hiding this comment

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

You most probably will have to create a method to get the inputStream so this method can be testable as such by creating an object instance and calling this method while providing your inputStream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've added a few simple tests. Is this what you were looking for?

List<String> resourceConfigList = config.getMetricConfigResources();
if (resourceConfigList != null) {
ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
for (String resourceName : resourceConfigList) {
LOGGER.info("Reading metric config resource " + resourceName);
InputStream inputStream = classLoader.getResourceAsStream(resourceName);
if (inputStream == null) {
LOGGER.warn("Cannot find metric config resource" + resourceName);
} else {
try {
LinkedHashMap<String, ArrayList<LinkedHashMap<String, Object>>> topYaml = (LinkedHashMap<String, ArrayList<LinkedHashMap<String, Object>>>) YAML.get().load(inputStream);
ArrayList<LinkedHashMap<String, Object>> jmxConf = topYaml.get("jmx_metrics");
if(jmxConf != null) {
for (LinkedHashMap<String, Object> conf : jmxConf) {
configurationList.add(new Configuration(conf));
}
} else {
LOGGER.warn("jmx_metrics block not found in resource " + resourceName);
}
} catch (Exception e) {
LOGGER.warn("Cannot parse yaml resource " + resourceName, e);
} finally {
try {
inputStream.close();
} catch (IOException e) {
// ignore
}
}
}
}
}
}

/**
* Format the instance tags defined in the YAML configuration file to a `LinkedHashMap`.
* Supported inputs: `List`, `Map`.
Expand Down
2 changes: 2 additions & 0 deletions src/test/java/org/datadog/jmxfetch/TestLogInitialization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public AppConfig call() throws Exception {
return AppConfig.create(
ImmutableList.of("org/datadog/jmxfetch/dd-java-agent-jmx.yaml"),
Collections.<String>emptyList(),
Collections.<String>emptyList(),
(int) TimeUnit.SECONDS.toMillis(30),
(int) TimeUnit.SECONDS.toMillis(30),
Collections.<String, String>emptyMap(),
Expand All @@ -51,6 +52,7 @@ public AppConfig call() throws Exception {
return AppConfig.create(
ImmutableList.of("org/datadog/jmxfetch/remote-jmx.yaml"),
Collections.<String>emptyList(),
Collections.<String>emptyList(),
(int) TimeUnit.SECONDS.toMillis(30),
(int) TimeUnit.SECONDS.toMillis(30),
Collections.<String, String>emptyMap(),
Expand Down