Allow auto-configuration of Metricbeat stack modules for stack monitoring #17609

ycombinator · 2020-04-08T12:34:14Z

What does this PR do?

Metricbeat offers four Elastic stack modules that can be used to collect and index data intended for the Stack Monitoring UI in Kibana: elasticsearch, kibana, logstash, and beat.

Each of these modules' configurations accepts an optional boolean flag, xpack.enabled. By default, this flag is not set, in which case it's default value, false is used. When this flat is set to true, this signals to the modules that they are to be used for the express purpose of collecting data for the Stack Monitoring UI in Kibana. In this case, specific metricsets in the module must be enabled, as the UI expects data collected by these metricsets to function properly.

This PR makes it so that if xpack.enabled: true is set in a stack module's configuration, the required metricsets are automatically enabled in the module's configuration.

Why is it important?

Without such automatic configuration the user must manually configure exactly the required metricsets when xpack.enabled: true is set.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~I have made corresponding changes to the documentation~~
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Closes Auto-configure stack monitoring metricsets when xpack.enabled is set to true #16471

elasticmachine · 2020-04-08T20:17:23Z

Pinging @elastic/integrations-services (Team:Services)

elasticmachine · 2020-04-08T20:17:24Z

Pinging @elastic/stack-monitoring (Stack monitoring)

metricbeat/mb/mb.go

jsoriano

Good job here, it is an interesting approach to use the module builders for this, I sometimes wonder if we should have them, but this proves that they are useful 👍

I have added some suggestions, let me know what you think. I would like to limit the availability of Reconfigure method so it is more clear that is only intended to be used on the base module on module factories.

metricbeat/mb/mb.go

jsoriano · 2020-04-09T09:20:14Z

metricbeat/mb/mb.go

+	UnpackConfig(to interface{}) error
+
+	// ReConfigure reconfigures the module with the new configuration object.
+	ReConfigure(config *common.Config, register *Register) error


Remove it from the Module interface? So this is only allowed in principle where a BaseModule is received, as in module builders.

Sure. I'd like to learn — mostly for my own curiousity — why this difference is significant. The only place I could find in the Metricbeat codebase where mb.Module is implemented but mb.BaseModule is not embedded was here:

beats/metricbeat/mb/module/wrapper.go

Line 53 in 95626b8

type Wrapper struct {

.

Changed in fc45f1d55.

Sure. I'd like to learn — mostly for my own curiousity — why this difference is significant. The only place I could find in the Metricbeat codebase where mb.Module is implemented but mb.BaseModule is not embedded was here:

Well, this is not so significant, but there are two reasons why I would prefer to avoid adding a method to this interface:

mb.Module is something like a "read-only" interface, that can be used to access to information about the module, but not modifying it, so mb.Module can be seen as a way to control visibility. When a method receives a mb.Module it knows that it is only to get information of the module. When a method receives a *mb.BaseModule it has access to all the base module, this can be intended for methods like the module builders, where it can make sense to allow modifications of the module as it is still being built.

mb.Module is a public interface, adding methods to it can break existing code (as happened with the test here). Though I don't think many community beats are implementing their own mb.Modules... so not so important.

metricbeat/mb/mb.go

metricbeat/helper/elastic/elastic.go

metricbeat/mb/testing/modules.go

jsoriano · 2020-04-09T10:12:43Z

metricbeat/helper/elastic/elastic_test.go

@@ -140,3 +142,81 @@ func TestFixTimestampField(t *testing.T) {
 		})
 	}
 }
