Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Module configuration variants #9118

Merged
merged 14 commits into from
Apr 4, 2019

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Nov 15, 2018

Resolves #8969.

This PR introduces the concept of a module configuration variant, as detailed in #8969. This works with filebeat as well as metricbeat modules.

Usage examples

Listing modules and their variants

$ ./metricbeat modules list
Enabled:
system

Disabled:
...
docker
dropwizard
elasticsearch
elasticsearch:xpack
envoyproxy
etcd
...
$ ls -l  modules.d/elasticsearch*
-rw-r--r--  1 shaunak  staff  402 Nov 15 16:39 modules.d/elasticsearch.xpack.yml.disabled
-rw-r--r--  1 shaunak  staff  286 Nov 15 16:39 modules.d/elasticsearch.yml.disabled

Enabling a module variant configuration

$ ./metricbeat modules enable elasticsearch:xpack
Enabled elasticsearch:xpack
$ ls -l  modules.d/elasticsearch*
-rw-r--r--  1 shaunak  staff  402 Nov 15 16:39 modules.d/elasticsearch.xpack.yml
-rw-r--r--  1 shaunak  staff  286 Nov 15 16:39 modules.d/elasticsearch.yml.disabled

Source config files for a module

ls -l module/elasticsearch/_meta/config*
-rw-r--r--  1 shaunak  staff  377 Nov 15 11:17 module/elasticsearch/_meta/config.reference.yml
-rw-r--r--  1 shaunak  staff  276 Nov 15 11:34 module/elasticsearch/_meta/config.xpack.yml
-rw-r--r--  1 shaunak  staff  160 Oct 25 14:26 module/elasticsearch/_meta/config.yml

@ycombinator
Copy link
Contributor Author

I still need to write tests, docs and a changelog entry for the functionality in this PR but just wanted to put it out there and get some early feedback.

filebeat/Makefile Outdated Show resolved Hide resolved
script/modules_collector.py Outdated Show resolved Hide resolved
script/modules_collector.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kvch kvch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please port the Python script to Golang.

@ycombinator ycombinator changed the title [WIP] Module configuration variants Module configuration variants Nov 16, 2018
@ycombinator
Copy link
Contributor Author

@jsoriano and @kvch Thanks for your LGTMs. I've updated this PR with unit tests and CHANGELOG entries. Additionally, given that Filebeat now uses the same module config collection script as Metricbeat, the Filebeat module configuration files now each contain a header with a link to the module's documentation. Given these changes, you might want to re-review this PR.

@dedemorton I could use your guidance on where best to document the functionality introduced by this PR. Somewhere in https://www.elastic.co/guide/en/beats/filebeat/current/configuration-filebeat-modules.html seems appropriate to me but I'll defer to your opinion.

Everyone: Let's now have the naming discussion 😄. Are we okay calling these alternative out-of-the-box module configurations as "variants"? Or do folks like "flavors" better as used by @jsoriano and @ruflin in #8969? Or something else?

@ruflin
Copy link
Contributor

ruflin commented Nov 19, 2018

Could we split up this PR in two parts? 1 that moves the python files + adds new headers to config files and a second one that does the actual changes with flavors etc? Like this the flavor / variants PR will be much smaller.

For the naming: There is also a discussion around flavor for datasets. Meaning it can have different inputs but the output data structure will be identical. Here I think flavor fits well. For the module I'm asking myself if it the same thing? The x-pack one is a bit special as it creates a completely different data structure. What is a flavor of a module in a more generic case? It is just the same module with different default config options?

@ycombinator
Copy link
Contributor Author

Could we split up this PR in two parts? 1 that moves the python files + adds new headers to config files and a second one that does the actual changes with flavors etc? Like this the flavor / variants PR will be much smaller.

I've made a new PR for just the python file reuse + adding new headers: #9150. And I have rebased this PR (#9118) on top of the new one (#9150). Once the new one is reviewed and merged, I will rebase this PR on master so it becomes smaller.

@ycombinator ycombinator force-pushed the metricbeat-module-variants branch from 030c06f to 935fc53 Compare November 19, 2018 15:25
ycombinator added a commit that referenced this pull request Nov 19, 2018
…#9150)

