Convert to library #180

mar-kolya · 2018-09-17T17:20:23Z

Allow JMX fetch to be used as a library.

At this point this is more of an RFC and not intended to be merged directly. Main changes include:

Make JMXFetch releasable on maven. This PR currently releases into temporary repo and this would need to change.
Refactor code so System.exit() is never called from main run function.
Add support for loading configuration from Java resources.
Add additional parameters and constructor method to config object to allow creating it from the user of this library.

truthbk

This mostly looks good to me, very non-invasive transition to a JAR usable by the APM tracer. Added a few small comments, but this looks real promising. Also, thanks for the cleanup on a few things! 🙇

truthbk · 2018-10-02T05:29:00Z

src/main/java/org/datadog/jmxfetch/util/CustomLogger.java

@@ -24,8 +24,10 @@ public static void setup(Level level, String logLocation) {
            Logger.getRootLogger().addAppender(fa);
            LOGGER.info("File Handler set");
        } else {
-            System.out.println("Log location is not set, will output log to stdout.");
-            ConsoleAppender consoleAppender = new ConsoleAppender(new PatternLayout(LOGGER_LAYOUT));
+            ConsoleAppender consoleAppender = new ConsoleAppender(new PatternLayout(LOGGER_LAYOUT), logLocation);


As per the log4j docs I believe logLocation can be either "System.out" or "System.err", not sure if we're being defensive enough here, also unsure what the behavior is otherwise.

Yeah, I should not have passed logLocation there, it is handled later. Is this what you meant the problem is?

If you revert this line, this looks good to me.

Yes, done. Sorry, I've reverted that a while back but apparently pushed into a wrong place. My bad, fixed now.

truthbk · 2018-10-02T05:39:15Z

src/main/java/org/datadog/jmxfetch/reporter/ConsoleReporter.java

    }

    @Override
    public void displayMatchingAttributeName(JMXAttribute jmxAttribute, int rank, int limit) {
-        System.out.println("       Matching: " + rank + "/" + limit + ". " + jmxAttribute);
+        LOGGER.debug("       Matching: " + rank + "/" + limit + ". " + jmxAttribute);


These methods have associated commands in datadog-agent (stuff like datadog-agent jmxfetch list-matching) that is expected to print to stdout always, as it's a debugging command. These LOGGER.debug changes, unless I'm missing something, would make it subject to the log level, thus potentially breaking the agent commands. If we're going to keep this, it should probably be info or warning at the least - still maybe preferable to print to stdout.... 🤔

ConsoleReporter is accessible via tracing agent as well, so it would be nice to be consistent with it. I'll make this an info.

truthbk · 2018-10-02T06:02:07Z

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

-        configurationList.add(new Configuration((LinkedHashMap<String, Object>) new YamlParser(this.getClass().getResourceAsStream("/jmx-2.yaml")).getParsedYaml()));
+        loadMetricConfigFiles(appConfig, configurationList);
+
+        ArrayList<LinkedHashMap<String, Object>> defaultConf = (ArrayList<LinkedHashMap<String, Object>>) new Yaml().load(this.getClass().getResourceAsStream("default-jmx-metrics.yaml"));


I guess in your opinion we should always be submitting the resource configs with jmxfetch, even if those beans aren't configured. That might be a valid point, still I think the issue here might be the fact that jmxfetch metrics are only "free" for the first 350 metrics - after this limit they will be billed as custom metrics. Although this limit is normally more than enough, we should double-check the implications, maybe make it an opt-in feature when using jmxfetch on "standalone" mode.

This change specifically just replaces jmx-1 and jmx-2 with single configuration file, and extends that configuration slightly. So this is sort of preexisting behavior.
Do you see a problem with this?

truthbk · 2018-10-02T06:09:41Z

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

+        return configs;
+    }
+
+    private void loadFileConfigs(AppConfig config, ConcurrentHashMap<String, YamlParser> configs) {


As you probably saw, currently on Agent6 the default way to provide configs to jmxfetch is via the datadog-agent API. That's where we get the metrics, as a JSON payload from. Not sure if that use-case makes sense to you guys - maybe grabbing those from the agent itself, or from the APM agent. Just mentioning it in case that might apply, the more consistent we can be across use-cases (lib vs standalone, etc), the better.

Well, 'load files' was a preexisting functionality - I've just refactored some code into this helper function.
The new thing is below - loading files via resources. And this is kind of useful because this allows APM agent to package resource with itself and pass it to jmxfetch without too much hassle.

arbll

Looks good overall. This is done in a way that does not affect the logic of Jmxfetch so I am not worried about that part.
The main thing we need to be careful with is keeping compatibility with preexisting setups.

arbll · 2018-10-05T12:25:25Z

src/main/resources/org/datadog/jmxfetch/default-jmx-metrics.yaml

+- include:
+    domain: java.lang
+    type: GarbageCollector
+    name: Copy


Not seeing a mention to this filter in https://docs.datadoghq.com/integrations/java/#description-of-the-filters. What does it do ? And how do we differenciate jvm.gc.minor_collection_count from Copy vs ParNew for ex ? Does it appends something to the metric name or add a tag ?

Thad documentation says On top of these parameters, the filters support “custom” keys which means that you can filter by bean parameters. - which is what we are using here. We are just filtering by specific GC name.

Okay I understand now, seems like I missed that part 😅 . Now for the second part of my comment, I guess you can only have one Young Gen Collector and one Old Gen Collector ?

Yes, JVM allows only one Old and one New gen collector - by there are a few different possibly types of those.

arbll · 2018-10-05T12:27:49Z

src/main/resources/org/datadog/jmxfetch/default-jmx-metrics.yaml

@@ -0,0 +1,145 @@
+# Memory


I see multiple differences between default-jmx-metrics.yaml and the default metrics that we used to collect in jmx-1 and jmx-2 . It's very important that we don't remove/rename any metrics as it would break the graphs, monitors, etc built around them.

Here are the diffs that I found Before => Now :

jvm.thread_count => thread.count

jvm.gc.parnew.time => jvm.gc.minor_collection_time ?

jvm.gc.cms.count => jvm.gc.minor_collection_count ?

There are also multiple new metrics introduced, do we need to add them and would it be possible to move them to another PR ? That way we can focus on keeping compatibility. Improving the testing here might make sense.

Essentially the idea is that JVM may run a few different types of garbage collection - but it is always one for new gen and one for old gen. Current ('old') version of the config doesn't really take this into account and sort of lumps the together - and also names metrics with GC type name, making it further confusing since it is not what actually is being reported.

The change I'm proposing is using example from here: https://github.com/DataDog/integrations-core/blob/bf11a2812c56b129c56f5cc389e74ee469bbd414/cassandra/datadog_checks/cassandra/data/conf.yaml (and couple of other places) that covers all known GC types and put them into predefined metrics. This would mean clients get GC metrics properly separated by GC type which seems like a useful thing.

Would it be ok if I leave current change in place and put back old metrics I've removed/changed?

I've pushed that change - please let me know if this is an acceptable solution.

Thanks for that. This sounds acceptable and adding 4 new metrics should not be an issue. This would also have the nice side effect of allowing us to remove some complexity from the config files of other jmx integrations like cassandra.
We should remove jvm.gc.parnew.time and jvm.gc.cms.count from the docs and add the new ones after this is merged.

I'll discuss this changes with my team to see if there is anything I am missing on why this was not added previously. I'll then do some sanity check with A5 and A6 and if everything looks good I'll approve

arbll · 2018-10-05T12:30:53Z

src/test/resources/org/datadog/jmxfetch/default-jmx-metrics.yaml

@@ -0,0 +1,43 @@
+# This is a reduced set of 'default' metrics to make tests more predictable


Same as above, let's keep compatibility with the metric names. You can use https://docs.datadoghq.com/integrations/java/#metrics

This file is actually use only by tests, so it is not terribly important what specific metrics are inhere.

Yes I know but I was hoping that the tests would catch the metric name changes to avoid regressions. Did they not ?

I had to make some changes to tests to make build pass... So I guess they did to some extent.

arbll · 2018-10-05T12:33:34Z

pom.xml

-	<packaging>jar</packaging>
+    <groupId>datadog</groupId>
+    <artifactId>jmxfetch</artifactId>
+    <version>0.20.3</version>


We'll bump that in another PR (and probably increase the minor since it's a pretty big change 😛 )

