[jmxfetch] Add agent5-like JMXFetch helper commands #1208

arbll · 2018-02-07T19:04:47Z

What does this PR do?

Adds agent5-like JMXFetch helper commands.

https://docs.datadoghq.com/integrations/java/#troubleshooting

Motivation

Help with jmx troubleshooting.

Additional Notes

I would advice to review the second commit (656bb20) separately since it's a refactoring and could be merged separately from the rest.

Depends on DataDog/jmxfetch#171

olivielpeau · 2018-02-12T13:14:22Z

discussed this IRL: this would make the helpers work only if for example the foo check's config is defined in a file located at conf.d/foo.d/conf.yaml, which is not always the case (and, actually, in most cases it's not the only file where the config would be located).

Let's generalize this to at least pull the configs from the FileProvider config provider (and ideally all valid config providers, depending on the amount of work needed on JMXFetch's side)

olivielpeau · 2018-03-06T15:10:04Z

@arbll coming back to reviewing this, have a few pre-review comments:

could you update the docs to reflect the new commands, and could you also document their caveats if there are any? (for instance on the source of the yaml configs)
can you fix the merge conflict?

Had a quick look at the code and didn't notice any blocker, but I'll give this a thorough review once you've addressed these 2 points, thanks! :)

olivielpeau

I'm a bit concerned about the general approach of copying check configs to a temp directory and running jmxfetch against it. We have to be very careful about the permissions that get applied to these files, and if the command fails unexpectedly the copied files would remain there. These files can very well contain secrets that shouldn't be written in some unclear location on the filesystem, even if the ownership/perms are good.

If there's no other reasonable way to implement this, I think we should make the behavior very clear by asking users to provide a directory (as a command line option) where all the pulled configs would be written to.

Talking about alternative implementation approaches, have you looked into making this command use the same approach as a running agent (i.e. the agent provides an authenticated https endpoint that JMXFetch hits to pull the check configs)? Is JMXFetch able to run list_ commands and pull configs from that endpoint instead of the filesystem?

olivielpeau · 2018-03-15T19:12:35Z

cmd/agent/app/jmx.go

+	jmxListCmd.AddCommand(jmxListEverythingCmd, jmxListMatchingCmd, jmxListLimitedCmd, jmxListCollectedCmd, jmxListNotMatchingCmd)
+
+	jmxListCmd.PersistentFlags().StringSliceVar(&checks, "checks", []string{"jmx"}, "JMX checks (ex: jmx,tomcat)")
+	jmxCollectCmd.PersistentFlags().StringSliceVar(&checks, "checks", []string{"jmx"}, "JMX checks (ex: jmx,tomcat)")


It's a good idea to allow specifying checks, but the default should be that it collects from all configured jmx checks (unless it's very complex to implement)

olivielpeau · 2018-03-15T19:25:21Z

cmd/agent/app/jmx.go

+	if err != nil {
+		log.Fatalln(err)
+	}
+	defer os.RemoveAll(dir)


deferred calls won't be executed if os.Exit is called, which is what log.Fatal does.

Alternative: make the commands return an error, use cobra.Command{RunE: [...]}. Commands return an error whenever they run into "fatal" errors like this, and cobra takes cares of printing the error message.

olivielpeau · 2018-03-15T19:56:51Z

cmd/agent/app/jmx.go

