Fixing logic to keep list of unique cluster UUIDs by ycombinator · Pull Request #22808 · elastic/beats

ycombinator · 2020-12-01T07:28:04Z

What does this PR do?

This PR fixes a pretty bad bug with the logstash module when xpack.enabled: true is set.

The logstash module has two metricsets: node and node_stats. When xpack.enabled: true is set, these metricsets emit events for Stack Monitoring with type: logstash_state and type: logstash_stats, respectively.

The node metricset is supposed to emit as many events per Fetch() cycle as there are monitored Logstash pipelines x the number of Elasticsearch clusters that each such pipeline talks to. If a Logstash pipeline does not talk to an Elasticsearch cluster, one event for that pipeline must be emitted.

Instead, due to terribly faulty logic in the code, the node metricset was emitting as many events per Fetch() cycle as there are monitored Logstash pipelines x the number of vertices in each such pipeline!

There was similar faulty logic in the node_stats metricset.

This PR fixes this bug in both metricsets.

Why is it important?

By fixing this bug, we are eliminating a huge source of redundant and unnecessary events being published from the logstash module when xpack.enabled: true is set. This will not only make the module's behavior correct but also drastically reduce memory consumption in Metricbeat and the number of events sent to Metricbeat's output over the wire.

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.

elasticmachine · 2020-12-01T07:28:06Z

Pinging @elastic/stack-monitoring (Stack monitoring)

elasticmachine · 2020-12-01T07:28:06Z

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

elasticmachine · 2020-12-01T07:33:02Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #22808 updated
Start Time: 2020-12-01T12:59:31.896+0000
Duration: 63 min 55 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2293
Skipped	508
Total	2801

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	2293
Skipped	508
Total	2801

ycombinator · 2020-12-01T07:33:14Z

metricbeat/module/logstash/node_stats/data_xpack.go

-		clusterToPipelinesMap[overrideClusterUUID] = pipelines
-		return clusterToPipelinesMap
-	}
-


I removed this section because it's also incorrect. We shouldn't be using the override cluster UUID, if set, for all pipelines so broadly. Instead we should figure out (as we do further below) the cluster UUIDs appropriate for each pipeline and build the cluster UUID => pipelines map that way.

This is still a change in semantics and maybe a bc breaking change.

Before this change all pipelines have had been associated with overrideClusterUUID. With this change pipelines are only associated with overrideClusterUUID iff they do no clusterUUID set. This way overrideClusterUUID actually becomes defaultClusterUUID.

Instead of removing this line, how about printing a deprecation warning if this setting is configured and introduce an a default cluster uuid setting that matches the new semantics?

Hmmm, looking at where the value of overrideClusterUUID comes from ultimately, it comes from the monitoring.cluster_uuid setting in logstash.yml: https://www.elastic.co/guide/en/logstash/current/monitoring-with-metricbeat.html#define-cluster__uuid.

Reading the documentation for that setting:

To bind the metrics of Logstash to a specific cluster, optionally define ...

This makes me think, if this setting is set and therefore overrideClusterUUID is set, we should make that the cluster UUID for all pipelines, ignoring what Elasticsearch clusters each pipeline might individually be talking to.

So I'm going to revert this change and, in fact, add a similar code block in the makeClusterToPipelinesMap function in the node metricset.

andresrc · 2020-12-01T08:14:50Z

/cc @sayden

urso · 2020-12-01T11:09:54Z

metricbeat/module/logstash/node_stats/data_xpack.go

-
 	for _, pipeline := range pipelines {
-		var clusterUUIDs []string
+		clusterUUIDs := make(map[string]struct{}, 0)


this is common.StringSet.

The overall data expension starts by converting the map[string]pipelineState into a []pipelineState. Would it make sense to keep the map instead, to reduce the chance of creating duplicate entries in general?

I've changed the code to use common.StringSet in 4cb337f but I didn't follow this:

The overall data expension starts by converting the map[string]pipelineState into a []pipelineState. Would it make sense to keep the map instead, to reduce the chance of creating duplicate entries in general?

I think it's because I'm not finding the map[string]pipelineState you mentioned. Where are you seeing that?

We do the conversion here.
I was wondering if it is necessary, or maybe can even lead to similar problems (in the future) if we are not careful about not producing duplicates.

urso · 2020-12-01T11:10:27Z

metricbeat/module/logstash/node_stats/data_xpack.go

 		}