This PR takes the module configuration collector script from Metricbeat and reuses it from Filebeat as well. A result of this is that all Filebeat module configs now have a header pointing to module docs, just like the Metricbeat counterparts.

This PR is in preparation for #9118.
@ycombinator ycombinator force-pushed the metricbeat-module-variants branch from 935fc53 to 1c3b4e8 Compare November 19, 2018 16:51
@ycombinator
Copy link
Contributor Author

#9150 has been merged now so I've rebased this PR on master. It now only contains the changes for implementing module configuration variants.

@dedemorton
Copy link
Contributor

dedemorton commented Nov 19, 2018

@ycombinator

I could use your guidance on where best to document the functionality introduced by this PR. Somewhere in https://www.elastic.co/guide/en/beats/filebeat/current/configuration-filebeat-modules.html seems appropriate to me but I'll defer to your opinion.

That seems like the right place, but we need to make sure we show the syntax in other places where we show the enable command.

How common will the variants/flavors be? If they are likely to be fairly common (on most/all modules), maybe we should document the variant as part of the main syntax: filebeat modules enable apache2:variant. We'd mention that variant is optional. If no variant is specified, Filebeat uses the the default configuration.

I suspect that a lot users don't read the topic about specifying which modules to run. They see command examples in other places (like the getting started and the docs for each module), so we need to make sure they see the new syntax there, too.

Re naming:

IMO, the word "variant" isn't great, but I don't have a better suggestion. I do not like "flavor" very much either. :-P If you compare the actual definitions of the two terms, I think "variant" is more accurate:

variant - a form or version of something that differs in some respect from other forms of the same thing or from a standard.

flavor - an indication of the essential character of something.

@ycombinator
Copy link
Contributor Author

ycombinator commented Nov 20, 2018

@dedemorton

That seems like the right place, but we need to make sure we show the syntax in other places where we show the enable command.

I've added a section on configuration variants to that page in 501c8b6. Please take a look when you get a chance.

How common will the variants/flavors be? If they are likely to be fairly common (on most/all modules), maybe we should document the variant as part of the main syntax: filebeat modules enable apache2:variant. We'd mention that variant is optional. If no variant is specified, Filebeat uses the the default configuration.

I suspect that a lot users don't read the topic about specifying which modules to run. They see command examples in other places (like the getting started and the docs for each module), so we need to make sure they see the new syntax there, too.

I don't expect module configuration variants to be very common, at least initially. The specific use case that prompted this feature was for X-Pack Monitoring with Metricbeat so we'll make sure to add the variant syntax to those docs. (cc: @lcawl - I'll put up the PRs but just so you have context for them, this is it). Besides that, let's document the variant for a module when we introduce it, i.e. in follow up PRs to this one.

Re naming:

IMO, the word "variant" isn't great, but I don't have a better suggestion. I do not like "flavor" very much either. :-P If you compare the actual definitions of the two terms, I think "variant" is more accurate:

variant - a form or version of something that differs in some respect from other forms of the same thing or from a standard.

flavor - an indication of the essential character of something.

Naming is hard :) Unless someone feels strongly about the name, I'm inclined to go with "variants" given your finding that it's more accurate.

@ruflin I saw your note about "flavor" above but I wasn't sure if you were advocating for or against it as compared to "variant". Putting aside the case of the xpack variant (as it's a bit special), other examples that @jsoriano came up with were system:raid or kubernetes:events, as these sections are completely commented out in the respective module configuration files currently.

@@ -182,3 +189,40 @@ func (g *GlobManager) Disable(name string) error {

return errors.Errorf("module %s not found", name)
}

// DisplayName returns the config file's display name
func (f *CfgFile) DisplayName() string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call it String? I think it is more common to use functions named String to get the string representation of a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kvch Made this change in 9ba27c7.

@ycombinator ycombinator force-pushed the metricbeat-module-variants branch from d1f6464 to 6a3cd0a Compare April 4, 2019 12:59
@ycombinator
Copy link
Contributor Author

jenkins, test this

