Add Kubernetes StatefulSet metricset #6236

fassisrosa · 2018-01-31T01:47:52Z

Added support for Kubernetes StatefulSet.

Conflicts: metricbeat/docs/modules_list.asciidoc

elasticmachine · 2018-01-31T01:47:53Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

houndci-bot · 2018-01-31T01:48:00Z

metricbeat/module/kubernetes/state_statefulset/state_statefulset_test.go

@@ -0,0 +1,111 @@
+// +build !integration
+
+package state_statefulset


don't use an underscore in package name

houndci-bot · 2018-01-31T01:48:01Z

metricbeat/module/kubernetes/state_statefulset/state_statefulset.go

@@ -0,0 +1,62 @@
+package state_statefulset


don't use an underscore in package name

houndci-bot · 2018-01-31T01:48:01Z

metricbeat/module/kubernetes/state_statefulset/data.go

+				continue
+			}
+			namespace := util.GetLabel(metric, "namespace")
+			statefulset_key := namespace + "::" + statefulset


don't use underscores in Go names; var statefulset_key should be statefulsetKey

I think the bot comment is valid here 😇

Now I see this, it seems the rest of state_* may miss the namespacing you did here, I'm glad you found this!

I was going to mention it after this pull request. Worth fixing others no doubt. :)

houndci-bot · 2018-01-31T01:48:01Z

metricbeat/module/kubernetes/state_statefulset/data.go

@@ -0,0 +1,52 @@
+package state_statefulset


don't use an underscore in package name

Following same pattern as other classes.

yes, please ignore the bot on this.

fassisrosa · 2018-01-31T01:51:50Z

Added unit testing for StatefulSet support. Made StatefulSet be relative to namespace (i.e. you can have two statefulsets with same name belonging to different namespaces).

fassisrosa · 2018-01-31T01:58:19Z

I signed CLA already.

ruflin · 2018-01-31T02:00:58Z

@fassisrosa Thanks for you contribution. Haven't look at the code yet but it for sure will need an entry in the CHANGELOG :-)

fassisrosa · 2018-01-31T03:12:10Z

Added entry to CHANGELOG.

exekias

Awesome contribution, thank you @fassisrosa 🎉, StatefulSets are a great addition! I left some minor comments 😄

exekias · 2018-01-31T09:54:26Z

metricbeat/module/kubernetes/state_statefulset/data.go

+				continue
+			}
+			namespace := util.GetLabel(metric, "namespace")
+			statefulset_key := namespace + "::" + statefulset


I think the bot comment is valid here 😇

Now I see this, it seems the rest of state_* may miss the namespacing you did here, I'm glad you found this!

exekias · 2018-01-31T09:55:16Z

metricbeat/module/kubernetes/state_statefulset/state_statefulset.go

+// Part of new is also setting up the configuration by processing additional
+// configuration entries if needed.
+func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
+	cfgwarn.Beta("The kubernetes state_statefulset metricset is beta")


You can remove this message, kubernetes module has been promoted to GA

Fixed bot comment.

exekias · 2018-01-31T09:57:08Z

jenkins, test it please

exekias · 2018-01-31T09:58:37Z

metricbeat/module/kubernetes/state_statefulset/state_statefulset_test.go

+	}
+
+	if len(testCases) > 0 {
+		t.Error("Test reference events not found: %v", testCases)


This should be Errorf

walktall · 2018-01-31T10:01:33Z

metricbeat/module/kubernetes/state_statefulset/data.go

+		}
+	}
+
+	var events []common.MapStr


Maybe you could pre allocate events by events := make([]common.MapStr, 0, len(eventsMap))

fassisrosa · 2018-02-01T02:00:38Z

Any idea what this Travis CI build fail is all about? Looks like a code convention violation but have no idea about the test’s expectations...

walktall · 2018-02-01T02:06:58Z

@fassisrosa You should obey the the go import style. First section with std pkgs, second is community pkgs such as github.com, third section is project pkg in this case is pkg under beats project.

fassisrosa · 2018-02-01T02:17:04Z

Understood, I think... but then how did state_replicaset_test.go pass checks? Tried to follow same convention.

Just to confirm, issue is in state_statefulset_test.go imports should be in correct order:

 "net/http"
 "net/http/httptest"
 "os"
 "testing"

 "github.com/stretchr/testify/assert"

 "github.com/elastic/beats/libbeat/common"
 mbtest "github.com/elastic/beats/metricbeat/mb/testing"

