Fail gracefully if X-Pack monitoring is enabled #6627

ruflin · 2018-03-22T10:07:09Z

X-Pack Monitoring feature keeps retrying to connect to Elasticsearch until either Elasticsearch is available and the Monitoring is enabled. Before each time a connection attempted failed an errors was logged. This PR changes the implementation that only an info is printed about not being able to connect on the first attempt. After that reporting will keep retrying to connect to Elasticsearch until it is successful, but no errors are logged. On success an message is logged that the connection was successful.

If for whatever reason X-Pack was available but at a later point isn't anymore, Monitoring will log errors as this is not an expected case.

Note: In Elasticsearch 6.2 Monitoring as an interval parameter for the collection. If it is set to -1 collection is disabled but events will still be written to Elasticsearch. The issue with this in 6.2 is that no cleanup of the sent events happen. In 6.3 there is a collection config option to turn collection off even if monitoring is enabled. If collection is turned off the bulk requests are still accepted and return a 200, but that will not be persisted. The Beat on startup only checks if Monitoring is installed. If collection is enabled or not is a setting internal to monitoring and monitoring is expected to do the right thing with the events here.

This PR has been manually tested against 6.2 but no automated tests were implemented yet.

kvch · 2018-03-22T10:41:15Z

libbeat/cmd/instance/beat.go