+
+func TestReconfigureXPackEnabledMetricSets(t *testing.T) {


Suggested change

func TestReconfigureXPackEnabledMetricSets(t *testing.T) {

func TestConfigureModule(t *testing.T) {

If not it looks like we are testing Reconfigure, but there is already a test for that in metricbeat/mb/mb_test.go.

Ah yes, sorry, this is a leftover from previous iterations of the implementation. Good catch, thanks, will fix!

Fixed in 81e1d967a.

metricbeat/helper/elastic/elastic.go

jsoriano · 2020-04-09T10:20:45Z

metricbeat/mb/mb.go

@@ -94,6 +104,44 @@ func (m *BaseModule) UnpackConfig(to interface{}) error {
 	return m.rawConfig.Unpack(to)
 }

+// ReConfigure re-configures the module with the given raw configuration


Maybe add a comment about this method being intended to be used in module factories.

Sure, will do. Again, just to further my own understanding of how Metricbeat modules work, what would be the downside / risk of calling this method from some place other than a module factory?

Added in 168f7e53c.

Again, just to further my own understanding of how Metricbeat modules work, what would be the downside / risk of calling this method from some place other than a module factory?

Metricsets can access the module through base.Module(), one metricset could modify the base module, unexpectedly affecting other metricsets of the same module.

metricbeat/mb/mb.go

metricbeat/helper/elastic/elastic.go

ycombinator · 2020-04-09T17:05:22Z

@jsoriano Thanks for the review. I've addressed all your feedback so this PR is ready for another round of review when you get a chance.

@dedemorton There are docs changes in this PR (see the thread here: #17609 (comment)). So I've requested your review as well.

Thank you both!

dedemorton

Added suggestions to fix copy/paste error.

metricbeat/module/beat/_meta/docs.asciidoc

metricbeat/docs/modules/beat.asciidoc

ycombinator · 2020-04-14T11:58:25Z

Hey @jsoriano @dedemorton this is ready for review when you get a chance. Thanks!

metricbeat/helper/elastic/elastic.go

jsoriano · 2020-04-14T14:06:18Z

metricbeat/docs/modules/beat.asciidoc

-The Beat module contains a minimal set of metrics to enable monitoring of any Beat or other software based on libbeat across
-multiple versions. To monitor more Beat metrics, use our {stack}
-{monitor-features}.
+There are two modules that collect metrics about any Beat or other software based on libbeat:


Not sure if we should talk so explicitly about this as two modules, this can create confusion. In the docs and in the code this is a single module.
The only place where this behaves as two modules is when using metricbeat module, but not everyone uses this approach to configure Metricbeat.
I think we should add something explaining xpack.enabled option, and there we should explain that this option changes the enabled metricsets, for people using these docs as reference config.

Sounds good. In addition to what you've said, I think it might be helpful to also mention under the xpack.enabled option documentation that this option can be configured via a shortcut: metricbeat modules enable <actual_module_name>-xpack. WDYT?

Updated in 9b48420e8.

ycombinator · 2020-04-14T18:57:10Z

metricbeat/docs/modules/beat.asciidoc

+UI in {kib}. To enable this usage, set `xpack.enabled: true` and remove any `metricsets`
+from the module's configuration. Alternatively, run `metricbeat modules disable beat` and
+metricbeat modules enable beat-xpack`.
+


@jsoriano Based on your feedback, I updated the docs for the beat module. WDYT? @dedemorton I'd like your feedback as well.

If/when we're happy with the beat module docs, I'll make similar changes to other stack modules' docs in this PR.

dedemorton

Except for the small nit I point out, the docs look fine.

We are really inconsistent throughout the docs where we refer to modules by name (sometimes uppercase, sometimes lowercase, and sometimes in back ticks. I like the way you've done it here (in back ticks) because it matches what users see in the config file. If you like this approach, I will create a clean up issue for this problem in the docs. Curious to hear what others think.

metricbeat/docs/modules/beat.asciidoc

jsoriano

Thanks!

metricbeat/module/logstash/logstash_test.go

jsoriano · 2020-04-15T11:04:54Z

metricbeat/docs/modules/beat.asciidoc

+UI in {kib}. To enable this usage, set `xpack.enabled: true` and remove any `metricsets`
+from the module's configuration. Alternatively, run `metricbeat modules disable beat` and
+metricbeat modules enable beat-xpack`.
+


metricbeat/helper/elastic/elastic.go

jsoriano · 2020-04-15T11:12:20Z

jenkins, test this again please

jsoriano · 2020-04-15T11:13:32Z

@ycombinator failure in CI is related:

# github.com/elastic/beats/v7/metricbeat/mb [github.com/elastic/beats/v7/metricbeat/mb.test]
mb/mb_test.go:472:31: too many arguments in call to bm.WithConfig
	have (*common.Config, *Register)
	want (common.Config)

Co-Authored-By: DeDe Morton <dede.morton@elastic.co>

dedemorton

LGTM

…ring (#17609) (#17772) * Add ability to reconfigure a module * Reconfigure Logstash module with required metricsets for xpack.enabled * Replace assert with require * Adding CHANGELOG entry * Update default configuration files * Auto-configure beat module metricsets when xpack.enabled = true * Refactoring common code into helper function * Adding tests for ReConfigure() / making it part of Module interface * Moving comments * Fixing infinite recursion 🤦 * Implement for kibana module * Implementing for elasticsearch module * Moving ReConfigure method to BaseModule from Module * Fixing test function name * Use errors.Wrapf * Logging config change * Adding comment about intent of use. * s/ReConfigure/Reconfigure/ * Don't pass registry * Return copy of reconfigured module * Updating module docs to clarify auto-configuration * Fixing test * Trying out docs for `beat` module * Fixing tests * Adding tests for ReConfigure() / making it part of Module interface * Moving comments * Fixing infinite recursion 🤦 * Implement for kibana module * Implementing for elasticsearch module * Moving ReConfigure method to BaseModule from Module * Logging config change * Return copy of reconfigured module * Updating module docs to clarify auto-configuration * Fixing test * Trying out docs for `beat` module * Update metricbeat/docs/modules/beat.asciidoc Co-Authored-By: DeDe Morton <dede.morton@elastic.co> * Uppercasing start of log message * Updating all stack modules' docs * Reodering imports * Fixing rebase error Co-authored-by: DeDe Morton <dede.morton@elastic.co> Co-authored-by: DeDe Morton <dede.morton@elastic.co>

ycombinator force-pushed the mb-stack-auto branch from fa6c19b to 0f14eaa Compare April 8, 2020 13:04

jsoriano self-requested a review April 8, 2020 18:18

ycombinator marked this pull request as ready for review April 8, 2020 20:16

ycombinator added enhancement Metricbeat Metricbeat module needs_backport PR is waiting to be backported to other branches. Feature:Stack Monitoring Team:Services (Deprecated) Label for the former Integrations-Services team v7.8.0 v8.0.0 labels Apr 8, 2020

ycombinator added the [zube]: In Review label Apr 8, 2020

ycombinator commented Apr 9, 2020

View reviewed changes

metricbeat/mb/mb.go Outdated Show resolved Hide resolved

jsoriano reviewed Apr 9, 2020

View reviewed changes

ycombinator requested review from jsoriano and dedemorton April 9, 2020 17:04

dedemorton requested changes Apr 9, 2020

View reviewed changes

metricbeat/module/beat/_meta/docs.asciidoc Outdated Show resolved Hide resolved

metricbeat/docs/modules/beat.asciidoc Outdated Show resolved Hide resolved

ycombinator force-pushed the mb-stack-auto branch from af1dcc5 to c054555 Compare April 14, 2020 13:35

jsoriano reviewed Apr 14, 2020

View reviewed changes

ycombinator force-pushed the mb-stack-auto branch from 3de4b25 to 9b48420 Compare April 14, 2020 18:55

ycombinator commented Apr 14, 2020

View reviewed changes

dedemorton requested changes Apr 14, 2020

View reviewed changes

metricbeat/docs/modules/beat.asciidoc Outdated Show resolved Hide resolved

jsoriano reviewed Apr 15, 2020

View reviewed changes

jsoriano approved these changes Apr 15, 2020

View reviewed changes

ycombinator and others added 13 commits April 15, 2020 06:32

Uppercasing start of log message

ee8ef67

Adding tests for ReConfigure() / making it part of Module interface

de14adf

Moving comments

e2c165f

Fixing infinite recursion 🤦

9003ce4

Implement for kibana module

4f56434

Implementing for elasticsearch module

d85376a

Moving ReConfigure method to BaseModule from Module

c137447

Logging config change

c5806c6

Return copy of reconfigured module

d442118

Updating module docs to clarify auto-configuration

7a2c2cc

Fixing test

22b0abe

Trying out docs for beat module

e2e8431

Update metricbeat/docs/modules/beat.asciidoc

b9c5fd3

Co-Authored-By: DeDe Morton <dede.morton@elastic.co>

ycombinator force-pushed the mb-stack-auto branch from 0ecaad5 to b9c5fd3 Compare April 15, 2020 13:32

ycombinator added 3 commits April 15, 2020 06:41

Updating all stack modules' docs

87a0070

Reodering imports

f21f36a

Fixing rebase error

a0d2bb1

dedemorton approved these changes Apr 16, 2020

View reviewed changes

ycombinator merged commit 8e9c73b into elastic:master Apr 16, 2020

ycombinator deleted the mb-stack-auto branch April 16, 2020 20:17

zube bot added [zube]: Done and removed [zube]: In Review labels Apr 16, 2020

ycombinator mentioned this pull request Apr 16, 2020

[7.x] Allow auto-configuration of Metricbeat stack modules for stack monitoring (#17609) #17772

Merged

ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Apr 16, 2020

jsoriano mentioned this pull request Apr 17, 2020

Add improved config/event output options to windows/perfmon metricset #17596

Merged

9 tasks

andresrc removed the [zube]: Done label Apr 27, 2020

ycombinator mentioned this pull request May 11, 2020

chore: add skeleton for the stack-monitoring tests elastic/e2e-testing#113

Closed

mdelapenya mentioned this pull request May 12, 2020

Pair stack releases with maintenance branches elastic/e2e-testing#119

Closed

ycombinator mentioned this pull request Jun 9, 2020

[DOCS] Adds scope to monitoring elastic/elasticsearch#57852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow auto-configuration of Metricbeat stack modules for stack monitoring #17609

Allow auto-configuration of Metricbeat stack modules for stack monitoring #17609

ycombinator commented Apr 8, 2020 •

edited by zube bot

Loading

elasticmachine commented Apr 8, 2020

elasticmachine commented Apr 8, 2020

jsoriano left a comment

jsoriano Apr 9, 2020

ycombinator Apr 9, 2020

ycombinator Apr 9, 2020

jsoriano Apr 15, 2020 •

edited

Loading

jsoriano Apr 9, 2020

ycombinator Apr 9, 2020

ycombinator Apr 9, 2020

jsoriano Apr 9, 2020

ycombinator Apr 9, 2020

ycombinator Apr 9, 2020

jsoriano Apr 15, 2020

ycombinator commented Apr 9, 2020 •

edited

Loading

dedemorton left a comment

ycombinator commented Apr 14, 2020

jsoriano Apr 14, 2020

ycombinator Apr 14, 2020

ycombinator Apr 14, 2020

ycombinator Apr 14, 2020

jsoriano Apr 15, 2020

dedemorton left a comment

jsoriano left a comment

jsoriano Apr 15, 2020

jsoriano commented Apr 15, 2020

jsoriano commented Apr 15, 2020

dedemorton left a comment

	func TestReconfigureXPackEnabledMetricSets(t *testing.T) {
	func TestConfigureModule(t *testing.T) {

Allow auto-configuration of Metricbeat stack modules for stack monitoring #17609

Allow auto-configuration of Metricbeat stack modules for stack monitoring #17609

Conversation

ycombinator commented Apr 8, 2020 • edited by zube bot Loading

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented Apr 8, 2020

elasticmachine commented Apr 8, 2020

jsoriano 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

jsoriano Apr 15, 2020 • 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

ycombinator commented Apr 9, 2020 • edited Loading

dedemorton left a comment

Choose a reason for hiding this comment

ycombinator commented Apr 14, 2020

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

dedemorton left a comment

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Apr 15, 2020

jsoriano commented Apr 15, 2020

dedemorton left a comment

Choose a reason for hiding this comment

ycombinator commented Apr 8, 2020 •

edited by zube bot

Loading

jsoriano Apr 15, 2020 •

edited

Loading

ycombinator commented Apr 9, 2020 •

edited

Loading