[flare] Add JMXFetch-specific info #1726

olivielpeau · 2015-06-25T16:07:50Z

Adds in a jmxinfo directory:

2 JMXFetch status files: jmx_status.yaml and jmx_status_python.yaml
JMXfetch commands output: list_matching_attributes and list_everything
java -version output (using the configured java path) in a java_version.log file

@remh One thing that may not work properly in flare is the change I've made here https://github.com/DataDog/dd-agent/compare/olivielpeau/jmx-flare?expand=1#diff-64a477c26f008c29252aad6c1499c099R271 , which basically leaves file descriptors open (close_fds=False) when the JMXFetch subprocess is launched from the flare command. Do you know what issues can happen when the main process leaves the file descriptors open ?

cc @degemer @yannmh

degemer · 2015-06-29T15:45:29Z

utils/flare.py

        if os.path.exists(temp_path):
            os.remove(temp_path)
        backup_out, backup_err = sys.stdout, sys.stderr
        backup_handlers = logging.root.handlers[:]
        out, err = StringIO.StringIO(), StringIO.StringIO()
+        if logger_name:
+            # If specified, redirect logger output to `out`
+            logger = logging.getLogger(logger_name)


Since you send the output of jmxfetch to stdout (https://github.com/DataDog/dd-agent/pull/1726/files#diff-64a477c26f008c29252aad6c1499c099R283), do you still need this ? I guess it's for jmxfetch.py, what does it log ? (I have no idea, just asking)

It's for jmxfetch.py indeed, it logs exceptions that occur, problems with configuration files, and the exact command that is used to launch JMXFetch (which includes the java bin path and JVM options)

degemer · 2015-06-29T17:23:39Z

Really nice, thanks !
Two remarks on the output of the flare command:

jmx commands output is always collected even if no integration is setup (files are ~empty, but you still display collecting java version), I think it might mislead users, could you not collect it if no integration using jmxfetch is setup ? (JMXFetch.should_run() ?)
when LOG_LEVEL: DEBUG (support recommends to do it before sending logs, so it's quite often), the ouptut is really verbose, maybe too much. Do you know if you could remove these debug statements (users won't have to debug flare) ?

yannmh · 2015-06-29T17:40:14Z

agent.py

    Watchdog,
 )
 from utils.flare import configcheck, Flare
+from utils.jmx_command import jmx_command


👍 for the cleanup !

Maybe we should merge utils.jmx_command and utils.jmxfiles to have only a single JMX util package. What do you think ?

I agree, that makes a lot of sense, I'll regroup them in one utils.jmx module ;)

Hmm actually I get circular imports when the two are merged together, as jmxfetch imports utils.jmx (for the JMXFiles class) that itself imports jmxfetch (for the JMXFetch class used by jmx_command).

So unless there's a better solution I'll leave the two modules separated for now...

Importing JMXFetch in the scope of the method only, would prevent this:

def jmx_command() from jmxfetch import JMX_LIST_COMMANDS, JMXFetch ...

yannmh · 2015-06-29T17:44:11Z

Another great feature, that will make support way easier. Thanks @olivielpeau.

Can you attach an example of a flare archive to have an idea of what it looks like ?

The command output is saved in an arbitrarily-named temp file

olivielpeau · 2015-06-30T15:22:25Z

Thanks a lot for your reviews @yannmh and @degemer!

I've added two commits that:

capture the whole output of the commands run by flare (so that users only see the actual output from flare)
only add jmx info if a valid jmx config file is present (as evaluated by JMXFetch.should_run())

And here is an example flare archive: https://www.dropbox.com/s/orjuu2kbn9d1aol/datadog-agent-2015-06-30-15-03-16.tar.bz2?dl=0

yannmh · 2015-07-01T18:44:48Z

utils/flare.py

+                os.path.join('jmxinfo', 'java_version.log'),
+                # We use lambda so that JMXFetch.get_configuration is evaluated in _add_command_output_tar,
+                # which captures the logging output from JMXFetch
+                lambda: self._java_version(JMXFetch.get_configuration(get_confd_path())[2])