@ycombinator ycombinator merged commit 22c4970 into elastic:master Apr 4, 2019
@ycombinator ycombinator added v7.2.0 v8.0.0 needs_backport PR is waiting to be backported to other branches. labels Apr 4, 2019
ycombinator added a commit to ycombinator/kibana that referenced this pull request Apr 8, 2019
…lastic#34599)

Now that elastic/beats#9118 is merged, starting 7.1 users will be able configure Metricbeat for monitoring Kibana instances using a simpler syntax.

Previously, users would have to run `metricbeat modules enable kibana` to enable the `kibana` Metricbeat module, then configure the module for Stack Monitoring by manually editing `modules.d/kibana.yml`. Going forward, users will be able to achieve the same effect by running `metricbeat modules enable kibana-xpack`.

This PR updates the docs with this change.

Related: elastic/elasticsearch#40879
ycombinator added a commit to elastic/kibana that referenced this pull request Apr 8, 2019
…34599)

Now that elastic/beats#9118 is merged, starting 7.1 users will be able configure Metricbeat for monitoring Kibana instances using a simpler syntax.

Previously, users would have to run `metricbeat modules enable kibana` to enable the `kibana` Metricbeat module, then configure the module for Stack Monitoring by manually editing `modules.d/kibana.yml`. Going forward, users will be able to achieve the same effect by running `metricbeat modules enable kibana-xpack`.

This PR updates the docs with this change.

Related: elastic/elasticsearch#40879
ycombinator added a commit to elastic/elasticsearch that referenced this pull request Apr 8, 2019
…40879)

Now that elastic/beats#9118 is merged, starting 7.1 users will be able configure Metricbeat for monitoring Elasticsearch nodes using a simpler syntax.

Previously, users would have to run `metricbeat modules enable elasticsearch` to enable the `elasticsearch` Metricbeat module, then configure the module for Stack Monitoring by manually editing `modules.d/elasticsearch.yml`. Going forward, users will be able to achieve the same effect by running `metricbeat modules enable elasticsearch-xpack`.

This PR updates the docs with this change.

Related: elastic/kibana#34599
ycombinator added a commit to ycombinator/elasticsearch that referenced this pull request Apr 8, 2019
…lastic#40879)

Now that elastic/beats#9118 is merged, starting 7.1 users will be able configure Metricbeat for monitoring Elasticsearch nodes using a simpler syntax.

Previously, users would have to run `metricbeat modules enable elasticsearch` to enable the `elasticsearch` Metricbeat module, then configure the module for Stack Monitoring by manually editing `modules.d/elasticsearch.yml`. Going forward, users will be able to achieve the same effect by running `metricbeat modules enable elasticsearch-xpack`.

This PR updates the docs with this change.

Related: elastic/kibana#34599
ycombinator added a commit to elastic/kibana that referenced this pull request Apr 8, 2019
…34599) (#34726)

Now that elastic/beats#9118 is merged, starting 7.1 users will be able configure Metricbeat for monitoring Kibana instances using a simpler syntax.

Previously, users would have to run `metricbeat modules enable kibana` to enable the `kibana` Metricbeat module, then configure the module for Stack Monitoring by manually editing `modules.d/kibana.yml`. Going forward, users will be able to achieve the same effect by running `metricbeat modules enable kibana-xpack`.

This PR updates the docs with this change.

Related: elastic/elasticsearch#40879
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
…lastic#40879)

Now that elastic/beats#9118 is merged, starting 7.1 users will be able configure Metricbeat for monitoring Elasticsearch nodes using a simpler syntax.

Previously, users would have to run `metricbeat modules enable elasticsearch` to enable the `elasticsearch` Metricbeat module, then configure the module for Stack Monitoring by manually editing `modules.d/elasticsearch.yml`. Going forward, users will be able to achieve the same effect by running `metricbeat modules enable elasticsearch-xpack`.

This PR updates the docs with this change.

Related: elastic/kibana#34599
@ycombinator ycombinator deleted the metricbeat-module-variants branch December 25, 2019 11:17
@ycombinator ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Module configuration variants
8 participants