-		for _, clusterUUID := range clusterUUIDs {
+		for clusterUUID, _ := range clusterUUIDs {


nit clusterUUID := range clusterUUIDS { ... }

urso · 2020-12-01T11:14:22Z

metricbeat/module/logstash/node_stats/data_xpack.go

-		clusterToPipelinesMap[overrideClusterUUID] = pipelines
-		return clusterToPipelinesMap
-	}
-


This is still a change in semantics and maybe a bc breaking change.

Before this change all pipelines have had been associated with overrideClusterUUID. With this change pipelines are only associated with overrideClusterUUID iff they do no clusterUUID set. This way overrideClusterUUID actually becomes defaultClusterUUID.

Instead of removing this line, how about printing a deprecation warning if this setting is configured and introduce an a default cluster uuid setting that matches the new semantics?

urso · 2020-12-01T11:23:32Z

metricbeat/module/logstash/node_stats/data_xpack_test.go

+				},
+			},
+		},
+	}


Is this particular case able to reproduce the issue we have had? If not, we should add one.

The expannsion into the cluster map is somewhat critical in order to produce correct docs, that show the correct association of configurations to single Elasticsearch clusters. Can we make this test more exhaustive?

Yes, this is the case that was problematic. Before this PR this case would've produced 3 PipelineState associated with the prod_cluster_id cluster UUID when we should've only got 1 PipelineState.

I will expand this test to add a few more test cases.

Added more test cases in 672ee5e.

urso · 2020-12-01T11:24:02Z

metricbeat/module/logstash/node/data_xpack.go


 	for _, pipeline := range pipelines {
-		var clusterUUIDs []string
+		clusterUUIDs := make(map[string]struct{}, 0)


common.StringSet

… if set

ycombinator · 2020-12-01T12:26:20Z

@urso I've addressed all your feedback. Would you mind re-reviewing please, when you get a chance? Thanks!

urso · 2020-12-01T12:51:44Z

metricbeat/module/logstash/node/data_xpack.go

 		}