Is this the correct fix?

Thanks for your patience, this is my first commit to beats, trying to understand the rules for submissions :)

exekias · 2018-02-01T11:52:34Z

That looks correct, you can use make fmt to automatically fix formatting

fassisrosa · 2018-02-01T12:29:44Z

Thanks @exekias , made changes, ran fmt, pushed.

exekias

Waiting for green

exekias · 2018-02-01T12:53:37Z

ok to test

exekias · 2018-02-01T12:55:02Z

metricbeat/module/kubernetes/state_statefulset/state_statefulset.go

+func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
+	return &MetricSet{
+		BaseMetricSet: base,
+		prometheus:    helper.NewPrometheusClient(base),


This line is failing in the CI, NewPrometheusClient can return an error

To put some context, this changed very recently: #6205, probably your working copy doesn't have the change

…etes_metrics

exekias · 2018-02-02T08:56:06Z

Thank you so much for this great addition @fassisrosa 🎉!!

fassisrosa · 2018-02-03T03:17:47Z

Thanks so much for the review comments. Love metricbeat, using it all the time! :)

exekias · 2018-02-05T15:27:43Z

I've opened #6281 to fix the namespacing issue in the rest of state_ metricsets, plan to take it next week, let me know if you want to take a stab at it 😸

fassisrosa · 2018-02-06T04:01:04Z

Taking a stab at it... Pull request above. :)

Francisco Assis Rosa added 3 commits December 14, 2017 18:53

metricbeat: add support for StatefulSet reporting in Kubernetes module

16d57a1

Merge branch 'master' into kubernetes_metrics

265b892

Conflicts: metricbeat/docs/modules_list.asciidoc

metricbeat: add support for StatefulSet reporting in Kubernetes module

6a46aba

houndci-bot reviewed Jan 31, 2018

View reviewed changes

ruflin added module review Metricbeat Metricbeat labels Jan 31, 2018

ruflin requested a review from exekias January 31, 2018 01:59

metricbeat: add support for StatefulSet reporting in Kubernetes module

1f7be96

exekias reviewed Jan 31, 2018

View reviewed changes

exekias changed the title ~~Kubernetes metrics~~ Add Kubernetes StatefulSet metricset Jan 31, 2018

walktall reviewed Jan 31, 2018

View reviewed changes

metricbeat: add support for StatefulSet reporting in Kubernetes module

99eb905

metricbeat: add support for StatefulSet reporting in Kubernetes module

7e06452

metricbeat: add support for StatefulSet reporting in Kubernetes module

77d182c

exekias approved these changes Feb 1, 2018

View reviewed changes

exekias reviewed Feb 1, 2018

View reviewed changes

Francisco Assis Rosa added 2 commits February 1, 2018 14:27

Merge branch 'master' of https://github.com/elastic/beats into kubern…

17f1ef4

…etes_metrics

metricbeat: add support for StatefulSet reporting in Kubernetes module

5a397e5

exekias merged commit 4cbb1de into elastic:master Feb 2, 2018

fassisrosa mentioned this pull request Feb 6, 2018

Kubernetes state_* metricsets are ignoring namespace #6281 #6294

Merged

		@@ -0,0 +1,111 @@
		// +build !integration

		package state_statefulset

Add Kubernetes StatefulSet metricset #6236

Add Kubernetes StatefulSet metricset #6236

Conversation

fassisrosa commented Jan 31, 2018

elasticmachine commented Jan 31, 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

fassisrosa commented Jan 31, 2018

fassisrosa commented Jan 31, 2018

ruflin commented Jan 31, 2018

fassisrosa commented Jan 31, 2018

exekias 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

exekias commented Jan 31, 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

fassisrosa commented Feb 1, 2018

walktall commented Feb 1, 2018

fassisrosa commented Feb 1, 2018 • edited Loading

exekias commented Feb 1, 2018

fassisrosa commented Feb 1, 2018

exekias left a comment

Choose a reason for hiding this comment

exekias commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias commented Feb 2, 2018

fassisrosa commented Feb 3, 2018

exekias commented Feb 5, 2018 • edited Loading

fassisrosa commented Feb 6, 2018

fassisrosa commented Feb 1, 2018 •

edited

Loading

exekias commented Feb 5, 2018 •

edited

Loading