Nitpicky: we are also adding the output of JMXFetch.get_configuration(get_confd_path()) to java_version.log. Is it intended ?

>>>> STDOUT <<<< tomcat check does not have a valid JMX configuration: You need to have at least one instance defined in the YAML file for this check Popen(['java', '-version'], stderr = -2, stdout = -1) called java version "1.7.0_51" Java(TM) SE Runtime Environment (build 1.7.0_51-b13) Java HotSpot(TM) 64-Bit Server VM (build 24.51-b03, mixed mode) >>>> STDERR <<<<

Maybe we could drop it and add a >>>> CMD <<<< section to _add_command_output_tar with the actual command printed (in this case it's interesting as it may contains a custom java path).

yannmh · 2015-07-01T19:05:31Z

Looks great @olivielpeau. Let's merge it whenever you want.

- extract JMXFetch setup and launch from agent.py to a separate module to allow JMXFetch call from flare - put all jmx utils (ie JMXFiles and the new jmx_command) in a single utils.jmx module

- To make it easier to get JMXFetch's configuration from flare, `get_configuration` becomes a classmethod that calls `_is_jmx_check` as a staticmethod - For more consistency, `check_list` is renamed to `checks_list`

The user should not see any output from the called commands. In `check_status.py`, stop assuming that all streams have a `name` attribute (for instance `StringIO.StringIO` instances do not have one).

The output/loggers capturing logic has been extracted in a separate method to allow retrieving the returned value of the command (useful here to get the value `jmx_process.shoud_run()`)

olivielpeau · 2015-07-01T23:01:45Z

Thanks @yannmh for your very good points ;)

I've merged jmx_command and JMXFiles in a single utils.jmx module and fixed an issue that could make flare fail silently (see 5c75b06).

Now commands can also have an optional description. jmxinfo/java_version.log now looks like (note that the Popen line is only there when the log_level is DEBUG):

>>>> CMD <<<<
java -version
>>>> STDOUT <<<<
Popen(['java', '-version'], stderr = -2, stdout = -1) called
java version "1.7.0_51"
Java(TM) SE Runtime Environment (build 1.7.0_51-b13)
Java HotSpot(TM) 64-Bit Server VM (build 24.51-b03, mixed mode)

>>>> STDERR <<<<

degemer · 2015-07-01T23:06:49Z

utils/jmx.py

-from config import _windows_commondata_path
-from util import yDumper
+from config import _windows_commondata_path, get_confd_path
+from util import get_os, yDumper


Can you remove this import ? (get_os) If get_confd_path doesn't have a get_os(), it will call it itself, so it's no necessary.

These exceptions are output either to stderr or to the returned err stream

Also remove the output of JMXFetch.get_configuration as it is redundant

Removed it as get_confd_path calls get_os on its own.

olivielpeau · 2015-07-02T19:24:42Z

Thanks again @degemer! I've changed the get_os import and the handling of exceptions:

exceptions from commands which output is printed to a file in the tar are printed in the file
exceptions from commands which output is not printed at all are printed in stderr
all Exceptions raised in those commands are handled by flare

degemer · 2015-07-02T19:28:34Z

[flare] Fix typo is not going to work as a commit short desc.
Otherwise, really nice additions, when it's fixed, !

`ouput` --> `output`

olivielpeau · 2015-07-02T19:36:21Z

Haha, I guess I got lazy on this one, I've fixed it, thanks!

Will merge as soon as the ci passes.

[flare] Add JMXFetch-specific info

olivielpeau added feature core labels Jun 25, 2015

olivielpeau force-pushed the olivielpeau/jmx-flare branch from 8e5f156 to b507c0b Compare June 29, 2015 15:18

degemer reviewed Jun 29, 2015
View reviewed changes

yannmh reviewed Jun 29, 2015
View reviewed changes

olivielpeau force-pushed the olivielpeau/jmx-flare branch from b507c0b to 7047f84 Compare June 29, 2015 21:12

olivielpeau added 2 commits June 29, 2015 17:13

[flare] Allow outputting commands to a sub-directory of the tar

6c373ef

The command output is saved in an arbitrarily-named temp file

