Allow metrics configs to be loaded via resources #205

tylerbenson · 2019-01-02T18:27:20Z

Metrics configs embedded inside the agent aren’t directly accessible on the file system. Loading as a resource avoids having to copy to an external location.

These files should match the standard metrics.yaml format, and include jmx_metrics blocks at the top level.

gzussa · 2019-01-03T14:50:32Z

src/main/java/org/datadog/jmxfetch/App.java

@@ -220,7 +219,7 @@ public boolean processAutoDiscovery(byte[] buffer) {
      String[] discovered;

      try {
-        String configs = new String(buffer, CharEncoding.UTF_8);
+        String configs = new String(buffer, "UTF-8");


Good catch to remove this dependency since it was only used for this.
What about creating constant instead of having duplicated strings?

Done. I changed it to use a proper Charset instead.

gzussa · 2019-01-03T14:52:03Z

src/main/java/org/datadog/jmxfetch/Instance.java

@@ -38,6 +39,13 @@
    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>() {


Why does it need to be a ThreadLocal object?

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.

ok I trust you judgement here

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

gzussa · 2019-01-03T14:54:23Z

src/main/java/org/datadog/jmxfetch/Instance.java

@@ -199,6 +208,40 @@ private void loadMetricConfigFiles(AppConfig appConfig, LinkedList<Configuration
        }
    }

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


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

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?

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.

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

Avoids the need to catch the exception for an unknown charset.

Metrics configs embedded inside the agent aren’t directly accessible on the file system. Loading as a resource avoids having to copy to an external location.

gzussa · 2019-01-03T19:51:45Z

src/main/java/org/datadog/jmxfetch/App.java

@@ -51,6 +50,7 @@
    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");


Nice refactoring! 👍

gzussa · 2019-01-03T19:52:52Z

src/main/java/org/datadog/jmxfetch/Instance.java

@@ -38,6 +39,13 @@
    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>() {


ok I trust you judgement here

gzussa · 2019-01-03T19:57:55Z

src/main/java/org/datadog/jmxfetch/Instance.java

@@ -199,6 +208,40 @@ private void loadMetricConfigFiles(AppConfig appConfig, LinkedList<Configuration
        }
    }

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


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.

gzussa

Thanks for adding tests. This is solid work :) 💯

tylerbenson · 2019-01-07T19:54:32Z

@gzussa would you like to merge this or should I request access to merge it myself?

Using copy/paste from integrations-core. Will depend on a new jmxfetch release with DataDog/jmxfetch#205 before this will work.

truthbk

Looks good to me

truthbk · 2019-01-11T22:22:29Z

src/main/java/org/datadog/jmxfetch/App.java

@@ -219,38 +219,29 @@ public boolean processAutoDiscovery(byte[] buffer) {
      boolean reinit = false;
      String[] discovered;

-      try {


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

truthbk · 2019-01-11T22:31:20Z

src/main/java/org/datadog/jmxfetch/Instance.java

@@ -38,6 +39,13 @@
    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>() {


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

tylerbenson mentioned this pull request Jan 2, 2019

Embedding existing jmx integrations with git submodule DataDog/dd-trace-java#639

Merged

gzussa suggested changes Jan 3, 2019

View reviewed changes

tylerbenson added 2 commits January 3, 2019 11:40

Remove unneeded dependency and use Charset instead of String

bcb08ba

Avoids the need to catch the exception for an unknown charset.

Allow metrics configs to be loaded via resources

2125937

Metrics configs embedded inside the agent aren’t directly accessible on the file system. Loading as a resource avoids having to copy to an external location.

tylerbenson force-pushed the tyler/metrics-from-resources branch from 314bb0a to 2125937 Compare January 3, 2019 19:41

gzussa reviewed Jan 3, 2019

View reviewed changes

Add some tests for loading metric configs.

0f43dc7

gzussa approved these changes Jan 4, 2019

View reviewed changes

tylerbenson added a commit to DataDog/dd-trace-java that referenced this pull request Jan 8, 2019

First pass at embedding existing jmx integrations

de166e5

Using copy/paste from integrations-core. Will depend on a new jmxfetch release with DataDog/jmxfetch#205 before this will work.

truthbk approved these changes Jan 16, 2019

View reviewed changes

tylerbenson merged commit 59fbc29 into DataDog:master Jan 16, 2019

tylerbenson deleted the tyler/metrics-from-resources branch January 16, 2019 21:20

olivielpeau added this to the 0.25.0 milestone Jan 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow metrics configs to be loaded via resources #205

Allow metrics configs to be loaded via resources #205

tylerbenson commented Jan 2, 2019

gzussa Jan 3, 2019

tylerbenson Jan 3, 2019 •

edited

Loading

gzussa Jan 3, 2019

tylerbenson Jan 3, 2019

gzussa Jan 3, 2019

truthbk Jan 11, 2019

gzussa Jan 3, 2019

tylerbenson Jan 3, 2019

gzussa Jan 3, 2019

tylerbenson Jan 3, 2019

gzussa Jan 3, 2019

gzussa Jan 3, 2019

gzussa Jan 3, 2019

gzussa left a comment

tylerbenson commented Jan 7, 2019

truthbk left a comment

truthbk Jan 11, 2019

truthbk Jan 11, 2019

Allow metrics configs to be loaded via resources #205

Allow metrics configs to be loaded via resources #205

Conversation

tylerbenson commented Jan 2, 2019

Choose a reason for hiding this comment

tylerbenson Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzussa left a comment

Choose a reason for hiding this comment

tylerbenson commented Jan 7, 2019

truthbk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerbenson Jan 3, 2019 •

edited

Loading