Should be fixed now.

arbll

Looks good to me now, sorry that it was so long to merge this. Once https://github.com/DataDog/jmxfetch/pull/180/files#r222989521 is addressed, feel free to merge.

arbll

mar-kolya added 2 commits September 17, 2018 13:16

Add toString to ExitWatcher so help output looks better

dc94710

Alow running as a library from another process

930325e

mar-kolya mentioned this pull request Sep 26, 2018

circleci #183

Merged

truthbk reviewed Oct 2, 2018

View reviewed changes

mar-kolya added 2 commits October 2, 2018 08:41

Do not pass logLocation to Appender constructor since it can be null

b40d641

Use info log level in ConsoleReporter

303d0da

olivielpeau assigned arbll Oct 5, 2018

arbll suggested changes Oct 5, 2018

View reviewed changes

Put back old metrcs for compatibility

cfc9603

arbll approved these changes Oct 9, 2018

View reviewed changes

mar-kolya added 2 commits October 9, 2018 09:32

Undo version change

62983bb

Remove test release information from pom

2fdd87a

arbll approved these changes Oct 9, 2018

View reviewed changes

mar-kolya merged commit dd406bb into DataDog:master Oct 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to library #180

Convert to library #180

mar-kolya commented Sep 17, 2018 •

edited

Loading

truthbk left a comment

truthbk Oct 2, 2018

mar-kolya Oct 2, 2018

arbll Oct 5, 2018

mar-kolya Oct 5, 2018

truthbk Oct 2, 2018

mar-kolya Oct 2, 2018 •

edited

Loading

truthbk Oct 2, 2018

mar-kolya Oct 2, 2018

truthbk Oct 2, 2018

mar-kolya Oct 2, 2018

arbll left a comment

arbll Oct 5, 2018 •

edited

Loading

mar-kolya Oct 5, 2018

arbll Oct 5, 2018

mar-kolya Oct 5, 2018

arbll Oct 5, 2018

mar-kolya Oct 5, 2018

mar-kolya Oct 5, 2018

arbll Oct 5, 2018

arbll Oct 5, 2018 •

edited

Loading

arbll Oct 5, 2018

mar-kolya Oct 5, 2018

arbll Oct 5, 2018

mar-kolya Oct 5, 2018

arbll Oct 5, 2018

mar-kolya Oct 9, 2018

arbll left a comment

arbll left a comment

		@@ -0,0 +1,43 @@
		# This is a reduced set of 'default' metrics to make tests more predictable

Convert to library #180

Convert to library #180

Conversation

mar-kolya commented Sep 17, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mar-kolya Oct 2, 2018 • 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

arbll left a comment

Choose a reason for hiding this comment

arbll Oct 5, 2018 • 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

arbll Oct 5, 2018 • 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

arbll left a comment

Choose a reason for hiding this comment

arbll left a comment

Choose a reason for hiding this comment

mar-kolya commented Sep 17, 2018 •

edited

Loading

mar-kolya Oct 2, 2018 •

edited

Loading

arbll Oct 5, 2018 •

edited

Loading

arbll Oct 5, 2018 •

edited

Loading