[service_discovery][jmx] trying to pick-up JMX changes with SD. #3010

truthbk · 2016-11-09T17:24:01Z

What does this PR do?

Enables service discovery for JMX integrations.

Once a service discovery index config is detected, we parse the config from JSON (as stored in etcd/consul) to YAML and the relayed over to JMXfetch over a named pipe dd-service_discovery that lives in /tmp by default.

If multiple changes are detected, these are all submitted to JMXfetch by a single write operation to avoid race conditions. The pipe should take care of locking from the reading and writing ends, and by dropping all relevant configs in a single write operation we can guarantee all configs will be read on the JMXfetch in a single iteration and thus help avoid having multiple versions of the same config in the pipe at any given time.

Motivation

Enabling Service Discovery for JMXfetch integrations.

Testing Guidelines

An overview on testing
is available in our contribution guidelines.

Additional Notes

Related PR: DataDog/jmxfetch#113

degemer · 2016-11-09T18:03:15Z

agent.py

+JMX_GRACE_SECS = 2
+SERVICE_DISCOVERY_PREFIX = 'SD-'
+SD_PIPE_NAME = "dd-service_discovery"
+SD_PIPE_UNIX_PATH = "/tmp"


Quick note: I think using /opt/datadog-agent/run is more appropriate and easier to adapt to Windows, unless you have a good reason to use /tmp. 😃

Named pipes are only supported on windows in a special path (and I don't even think all versions will support it anyways), so we'll need a specific path for windows anyway.

I used /tmp to avoid issues with the source installs.

truthbk · 2016-11-09T19:12:21Z

TODO: I could probably add some tests for the generate_jmx_configs function.

hkaj

Nice one @truthbk ! This will be a major improvement for Java apps monitoring :)
Left a few comments.

hkaj · 2016-11-16T22:38:00Z

agent.py

+
+        if not os.path.exists(pipe_name):
+            os.mkfifo(pipe_name)
+        self.sd_pipe = os.open(pipe_name, os.O_RDWR) # RW to avoid blocking (will only W)


i don't think we should create the fifo if SD is not enabled

hkaj · 2016-11-16T22:40:29Z

config.py

@@ -1,4 +1,3 @@
-# (C) Datadog, Inc. 2010-2016


let's keep this guy around

Not sure how I got rid of that! 😓

hkaj · 2016-11-16T23:14:49Z

config.py

+            try:
+                yaml = config_to_yaml(check_config)
+                # generated["{}_{}".format(check_name, idx)] = yaml
+                generated["{}_{}".format(check_name, 0)] = yaml


what's the _0 for?

So this was actually a product of an original misunderstanding on my part. But then I saw some utility for it (although I have to take another look at this code) - for example simultaneous containers for the same service, but with different tags or whatever, that could have different configurations. On the JMX side we'd use the check name as the key in the map, so this would allow us to support that in the future, that's why we have the commented line on the top. We can discuss and possibly remove.

hkaj · 2016-11-16T23:15:52Z

config.py

+                log.debug("YAML generated: %s", yaml)
+            except Exception as e:
+                log.exception("Unable to generate YAML config for %s: %s", check_name, e)
+


except: log.exception("Unable to generate YAML config for %s", check_name)

good catch!

hkaj · 2016-11-16T23:17:22Z

datadog.conf.example

@@ -123,6 +123,9 @@ gce_updated_hostname: yes
 # and modify its value.
 # sd_template_dir: /datadog/check_configs
 #
+# JMX Service Disocvery


a better description would be nice, something like Enable service discovery for JMX checks ?

hkaj · 2016-11-16T23:38:37Z

util.py

+
+    return yaml_output
+
+def dump_yaml(path, config):


doesn't seem to be used anywhere

hkaj · 2016-11-17T10:32:02Z

agent.py

@@ -216,9 +248,17 @@ def run(self, config=None):
        if self._agentConfig.get('service_discovery'):
            self.sd_backend = get_sd_backend(self._agentConfig)

+        # Initialize Supervisor proxy (unix specific)
+        self.supervisor_proxy = self._get_supervisor_socket(self._agentConfig)


do we want to init that if SD is not enabled?

hkaj · 2016-11-17T10:32:17Z

agent.py

+        # Load JMX configs if available
+        jmx_sd_configs = generate_jmx_configs(self._agentConfig, hostname)
+        if jmx_sd_configs:
+            self._submit_jmx_service_discovery(jmx_sd_configs)


ditto, this will trigger even without SD, is that expected?

hkaj · 2016-11-21T10:08:45Z

LGTM

[service_discovery] adding named pipe for IPC. [util] if file doesnt exist, create it. [service_discovery][jmxfetch] add some service discovery configurability. [jmxfetch][service_discovery] increase max heap size for SD to 512. [service_discovery][jmxfetch] create jmx service discovery option in config. [service_discovery][jmx] if jmx is not up, try to bring it up - wait for it to boot too. [service_discovery] if the previous index was None, consider that a change (bootup changes to apply). [service_discovery][jmx] cleaning up service discovery/supervisor interactions. [service_discovery][jmx] supervisor_proxy should not be None, cleanup. [service_discovery][jmxfetch] avoid double-reload of checks at start-up. Refactor.

