Metricbeat: Add munin module #6517

jsoriano · 2018-03-09T07:20:03Z

Add module to export metrics from munin nodes (#6473). It can obtain metrics from any agent that implements the basic protocol for data exchange between master and nodes. Under the terms of this protocol, metricbeat would be acting as a master.

houndci-bot · 2018-03-09T07:20:08Z

metricbeat/module/munin/munin.go

+	return n, err
+}
+
+// Releases node connection resources


comment on exported method Node.Close should be of the form "Close ..."

houndci-bot · 2018-03-09T07:20:08Z

metricbeat/module/munin/munin.go

+	reader *bufio.Reader
+}
+
+// Connects with a munin node


comment on exported function Connect should be of the form "Connect ..."

houndci-bot · 2018-03-09T07:20:08Z

metricbeat/module/munin/munin.go

+	unknownValue = "U"
+)
+
+// Munin node client


comment on exported type Node should be of the form "Node ..." (with optional leading article)

jsoriano · 2018-03-10T03:26:59Z

jenkins, test it

exekias

Awesome new module! 🎉

I left some comments. It would be nice to generate an example data.json. have a look to make generate-json

exekias · 2018-03-11T22:27:31Z

metricbeat/module/munin/munin.go

+				continue
+			}
+			if f, err := strconv.ParseFloat(value, 64); err == nil {
+				event.Put(key, f)


for the record, this is a candidate for safemapstr.Put: #6434 as soon as it's in

FYI, the PR went in already, so you can update these :)

Good point, but I think dots are not allowed in munin metric names.
We could use safemapstr in any case to protect metricbeat against incorrect implementations of munin nodes or plugins, wdyt?

I'm wondering if the float parsing will make a difference in ES. If we have a value 3 and we parse it it will still put 3 into the event and on the ES side it will assume it's an integer so conflicts will happen afterwards. Sounds like a good use case for the local fields definitions.

Umm, so what do you recommend to do here?

I've checked metrics format, sounds like we don't need safemapstr.Put here

I think what I'm getting at we could even skip this code part here and it would make no difference? If someone wants to make sure the mapping is correct he has to use #6024

Please double check the above statement as I don't know much about the munin output.

exekias · 2018-03-11T22:27:56Z

metricbeat/module/munin/munin.go

+				event.Put(key, f)
+				continue
+			}
+			event.Put(key, value)


safemapstr.Put too

exekias · 2018-03-11T22:32:12Z

metricbeat/module/munin/node/node.go

+	config := struct{}{}
+	if err := base.Module().UnpackConfig(&config); err != nil {
+		return nil, err
+	}


I guess this is not needed, as there is no custom config to parse?

Actually there is no custom config so far, I'll remove this.

I have finally kept it to support namespaces.

ruflin · 2018-03-12T06:08:54Z

metricbeat/module/munin/munin.go

+	}
+	// Cosume and ignore first line returned by munin, it is a comment
+	// about the node
+	bufio.NewScanner(n.reader).Scan()


Should we check for the return value here? In case of false we should probably check the Err method for the error.

Yes, I will return scanner.Error().

ruflin · 2018-03-12T06:09:47Z

metricbeat/module/munin/munin.go

+	}
+
+	scanner := bufio.NewScanner(n.reader)
+	scanner.Scan()


Should we also check here the return value?

ruflin · 2018-03-12T06:10:20Z

metricbeat/module/munin/munin.go

+
+	scanner := bufio.NewScanner(n.reader)
+	scanner.Scan()
+	return strings.Fields(scanner.Text()), scanner.Err()


I see you run here scanner.Err() which makes the return value check obsolete before.

ruflin · 2018-03-12T06:16:41Z

metricbeat/module/munin/node/node.go

+}
+
+// Fetch method implements the data gathering
+func (m *MetricSet) Fetch() (common.MapStr, error) {


As if this metricset is used for different host, the returned data can contain very different structures. We should allow to set the namespace for this metricset as we do for example for the http/json metricset to make sure users can choose their namespace to prevent field conflicts.

Ok, adding namespace.

ruflin · 2018-03-12T06:18:19Z

metricbeat/module/munin/node/node.go

@@ -0,0 +1,60 @@
+package node


You wrote above that metricbeat is the master in this scenario so I was tempted to say we should call the default namespace master. But as it's about the data from the node (is that naming correct?) +1 on node.

Protocol-wise we are like implementing the server side, but the metrics on the metricset are about the node, so +1 too to node :)

ruflin

LGTM. Left some minor comments.

ruflin · 2018-03-14T07:31:14Z

metricbeat/module/munin/node/_meta/docs.asciidoc

+  hosts: ["localhost:4949"]
+---
+
+All metrics exposed by a single munin node will be sent as an only event,


"All metrics exposed by a single munin node will be sent in a single event, grouped by munin items." ?

An example here would be nice.

ruflin · 2018-03-14T07:32:18Z

metricbeat/modules.d/munin.yml.disabled