+	for _, c := range configs {
+		if strings.EqualFold(c.Name, checkName) {
+			if c.IsJMX() {
+				return &c, nil


Unhandled edge case: unless I'm missing something, configs may contain multiple configs for a given check name. This would only take into account the first one.

olivielpeau · 2018-03-16T10:50:00Z

Removing the 6.1 milestone on this, we have to agree on an general implementation design and the 6.1 freeze is coming soon

…etch

truthbk

Nice! Mostly looks great, just a couple of nits and questions!

truthbk · 2018-04-16T15:34:16Z

cmd/agent/app/jmx.go

+	config.Datadog.Set("cmd_port", 0)
+
+	// start the cmd HTTP server
+	if err := api.StartServer(); err != nil {


If the agent is running won't this try to listen on the same port as the running agent and thus fail? Maybe I'm missing something because you would've surely encountered this while testing.... 🤔

Good question. I am setting the port to 0 to let the OS attribute one for me. The port is then given to jmxfetch as a command line parameter

truthbk · 2018-04-16T15:54:51Z

pkg/jmxfetch/jmxfetch.go

+}
+
+// Run starts the JMXFetch process
+func (j *JMXFetch) Run() error {


Can we call this Start() instead? We're wrapping exec.Command pretty closely, and I'd rather not be misled to think that this method will actually block until completion.

truthbk · 2018-04-16T15:55:48Z

pkg/collector/corechecks/embed/jmx.go

+	c.runner.LogLevel = config.Datadog.GetString("log_level")
+	c.runner.JmxExitFile = jmxExitFile
+
+	err := c.runner.Run()


nit: see my comment below regarding the JMXFetch interface :)

truthbk · 2018-04-16T16:27:31Z

cmd/agent/app/jmx.go

+	return nil
+}
+
+func loadConfigs() {


Wondering if it would be more REST idiomatic to load all configs and use a GET query parameter to decide what we return. It feels a little more natural to me and a more predictable behavior. Your approach does minimize code changes on the JMXFetch side though.

truthbk · 2018-04-16T16:31:14Z

pkg/jmxfetch/jmxfetch.go

+	)
+
+	if j.ConfDirectory != "" {
+		subprocessArgs = append(subprocessArgs, "--check")


This is actually kind of irrelevant now right? Looks like the check will just pull all configs available on the agent endpoint...

truthbk · 2018-04-16T16:33:21Z

pkg/collector/check/check.go

-			yamlBuff.Write([]byte(line))
-			yamlBuff.Write([]byte("\n"))
-		}
+	buffer, err := yaml.Marshal(&rawConfig)


truthbk · 2018-04-17T09:07:42Z

pkg/collector/corechecks/embed/jmxloader.go

@@ -21,6 +21,15 @@ import (

 var JMXConfigCache = cache.NewBasicCache()

+func AddJMXCachedConfig(config check.Config) {


This function is exported so could you please add a comment for the godoc describing its (obvious) behavior and the fact it's a thread-safe call?

truthbk

Looks good to me! 👍

new approach does not use files, relies on IPC via an https endpoint

arbll requested a review from a team as a code owner February 7, 2018 19:04

arbll force-pushed the arbll/jmx-helper-commands branch 2 times, most recently from dcac445 to eb77bd0 Compare February 7, 2018 19:32

arbll force-pushed the arbll/jmx-helper-commands branch from eb77bd0 to fb75f5a Compare February 13, 2018 10:50

arbll requested a review from a team as a code owner February 13, 2018 10:50

arbll added this to the 6.0.0 milestone Feb 16, 2018

olivielpeau modified the milestones: 6.0.0, 6.1.0 Feb 20, 2018

arbll force-pushed the arbll/jmx-helper-commands branch from fb75f5a to faea963 Compare March 1, 2018 11:20

arbll force-pushed the arbll/jmx-helper-commands branch from faea963 to 9de5251 Compare March 6, 2018 16:56

olivielpeau previously requested changes Mar 15, 2018

View reviewed changes

olivielpeau modified the milestones: 6.1.0, 6.2.0 Mar 16, 2018

olivielpeau added the do-not-merge/WIP label Apr 10, 2018

arbll added 13 commits April 16, 2018 14:10

LUL

c498ff1

extract jmxfetch execution logic

84071c7

working jmx helper commands

2701858

linux by default

a2b0d02

sugar

c848f7a

copyright & license

e026bb2

comments

c26cb58

typo

dfb53d6

reno

b4b4076

make getAllConfigs public

079bd67

fix config yaml marshaling

d0fe668

retreive JMX configs using AC

cab3911

check if config is for jmx

7a09515

arbll added 10 commits April 16, 2018 14:10

add missing collect command

3367d7c

document changes

edec6f0

spacing

ac83bfa

better formating

b7532fc

doc

3056100

update failing test

94b0e25

Use https endpoint instead of temp config file to feed config to jmxf…

4f49093

…etch

fix typo

9e2f59b

default to all checks

2b587d8

Add #1461 changes

a2c398d

arbll force-pushed the arbll/jmx-helper-commands branch from 74da9cc to a2c398d Compare April 16, 2018 12:11

Clean things up

a141133

arbll added [deprecated] team/agent-core Deprecated. Use metrics-logs / shared-components labels instead.. kind/feature and removed do-not-merge/WIP labels Apr 16, 2018

truthbk self-requested a review April 16, 2018 14:56

truthbk reviewed Apr 17, 2018

View reviewed changes

arbll added 3 commits April 18, 2018 11:13

run -> start

2f1d085

remove unused ConfDirectory option

f879357

comment jmx config cache purpose

ce72345

truthbk previously approved these changes Apr 19, 2018

View reviewed changes

Update changes.md

27595b4

arbll dismissed truthbk’s stale review via 27595b4 April 19, 2018 13:17

arbll added 2 commits April 19, 2018 15:19

Update changes.md

1a22c7c

Update changes.md

a8dc42a

arbll merged commit 7c492b6 into master Apr 19, 2018

arbll deleted the arbll/jmx-helper-commands branch April 19, 2018 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jmxfetch] Add agent5-like JMXFetch helper commands #1208

[jmxfetch] Add agent5-like JMXFetch helper commands #1208

arbll commented Feb 7, 2018 •

edited

Loading

olivielpeau commented Feb 12, 2018

olivielpeau commented Mar 6, 2018

olivielpeau left a comment •

edited

Loading

olivielpeau Mar 15, 2018

olivielpeau Mar 15, 2018

olivielpeau Mar 15, 2018

olivielpeau commented Mar 16, 2018

truthbk left a comment

truthbk Apr 16, 2018

arbll Apr 17, 2018

truthbk Apr 16, 2018

truthbk Apr 16, 2018

truthbk Apr 16, 2018 •

edited

Loading

truthbk Apr 16, 2018

truthbk Apr 16, 2018

truthbk Apr 17, 2018

truthbk left a comment

		@@ -21,6 +21,15 @@ import (

		var JMXConfigCache = cache.NewBasicCache()

		func AddJMXCachedConfig(config check.Config) {

[jmxfetch] Add agent5-like JMXFetch helper commands #1208

[jmxfetch] Add agent5-like JMXFetch helper commands #1208

Conversation

arbll commented Feb 7, 2018 • edited Loading

What does this PR do?

Motivation

Additional Notes

olivielpeau commented Feb 12, 2018

olivielpeau commented Mar 6, 2018

olivielpeau left a comment • 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

olivielpeau commented Mar 16, 2018

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

truthbk Apr 16, 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

truthbk left a comment

Choose a reason for hiding this comment

arbll commented Feb 7, 2018 •

edited

Loading

olivielpeau left a comment •

edited

Loading

truthbk Apr 16, 2018 •

edited

Loading