Unify default module configurations #6908

exekias · 2018-04-20T10:43:07Z

Since we split module configurations into modules.d folder, we can keep
configurations more verbose, so users have a quick reference when
opening them for editing.

This change unifies configuration files using the followin rules:

Write at least the following settings, in this order: module,
metricsets, period, hosts.
Default metricsets list shown as it helps to know what's the list,
or disable just one. Inline if there is only one metricset, one line
per metricset if there is more than one.
Some modules have exceptions and include more definitions, or different
defaults.
Comments are allowed (and encouraged), both to explain a setting and
to suggest a full configuration.
enabled setting is not shown at all, modules are enabled by using
metricbeat modules enable <module name>. Use comments to suggest
settings that are disabled by default.

exekias · 2018-04-20T10:46:37Z

Changes are duplicated so there is only need to review 32 files under module/*/_meta/config.yml, modules.d and docs are generated from there

jsoriano · 2018-04-20T11:39:42Z

metricbeat/module/system/_meta/config.yml

+    #- core
+    #- diskio
+    #- socket
+  processes: ['.*']


This is the default, should we leave it commented out?

ruflin

Here is the meta issue where we discussed the initial changes: #6668 We should separate the discussion here between the short config and reference config:

short config:

We removed the metricsets from the config.yml to make sure always the defaults apply. So if we add one more metricset, the config does not have to change and you automatically get the new metricset. I like the config without the metricsets as it focuses on the modules. Most users are at first probably not interested in the metricsets.
period: Our take was to remove it if it's the default and only leave it in if it deviates. It makes the config shorter
+1 on module and hosts
Instead of adding lots of comments, we should link to the docs page. The reference config is that contains all the comments.

I personally would prefer that we keep the short config really short and use the reference for all the comments etc. I think this PR starts to blur the lines again.

ruflin · 2018-04-20T12:26:57Z

metricbeat/module/aerospike/_meta/config.yml

@@ -1,2 +1,4 @@
 - module: aerospike
+  metricsets: ["namespace"]


The reason we left out the metricsets in the short config is to make sure we always have the default behaviour an it's not influence by the config. So if we add one more metricset to the defaults, a user will get it automatically without having to change the config.

I see this point, what about having them commented?

ruflin · 2018-04-20T12:27:26Z

metricbeat/module/aerospike/_meta/config.yml

@@ -1,2 +1,4 @@
 - module: aerospike
+  metricsets: ["namespace"]
+  period: 10s


I remember @andrewkroh suggested to remove period. The part I like about it is that it makes the config even shorter.

One option would be to only include the period when it's not the default of 10s.

In general, I would prefer to keep it as it adds visibility to the setting, any user opening the config file for the module will know what that is and how it works really quick. If we remove it they need to go back to the docs.

ruflin · 2018-04-20T12:28:01Z

metricbeat/module/ceph/_meta/config.yml

@@ -1,7 +1,16 @@
 - module: ceph
-  metricsets: ["cluster_health", "cluster_status", "monitor_health"]
+  metricsets:
+    - cluster_health


+1 on the new structure here, makes it easier to comment one.

ruflin · 2018-04-20T12:28:13Z

metricbeat/module/ceph/_meta/config.yml

  period: 10s
+  hosts: ["localhost:5000"]


oh, seems like we forgot hosts here :-(

ruflin · 2018-04-20T12:28:24Z

metricbeat/module/ceph/_meta/config.yml

+    - cluster_disk
+    - osd_tree
+    - osd_df
+    - pool_disk
  period: 1m


this is a good example I think where having period in makes sense.

ruflin · 2018-04-20T12:29:33Z

metricbeat/module/windows/_meta/config.yml

@@ -1,2 +1,8 @@
 - module: windows
-  period: 60s
+  metricsets: ["perfmon"]


This is not enabled by default so it should not be here.

I commented it out

andrewkroh

👍

andrewkroh · 2018-04-20T12:59:42Z

metricbeat/modules.d/system.yml

-#    - core
-#    - diskio
-#    - socket
-
 - module: system
  period: 1m
  metricsets:


I think it might make sense to use the ignore_types option instead of the filter. Or maybe these mounts are filtered by default now?

That's a good question, @jsoriano do your recent changes affect this?

Oh sorry, I forgot this question. My latest changes filter by default the filesystem types marked as nodev in /proc/filesystems, and remove duplications caused by bind mounts and similar. Probably this is enough to have meaningful events by default without needing these processors.

andrewkroh · 2018-04-20T13:02:34Z

metricbeat/modules.d/windows.yml.disabled

-  period: 60s
+  metricsets: ["perfmon"]
+  period: 10s
+  perfmon.counters:


While it would be a deviance from the defaults, I think that including an example would be very helpful to users. We have some in the docs.

andrewkroh · 2018-04-20T13:04:29Z

metricbeat/modules.d/system.yml

+  period: 10s
+  metricsets:
+    - cpu
+    - load


load doesn't exist for Windows. Does load get deleted during packaging for Windows (check before-build in the Makefile)?

It should, but it's a good point, I must test this

Since we split module configurations into modules.d folder, we can keep configurations more verbose, so users can have a quick reference when opening them for editing. This change unifies configuration files using the followin rules: * Write at least the following settings, in this order: `module`, `metricsets`, `period`, `hosts`. * Default metricsets list shown as it helps to know what's the list, or disable just one. Inline if there is only one metricset, one line per metricset if there is more than one. * Some modules have exceptions and include more definitions, different defaults. * Comments are allowed (and encouraged), both to explain a setting and to suggest a full configuration. * `enabled` setting is not shown at all, modules are enabled by using `metricbeat modules enable <module name>`. Use comments to suggest settings that are disabled by default.

exekias · 2018-04-25T17:13:48Z

I gave this another spin: included link to docs + commented metricsets

ruflin · 2018-04-26T07:47:39Z

metricbeat/docs/modules/golang.asciidoc

@@ -20,7 +20,9 @@ in <<configuration-metricbeat>>. Here is an example configuration:
 ----
 metricbeat.modules:
 - module: golang
-  metricsets: ["expvar","heap"]
+#  metricsets:


Nit: Could you put the # directly before the metricset without space? It's more similar to what we do everywhere else.

ruflin · 2018-04-26T07:48:01Z

metricbeat/docs/modules/jolokia.asciidoc

@@ -19,28 +19,32 @@ in <<configuration-metricbeat>>. Here is an example configuration:
 ----
 metricbeat.modules:
 - module: jolokia
-  metricsets: ["jmx"]
+#  metricsets: ["jmx"]


Comment above applies to all #

ruflin · 2018-04-26T07:49:06Z

metricbeat/module/mysql/_meta/config.yml

+  # Password of hosts. Empty by default.
+  #password: secret
+
+  # By setting raw to true, all raw fields from the status metricset will be added to the event.


Let's remove this from here as I don't want to encourage people to use it.

ruflin · 2018-04-26T07:49:26Z

metricbeat/module/mysql/_meta/config.yml

@@ -1,2 +1,17 @@
 - module: mysql
-  hosts: ["tcp(127.0.0.1:3306)/"]
+#  metricsets: ["status"]


Could we use here also the - notation already?

ruflin · 2018-04-26T07:49:52Z

metricbeat/module/php_fpm/_meta/config.yml

  hosts: ["localhost:8080"]
+  status_path: "/status"


Is this a default? If yes, it should be commented out.

ruflin · 2018-04-26T07:50:42Z

metricbeat/module/redis/_meta/config.yml

+  # should never take longer then period, as otherwise calls can pile up.
+  #timeout: 1s
+
+  # Optional fields to be added to each event


This is not specific to redis, lets remove it.

ruflin · 2018-04-26T07:50:56Z

metricbeat/module/redis/_meta/config.yml

+  # Max number of concurrent connections. Default: 10
+  #maxconn: 10
+
+  # Filters can be used to reduce the number of fields sent.


This is not redis specific, lets remove it.

ruflin · 2018-04-26T07:52:23Z

metricbeat/scripts/modules_collector.py

+
+    # TODO add module release status if beta or experimental
+    header = """# Module: {module}
+# Docs: https://www.elastic.co/guide/en/beats/metricbeat/{docs_branch}/metricbeat-module-{module}.html


ruflin · 2018-04-26T07:53:08Z

metricbeat/scripts/modules_collector.py

+
+"""
+
+    # Create directory for module confs


Could we have the docs link also in the reference config file?

I gave this a try but it would change reference file for all beats, and URL patterns are different. Also, not sure if it's that useful there, as it's the reference file. What do you think?

As this is the module collection only for metricbeat, why did it change the other config files?

I was talking about reference file, which is being collected by a different script

Got it, lets leave it out for now then.

ruflin · 2018-04-26T07:53:53Z

metricbeat/Makefile

@@ -10,6 +10,7 @@ ES_BEATS?=..
 GOX_OS=netbsd linux windows
 GOX_FLAGS=-arch="amd64 386 arm ppc64 ppc64le"

+DOCS_BRANCH=$(shell grep doc-branch ../libbeat/docs/version.asciidoc | cut -c 14-)


LGTM for now but just want to mention that this pretty fragile as we found out other cases.

ruflin · 2018-05-14T11:00:55Z

metricbeat/metricbeat.reference.yml

@@ -251,7 +253,8 @@ metricbeat.modules:

 #-------------------------------- HTTP Module --------------------------------
 - module: http
-  metricsets: ["json"]
+  #metricsets:


Should we in the reference file have the enabled one commented out to make it more clear? I like it in the short config to make sure the defaults keep working but here it's more about documenting it?

ruflin

We are close :-)

ruflin · 2018-05-14T11:01:25Z

metricbeat/module/aerospike/_meta/config.yml

@@ -1,2 +1,4 @@
 - module: aerospike
+#  metricsets: ["namespace"]


should be before metricsets directly for consistency.

ruflin · 2018-05-14T11:03:02Z

metricbeat/module/postgresql/_meta/config.yml

@@ -1,2 +1,21 @@
 - module: postgresql
+  #metricsets:
+  #  # Stats about every PostgreSQL database


I would remove the comments for the metricsets. For these details we have the docs.

ruflin · 2018-05-14T11:03:56Z

metricbeat/module/rabbitmq/_meta/config.yml

  hosts: ["localhost:15672"]
+
+  username: guest


Should this be commented out or is guest a common default?

Yes, guest exists by default.

So it's better to connect by default with guest/guest the have no credentials?

guest/guest can be seen as some kind of "no credentials" for rabbitmq :)
By default it always exists, and I don't know if it is possible to connect with rabbitmq without credentials. I think it'd be good if metricbeat tries to connect with these credentials by default too, so it can collect some metrics out of the box.

SGTM. Not part of this PR but I wonder if we should in these case then also set them directly in the code as defaults.

Yes, good point, I'll take care of this.

Since we split module configurations into modules.d folder, we can keep configurations more verbose, so users have a quick reference when opening them for editing. This change unifies configuration files using the followin rules: * Write at least the following settings, in this order: `module`, `metricsets`, `period`, `hosts`. * Default metricsets list shown as it helps to know what's the list, or disable just one. Inline if there is only one metricset, one line per metricset if there is more than one. * Some modules have exceptions and include more definitions, or different defaults. * Comments are allowed (and encouraged), both to explain a setting and to suggest a full configuration. * `enabled` setting is not shown at all, modules are enabled by using `metricbeat modules enable <module name>`. Use comments to suggest settings that are disabled by default.

exekias added discuss Issue needs further discussion. review Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. v6.3.0 labels Apr 20, 2018

exekias force-pushed the unify-module-confs branch 2 times, most recently from 79b7642 to 69eee1a Compare April 20, 2018 10:55

jsoriano reviewed Apr 20, 2018

View reviewed changes

ruflin reviewed Apr 20, 2018

View reviewed changes

andrewkroh reviewed Apr 20, 2018

View reviewed changes

Carlos Pérez-Aradros Herce added 2 commits April 20, 2018 16:50

Update windows.yml after comments

2581f7e

exekias force-pushed the unify-module-confs branch from 36a3790 to 2581f7e Compare April 20, 2018 14:50

Update system.yml after comments

662dbaa

exekias force-pushed the unify-module-confs branch from 290b591 to ddf9cc9 Compare April 25, 2018 16:43

Carlos Pérez-Aradros Herce added 2 commits April 25, 2018 19:02

Add docs link to module config headers

3ab3e17

Coment metricsets when they are the default

efc0520

exekias force-pushed the unify-module-confs branch from 0b5829c to efc0520 Compare April 25, 2018 17:02

ruflin reviewed Apr 26, 2018

View reviewed changes

Carlos Pérez-Aradros Herce added 2 commits May 7, 2018 19:14

Rework metricsets and comments

a6ed0f3

Update redis config

13b1f0e

ruflin reviewed May 14, 2018

View reviewed changes

jsoriano mentioned this pull request May 15, 2018

Metricbeat: Set guest as default user in RabbitMQ module #7107

Merged

Remove some comments

6f1c0d9

ruflin merged commit 16015b5 into elastic:master May 17, 2018

exekias removed needs_backport PR is waiting to be backported to other branches. v6.3.0 labels Jun 8, 2018

		@@ -1,2 +1,4 @@
		- module: aerospike
		# metricsets: ["namespace"]

Unify default module configurations #6908

Unify default module configurations #6908

Conversation

exekias commented Apr 20, 2018 • edited Loading

exekias commented Apr 20, 2018 • edited Loading

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

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

andrewkroh 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

exekias Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

exekias commented Apr 25, 2018

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

exekias May 9, 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

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

should be before metricsets directly for consistency.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano May 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

exekias commented Apr 20, 2018 •

edited

Loading

exekias commented Apr 20, 2018 •

edited

Loading

exekias Apr 20, 2018 •

edited

Loading

exekias May 9, 2018 •

edited

Loading

jsoriano May 14, 2018 •

edited

Loading