+  metricsets: ["node"]
+  enabled: false
+  period: 10s
+  hosts: ["localhost:4949"]


The namespace config should be an option here.

ruflin · 2018-03-14T07:36:13Z

metricbeat/module/munin/node/node.go

+	cfgwarn.Experimental("The munin node metricset is experimental.")
+
+	config := struct {
+		Namespace string `config:"namespace" validate:"required"`


I know this is not the case in the other implementations but we should prefix all configs with the metricset to make sure it's obvious to which metricset they belong. So this should be node.namespace.

ruflin · 2018-03-14T07:36:36Z

metricbeat/module/munin/_meta/config.yml

+  metricsets: ["node"]
+  enabled: false
+  period: 10s
+  hosts: ["localhost:4949"]


Add namespace config.

ruflin · 2018-03-14T07:36:46Z

metricbeat/docs/modules/munin.asciidoc

+  metricsets: ["node"]
+  enabled: false
+  period: 10s
+  hosts: ["localhost:4949"]


Add namespace config

jsoriano · 2018-03-16T07:13:16Z

Added namespace config where it was missing, and renamed it to node.namespace.

jsoriano · 2018-03-16T07:47:55Z

I still have to take a look to the data.json generator.

ruflin · 2018-03-16T14:21:50Z

@jsoriano I think for this metricset the data.json does not make much sense. We also skipped it for the other "dynamic" metricsets. Could you rebase again and squash your commits into one with a nice commit message?

@exekias Can you also check the PR again?

jsoriano · 2018-03-16T16:42:18Z

@ruflin, after changing from namespace to node.namespace integration test is not working, metricbeat complains about missing required field. Actually in the generated build/system-tests/last_run/metricbeat.yml I couldn't see this setting.

I have seen in the base template that it expects the namespace to be in namespace. I have had to add it in the extras to make it work:

            "extras": {
                "node.namespace": namespace,
            },

Is this ok?

Add munin module, with node metricset that obtain metrics from a running munin node using the same protocol used by munin masters.

ruflin · 2018-03-19T08:09:01Z

The extras is fine to solve the CI issue.

I'm going forth and back if namespace should be prefix by the metricset or not. The reason I like to have it prefix is that we do that will all metricset specific configs. Like this it becomes obvious to which metricset it belongs and does not accidentially overlap with other metricsets configs. Probably the reason we have it globally is that "dynamic" metricsets like this one should also be configured alone, meaning that the list of metricsets enable in a module should only contain this one. We don't enforce this yet but probably should. Like this an overlap cannot happen. Having the namespace config option on the module level could allow us to make it supported in a generic way and not have to implement it in each module.

@exekias @andrewkroh Any thoughts here?

ruflin · 2018-03-19T08:10:48Z

I merged the PR to have the munin module in. The discussion about the naming of the namespace option should continue. As the module is not yet release and still experimental we can still make changes.

jsoriano added enhancement docs module review Metricbeat Metricbeat labels Mar 9, 2018

houndci-bot reviewed Mar 9, 2018

View reviewed changes

jsoriano force-pushed the munin-module branch 5 times, most recently from 45757a0 to 441345c Compare March 10, 2018 02:18

exekias reviewed Mar 11, 2018

View reviewed changes

ruflin reviewed Mar 12, 2018

View reviewed changes

jsoriano force-pushed the munin-module branch from 441345c to 2110da7 Compare March 14, 2018 06:08

ruflin reviewed Mar 14, 2018

View reviewed changes

jsoriano force-pushed the munin-module branch from 2110da7 to 8aed265 Compare March 16, 2018 07:12

jsoriano force-pushed the munin-module branch from 8aed265 to e812daf Compare March 16, 2018 07:18

ruflin approved these changes Mar 16, 2018

View reviewed changes

jsoriano force-pushed the munin-module branch from e812daf to e00ec8f Compare March 16, 2018 16:40

Metricbeat: Add munin module (elastic#6473)

af03473

Add munin module, with node metricset that obtain metrics from a running munin node using the same protocol used by munin masters.

jsoriano force-pushed the munin-module branch from e00ec8f to af03473 Compare March 16, 2018 16:43

ruflin merged commit 7756f94 into elastic:master Mar 19, 2018

jsoriano mentioned this pull request Mar 19, 2018

Support for munin module in Metricbeat #6473

Closed

jsoriano deleted the munin-module branch March 22, 2018 08:49

Metricbeat: Add munin module #6517

Metricbeat: Add munin module #6517

Conversation

jsoriano commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Mar 10, 2018

exekias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Mar 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Mar 14, 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

ruflin 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

jsoriano commented Mar 16, 2018

jsoriano commented Mar 16, 2018

ruflin commented Mar 16, 2018

jsoriano commented Mar 16, 2018 • edited Loading

ruflin commented Mar 19, 2018

ruflin commented Mar 19, 2018

jsoriano Mar 14, 2018 •

edited

Loading

jsoriano Mar 14, 2018 •

edited

Loading

jsoriano commented Mar 16, 2018 •

edited

Loading