-		for _, clusterUUID := range clusterUUIDs {
+		for _, clusterUUID := range clusterUUIDs.ToSlice() {


The ToSlice is a little overkill here (it copies and sorts all strings). The StringSet is defined as type StringSet map[string]struct{} and can be directly use with the range-clause.

Removed in dcfce4d.

#22808) (#22817) * Fixing logic to keep list of unique cluster UUIDs (#22808) * Fixing logic to keep list of unique cluster UUIDs * Adding CHANGELOG entry * Use common.StringSet * Adding more test cases * Adding back logic to broadly override cluster UUID for all pipelines, if set * Removing ToSlice() * Fixing loop * Fixing up CHANGELOG

* Fixing logic to keep list of unique cluster UUIDs * Adding CHANGELOG entry * Use common.StringSet * Adding more test cases * Adding back logic to broadly override cluster UUID for all pipelines, if set * Removing ToSlice() * Fixing loop

…-issues * upstream/master: (41 commits) Fix version parser regex for packaging (elastic#22581) Fix local_dynamic documentation and add providers inline doc. (elastic#22657) fix: use proper param name for e2e tests (elastic#22836) [Heartbeat] Fix exit on disabled monitor (elastic#22829) Update Golang to 1.14.12 (elastic#22790) docs: fix setup.template.overwrite typos (elastic#22804) Add docs section for ECS EC2 monitoring (elastic#22784) Fixing logic to keep list of unique cluster UUIDs (elastic#22808) Skip somewhat flaky UDP system test on Windows (elastic#22810) Fix polling node when it is not ready and monitor by hostname (elastic#22666) Skip Filebeat test_shutdown on windows 7 (elastic#22797) Make monitoring Namespace thread-safe (elastic#22640) Drop pkt_dstaddr and pkt_srcaddr when equals to "-" (elastic#22721) Add support for reading from UNIX datagram sockets (elastic#22699) Fix export dashboard command from Elastic Cloud (elastic#22746) Skip flaky winlogbeat test on Windows-7 (elastic#22754) Missing `>` (elastic#22763) (elastic#22766) Fix k8s watcher issue when node access to list nodes and ns (elastic#22714) [Metricbeat/Kibana/stats] Enforce `exclude_usage=true` (elastic#22732) Avoid sending non-numeric floats in cloud foundry integrations (elastic#22634) ...

…dows-7 * upstream/master: (41 commits) Fix version parser regex for packaging (elastic#22581) Fix local_dynamic documentation and add providers inline doc. (elastic#22657) fix: use proper param name for e2e tests (elastic#22836) [Heartbeat] Fix exit on disabled monitor (elastic#22829) Update Golang to 1.14.12 (elastic#22790) docs: fix setup.template.overwrite typos (elastic#22804) Add docs section for ECS EC2 monitoring (elastic#22784) Fixing logic to keep list of unique cluster UUIDs (elastic#22808) Skip somewhat flaky UDP system test on Windows (elastic#22810) Fix polling node when it is not ready and monitor by hostname (elastic#22666) Skip Filebeat test_shutdown on windows 7 (elastic#22797) Make monitoring Namespace thread-safe (elastic#22640) Drop pkt_dstaddr and pkt_srcaddr when equals to "-" (elastic#22721) Add support for reading from UNIX datagram sockets (elastic#22699) Fix export dashboard command from Elastic Cloud (elastic#22746) Skip flaky winlogbeat test on Windows-7 (elastic#22754) Missing `>` (elastic#22763) (elastic#22766) Fix k8s watcher issue when node access to list nodes and ns (elastic#22714) [Metricbeat/Kibana/stats] Enforce `exclude_usage=true` (elastic#22732) Avoid sending non-numeric floats in cloud foundry integrations (elastic#22634) ...

Fixing logic to keep list of unique cluster UUIDs

d885578

ycombinator added bug module Metricbeat Metricbeat Feature:Stack Monitoring Team:Services (Deprecated) Label for the former Integrations-Services team v7.11.0 v7.10.1 labels Dec 1, 2020

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Dec 1, 2020

Adding CHANGELOG entry

873c3fd

ycombinator requested a review from faec December 1, 2020 07:31

ycombinator commented Dec 1, 2020

View reviewed changes

andresrc assigned ycombinator Dec 1, 2020

andresrc requested a review from sayden December 1, 2020 09:18

urso reviewed Dec 1, 2020

View reviewed changes

ycombinator added 3 commits December 1, 2020 17:06

Use common.StringSet

4cb337f

Adding more test cases

672ee5e

Adding back logic to broadly override cluster UUID for all pipelines,…

bf166b6

… if set

ycombinator requested a review from urso December 1, 2020 12:26

urso reviewed Dec 1, 2020

View reviewed changes

urso approved these changes Dec 1, 2020

View reviewed changes

ycombinator added 2 commits December 1, 2020 18:26

Removing ToSlice()

dcfce4d

Fixing loop

ee7a963

ycombinator merged commit cddf4d4 into elastic:master Dec 1, 2020

ycombinator deleted the mon-ls-dup-cluster-uuid branch December 1, 2020 14:05

ycombinator mentioned this pull request Dec 1, 2020

Cherry-pick to 7.x: Fixing logic to keep list of unique cluster UUIDs (#22808) #22816

Merged

ycombinator added the needs_backport PR is waiting to be backported to other branches. label Dec 1, 2020

ycombinator mentioned this pull request Dec 1, 2020

Cherry-pick to 7.10: Fixing logic to keep list of unique cluster UUIDs (#22808) #22817

Merged

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

Conversation

ycombinator commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

Uh oh!

elasticmachine commented Dec 1, 2020

Uh oh!

elasticmachine commented Dec 1, 2020

Uh oh!

elasticmachine commented Dec 1, 2020 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

Test stats 🧪

Uh oh!

ycombinator Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresrc commented Dec 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urso Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator commented Dec 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ycombinator commented Dec 1, 2020 •

edited

Loading

elasticmachine commented Dec 1, 2020 •

edited by jenkins-beats-ci bot

Loading

ycombinator Dec 1, 2020 •

edited

Loading

urso Dec 1, 2020 •

edited

Loading