[flare] Allow redirecting logger output of command to tar

73d25f8

olivielpeau force-pushed the olivielpeau/jmx-flare branch 2 times, most recently from 7733c03 to 378fdae Compare June 29, 2015 22:30

yannmh reviewed Jul 1, 2015
View reviewed changes

olivielpeau added 4 commits July 1, 2015 18:39

[flare] Add JMXFetch beans lists and status files to tar

fdacd64

- extract JMXFetch setup and launch from agent.py to a separate module to allow JMXFetch call from flare - put all jmx utils (ie JMXFiles and the new jmx_command) in a single utils.jmx module

[flare] Add java version output

eacc61b

- To make it easier to get JMXFetch's configuration from flare, `get_configuration` becomes a classmethod that calls `_is_jmx_check` as a staticmethod - For more consistency, `check_list` is renamed to `checks_list`

[flare] Redirect root logger's output to tar

7d79a56

The user should not see any output from the called commands. In `check_status.py`, stop assuming that all streams have a `name` attribute (for instance `StringIO.StringIO` instances do not have one).

[flare] Add jmx-specific files only if JMXFetch should run

9043c9f

The output/loggers capturing logic has been extracted in a separate method to allow retrieving the returned value of the command (useful here to get the value `jmx_process.shoud_run()`)

olivielpeau force-pushed the olivielpeau/jmx-flare branch from 5660d86 to a4dd433 Compare July 1, 2015 22:50

degemer reviewed Jul 1, 2015
View reviewed changes

olivielpeau added 3 commits July 2, 2015 15:05

[flare] Handle exceptions raised by commands with captured outputs

1c3a1a9

These exceptions are output either to stderr or to the returned err stream

[flare] Allow optional description of commands

1fa41aa

Also remove the output of JMXFetch.get_configuration as it is redundant

[jmxfetch] Remove get_os() as a parameter of get_confd_path

44467ea

Removed it as get_confd_path calls get_os on its own.

olivielpeau force-pushed the olivielpeau/jmx-flare branch from a4dd433 to 3ce28db Compare July 2, 2015 19:16

[flare] Fix minor typo in error message of _print_output_command

4981b5a

`ouput` --> `output`

olivielpeau force-pushed the olivielpeau/jmx-flare branch from 3ce28db to 4981b5a Compare July 2, 2015 19:34

olivielpeau added this to the 5.5.0 milestone Jul 2, 2015

olivielpeau added a commit that referenced this pull request Jul 2, 2015

Merge pull request #1726 from DataDog/olivielpeau/jmx-flare

5ba2c66

[flare] Add JMXFetch-specific info

olivielpeau merged commit 5ba2c66 into master Jul 2, 2015

olivielpeau deleted the olivielpeau/jmx-flare branch July 2, 2015 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flare] Add JMXFetch-specific info #1726

[flare] Add JMXFetch-specific info #1726

olivielpeau commented Jun 25, 2015

degemer Jun 29, 2015

olivielpeau Jun 29, 2015

degemer commented Jun 29, 2015

yannmh Jun 29, 2015

olivielpeau Jun 29, 2015

olivielpeau Jun 29, 2015

yannmh Jul 1, 2015

yannmh commented Jun 29, 2015

olivielpeau commented Jun 30, 2015

yannmh Jul 1, 2015

yannmh commented Jul 1, 2015

olivielpeau commented Jul 1, 2015

degemer Jul 1, 2015

olivielpeau commented Jul 2, 2015

degemer commented Jul 2, 2015

olivielpeau commented Jul 2, 2015

[flare] Add JMXFetch-specific info #1726

[flare] Add JMXFetch-specific info #1726

Conversation

olivielpeau commented Jun 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

degemer commented Jun 29, 2015

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

yannmh commented Jun 29, 2015

olivielpeau commented Jun 30, 2015

Choose a reason for hiding this comment

yannmh commented Jul 1, 2015

olivielpeau commented Jul 1, 2015

Choose a reason for hiding this comment

olivielpeau commented Jul 2, 2015

degemer commented Jul 2, 2015

olivielpeau commented Jul 2, 2015