+	}{}
+	err = json.Unmarshal(body, &defaults)
+	if err != nil {
+		logp.Debug("monitoring", "Error decoding xpack monitoring defaults from Elasticsearch: %s", err)


%s/xpack monitoring/X-Pack Monitoring/

kvch · 2018-03-22T10:42:26Z

libbeat/cmd/instance/beat.go

+func (b *Beat) checkXPackMonitoring() bool {
+
+	if b.Config.Output.Name() != "elasticsearch" {
+		logp.Info("Disabling X-Pack monitoring as output is not Elasticsearch.")


%s/monitoring/Monitoring

kvch · 2018-03-22T10:42:51Z

libbeat/cmd/instance/beat.go

+	esConfig := b.Config.Output.Config()
+	esClient, err := elasticsearch.NewConnectedClient(esConfig)
+	if err != nil {
+		logp.Info("Disabling X-Pack monitoring as connection to Elasticsearch could not be established.")


%s/monitoring/Monitoring/

I think the user will want to know what the error was.

ruflin · 2018-03-22T13:08:29Z

I set to in progress again as I haven't managed to test it yet with X-Pack installed and ingestion disabled.

andrewkroh · 2018-03-22T13:22:38Z

libbeat/cmd/instance/beat.go

+	esConfig := b.Config.Output.Config()
+	esClient, err := elasticsearch.NewConnectedClient(esConfig)
+	if err != nil {
+		logp.Info("Disabling X-Pack monitoring as connection to Elasticsearch could not be established.")


I think the user will want to know what the error was.

andrewkroh · 2018-03-22T13:22:41Z

CHANGELOG.asciidoc

@@ -199,6 +199,7 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di
 - Add appender support to autodiscover {pull}6469[6469]
 - Add add_host_metadata processor {pull}5968[5968]
 - Retry configuration to load dashboards if Kibana is not reachable when the beat starts. {pull}6560[6560]
+- Enabled X-Pack monitoring by default but fail gracefully.


... if X-Pack Monitoring is not enabled.

ruflin · 2018-03-26T12:16:14Z

@kvch @andrewkroh Review comments applied. But please have a second look. The implementation was simplified a bit was decided not to check the collection part (see updated PR message).

Ready for an other round of reviews.

kvch · 2018-03-26T12:39:25Z

libbeat/cmd/instance/beat.go

+	}{}
+	err = json.Unmarshal(body, &features)
+	if err != nil {
+		logp.Debug("monitoring", "Error fetching monitoring enabled feature from Elasticsearch: %s", err)


Shouln't this be logged at error level?

The idea here was that in all cases where something strange comes back from ES because of Monitoring, not errors are logged. When reading this review I realised there should be an additional error check on line 339 for the error there.

andrewkroh

The code looks fine. I'm just thinking it may belong in a different location.

andrewkroh · 2018-03-26T16:04:58Z

libbeat/cmd/instance/beat.go

-		reporter, err := report.New(b.Info, b.Config.Monitoring, b.Config.Output)
-		if err != nil {
-			return err
+		reportingActive := b.checkXPackMonitoring()


It seems like this logic is integral to the Elasticsearch monitoring output. So why not have all of the related code to check if x-pack monitoring is enabled part of the same package?

Looking into this I realised we already have an almost identical code here: https://github.com/elastic/beats/blob/master/libbeat/monitoring/report/elasticsearch/client.go#L41 I need to revisit the implementation to see how the two can be combined.

X-Pack Monitoring feature keeps retrying to connect to Elasticsearch until either Elasticsearch is available and the Monitoring is enabled. Before each time a connection attempted failed an errors was logged. This PR changes the implementation that only an info is printed about not being able to connect on the first attempt. After that reporting will keep retrying to connect to Elasticsearch until it is successful, but no errors are logged. On success an message is logged that the connection was successful. If for whatever reason X-Pack was available but at a later point isn't anymore, Monitoring will log errors as this is not an expected case. Note: In Elasticsearch 6.2 Monitoring as an interval parameter for the collection. If it is set to `-1` collection is disabled but events will still be written to Elasticsearch. The issue with this in 6.2 is that no cleanup of the sent events happen. In 6.3 there is a collection config option to turn collection off even if monitoring is enabled. If collection is turned off the bulk requests are still accepted and return a 200, but that will not be persisted. The Beat on startup only checks if Monitoring is installed. If collection is enabled or not is a setting internal to monitoring and monitoring is expected to do the right thing with the events here. This PR has been manually tested against 6.2 but no automated tests were implemented yet.

ruflin · 2018-04-09T12:27:07Z

@kvch @andrewkroh The PR is completely rewritten. Please have an other look and have a look again through the PR description if that is also the behaviour you expect.

andrewkroh

LGTM

ruflin · 2018-04-10T07:37:08Z

@andrewkroh in progress label removed. Can be merged.

ruflin added review libbeat labels Mar 22, 2018

kvch reviewed Mar 22, 2018

View reviewed changes

ruflin added the in progress Pull request is currently in progress. label Mar 22, 2018

andrewkroh reviewed Mar 22, 2018

View reviewed changes

ruflin force-pushed the monitoring-check branch from 7da4dfb to 6a53a67 Compare March 26, 2018 12:15

ruflin added monitoring and removed in progress Pull request is currently in progress. labels Mar 26, 2018

kvch reviewed Mar 26, 2018

View reviewed changes

ruflin force-pushed the monitoring-check branch from 34549d3 to a4e43a5 Compare March 26, 2018 14:12

andrewkroh approved these changes Mar 26, 2018

View reviewed changes

ruflin added the in progress Pull request is currently in progress. label Mar 27, 2018

ruflin force-pushed the monitoring-check branch from a4e43a5 to f9d0465 Compare April 9, 2018 12:24

ruflin force-pushed the monitoring-check branch from f9d0465 to cce0784 Compare April 9, 2018 12:26

andrewkroh approved these changes Apr 9, 2018

View reviewed changes

ruflin removed the in progress Pull request is currently in progress. label Apr 10, 2018

kvch merged commit 80e6f72 into elastic:master Apr 10, 2018

ph mentioned this pull request Aug 24, 2018

How to disable xpack check in Filebeat #8086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail gracefully if X-Pack monitoring is enabled #6627

Fail gracefully if X-Pack monitoring is enabled #6627

ruflin commented Mar 22, 2018 •

edited

Loading

kvch Mar 22, 2018

kvch Mar 22, 2018 •

edited

Loading

kvch Mar 22, 2018

andrewkroh Mar 22, 2018

ruflin commented Mar 22, 2018

andrewkroh Mar 22, 2018

andrewkroh Mar 22, 2018

ruflin commented Mar 26, 2018

kvch Mar 26, 2018

ruflin Mar 26, 2018

andrewkroh left a comment

andrewkroh Mar 26, 2018

ruflin Mar 27, 2018

ruflin commented Apr 9, 2018

andrewkroh left a comment

ruflin commented Apr 10, 2018

Fail gracefully if X-Pack monitoring is enabled #6627

Fail gracefully if X-Pack monitoring is enabled #6627

Conversation

ruflin commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

kvch Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Mar 26, 2018

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

ruflin commented Apr 9, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

ruflin commented Apr 10, 2018

ruflin commented Mar 22, 2018 •

edited

Loading

kvch Mar 22, 2018 •

edited

Loading