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

Allow metrics configs to be loaded via resources #205

Merged
merged 3 commits into from
Jan 16, 2019
Merged
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
7 changes: 1 addition & 6 deletions pom.xml
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
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
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
52 changes: 49 additions & 3 deletions src/main/java/org/datadog/jmxfetch/Instance.java
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package org.datadog.jmxfetch;

import com.google.common.annotations.VisibleForTesting;
import java.io.File;
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 +40,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 +158,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,21 +174,22 @@ 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));
}
}

private void loadMetricConfigFiles(AppConfig appConfig, LinkedList<Configuration> configurationList) {
@VisibleForTesting
static void loadMetricConfigFiles(AppConfig appConfig, LinkedList<Configuration> configurationList) {
if (appConfig.getMetricConfigFiles() != null) {
for (String fileName : appConfig.getMetricConfigFiles()) {
String yamlPath = new File(fileName).getAbsolutePath();
FileInputStream yamlInputStream = null;
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 +210,41 @@ private void loadMetricConfigFiles(AppConfig appConfig, LinkedList<Configuration
}
}

@VisibleForTesting
static void loadMetricConfigResources(AppConfig config, LinkedList<Configuration> configurationList) {
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
27 changes: 27 additions & 0 deletions src/test/java/org/datadog/jmxfetch/TestInstance.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import com.google.common.collect.Lists;
import java.net.URL;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.Arrays;
Expand Down Expand Up @@ -72,4 +76,27 @@ public void testEmptyDefaultHostname() throws Exception {
this.assertHostnameTags(Arrays.asList(tags));
}
}

@Test
public void testLoadMetricConfigFiles() throws Exception {
URL defaultConfig = Instance.class.getResource("default-jmx-metrics.yaml");
AppConfig config = mock(AppConfig.class);
when(config.getMetricConfigFiles()).thenReturn(Lists.newArrayList(defaultConfig.getPath()));
LinkedList<Configuration> configurationList = new LinkedList<Configuration>();
Instance.loadMetricConfigFiles(config, configurationList);

assertEquals(2, configurationList.size());
}

@Test
public void testLoadMetricConfigResources() throws Exception {
URL defaultConfig = Instance.class.getResource("sample-metrics.yaml");
String configResource = defaultConfig.getPath().split("test-classes/")[1];
AppConfig config = mock(AppConfig.class);
when(config.getMetricConfigResources()).thenReturn(Lists.newArrayList(configResource));
LinkedList<Configuration> configurationList = new LinkedList<Configuration>();
Instance.loadMetricConfigResources(config, configurationList);

assertEquals(2, configurationList.size());
}
}
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
43 changes: 43 additions & 0 deletions src/test/resources/org/datadog/jmxfetch/sample-metrics.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Same as default-jmx-metrics.yaml but with the top level jmx_metrics to match standard metrics.yaml file structure.

jmx_metrics:

# Memory
- include:
domain: java.lang
type: Memory
attribute:
HeapMemoryUsage.used:
alias: jvm.heap_memory
metric_type: gauge
HeapMemoryUsage.committed:
alias: jvm.heap_memory_committed
metric_type: gauge
HeapMemoryUsage.init:
alias: jvm.heap_memory_init
metric_type: gauge
HeapMemoryUsage.max:
alias: jvm.heap_memory_max
metric_type: gauge
NonHeapMemoryUsage.used:
alias: jvm.non_heap_memory
metric_type: gauge
NonHeapMemoryUsage.committed:
alias: jvm.non_heap_memory_committed
metric_type: gauge
NonHeapMemoryUsage.init:
alias: jvm.non_heap_memory_init
metric_type: gauge
NonHeapMemoryUsage.max:
alias: jvm.non_heap_memory_max
metric_type: gauge

# Threads
- include:
domain: java.lang
type: Threading
attribute:
ThreadCount:
alias: jvm.thread_count
metric_type: gauge