…pt/datadog-agent/run/

truthbk · 2016-12-02T19:28:19Z

No changes to the logic, just added the test. We're green, so merging! Thanks @hkaj

* master: (53 commits) [nginx] Update example config [service_discovery] Add a Zookeeper service discovery implementation. [aggregator] if sample rate is bad, fix it but still parse tags. (#3073) [yarn] whitelist authorized application_tags Alex poe/update jmx with refresh beans (#3068) [config] Fix `_is_affirmative` when passed argument is `None` (#3063) Send all configured tags with process checks. (#2976) fix flake8 errors [flare] ignore whitespace before proxy credentials [core] add a switch to disable profiling, but still use developer mode (#2898) [tests] allow tests to use the additional_checksd parameter (#3056) [service_discovery][jmx] trying to pick-up JMX changes with SD. (#3010) [install_script] Make `dd-agent` group of `datadog.conf` (#3036) [postgres] Allow disable postgresql.database_size (#3035) [core] Fixes IndexError for process lookup (#3043) remove warning message leaking password strings (#3053) trap psutil.NoSuchProcess exception (#3052) Fix grammar and casing in exception text (#3050) allow override of kubelet host with KUBERNETES_KUBELET_HOST env var [service discovery] properly handle config reload for removed containers ...

truthbk force-pushed the jaime/pipedsd branch 2 times, most recently from dd70cb1 to 7c96a62 Compare November 9, 2016 17:59

degemer reviewed Nov 9, 2016

View reviewed changes

truthbk mentioned this pull request Nov 9, 2016

[service_discovery] enabling service discovery via named pipe. DataDog/jmxfetch#113

Merged

truthbk force-pushed the jaime/pipedsd branch 2 times, most recently from 177649c to b9f8d01 Compare November 16, 2016 18:45

hkaj requested changes Nov 17, 2016

View reviewed changes

truthbk force-pushed the jaime/pipedsd branch from 25c5199 to edb94e0 Compare November 18, 2016 15:33

hkaj approved these changes Nov 21, 2016

View reviewed changes

truthbk added 6 commits December 1, 2016 18:51

[service_discovery][jmx] enable auto-conf for JMX checks.

316cf62

[service_discovery][jmx] some cleanup.

9cc3762

[service_discovery][jmx] refactor config. Default pipe location to /o…

779f58d

…pt/datadog-agent/run/

[service_discovery][jmx] adding JMX YAML generation test.

51808fa

[service_discovery][jmx] empty result set if configs not retrievable.

aec4d71

truthbk force-pushed the jaime/pipedsd branch from 63d8ce5 to aec4d71 Compare December 2, 2016 18:41

[service_discovery][tests][skip ci] unneccessary noqa.

809494b

truthbk merged commit 13d63df into master Dec 2, 2016

truthbk deleted the jaime/pipedsd branch December 2, 2016 19:28

olivielpeau mentioned this pull request Dec 6, 2016

[config] Fix _is_affirmative when passed argument is None #3063

Merged

olivielpeau added this to the 5.11.0 milestone Dec 6, 2016

hkaj mentioned this pull request Jan 18, 2017

prevent bad service disco configs from crashing dd-agent #3110

Closed

masci modified the milestones: 5.11.0, 5.12.0 Jan 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[service_discovery][jmx] trying to pick-up JMX changes with SD. #3010

[service_discovery][jmx] trying to pick-up JMX changes with SD. #3010

truthbk commented Nov 9, 2016 •

edited

Loading

degemer Nov 9, 2016

truthbk Nov 9, 2016 •

edited

Loading

truthbk commented Nov 9, 2016

hkaj left a comment

hkaj Nov 16, 2016

hkaj Nov 16, 2016

truthbk Nov 17, 2016

hkaj Nov 16, 2016

truthbk Nov 17, 2016

hkaj Nov 16, 2016

truthbk Nov 17, 2016

hkaj Nov 16, 2016

hkaj Nov 16, 2016

hkaj Nov 17, 2016

hkaj Nov 17, 2016

hkaj commented Nov 21, 2016

truthbk commented Dec 2, 2016

[service_discovery][jmx] trying to pick-up JMX changes with SD. #3010

[service_discovery][jmx] trying to pick-up JMX changes with SD. #3010

Conversation

truthbk commented Nov 9, 2016 • edited Loading

What does this PR do?

Motivation

Testing Guidelines

Additional Notes

Choose a reason for hiding this comment

truthbk Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

truthbk commented Nov 9, 2016

hkaj 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

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

hkaj commented Nov 21, 2016

truthbk commented Dec 2, 2016

truthbk commented Nov 9, 2016 •

edited

Loading

truthbk Nov 9, 2016 •

edited

Loading