Add kubernetes apiserver metricset #7059

exekias · 2018-05-09T15:50:06Z

This new metricset gathers metrics from Kubernetes API servers, giving visibility on number of requests and latency.

This change adds support for summary and histogram metric type to our Prometheus helper, histogram is used to retrieve latency info.

I also simplified the process of creating a new metricset based on this helper, to the point you only need this to create it and this to test it.

houndci-bot · 2018-05-09T15:50:27Z

metricbeat/helper/prometheus/module.go

+	}.Build()
+)
+
+func MetricSetBuilder(mapping *MetricsMapping) func(base mb.BaseMetricSet) (mb.MetricSet, error) {


exported function MetricSetBuilder should have comment or be unexported

houndci-bot · 2018-05-09T15:50:28Z

metricbeat/helper/prometheus/module.go

+	}.Build()
+)
+
+func MetricSetBuilder(mapping *MetricsMapping) func(base mb.BaseMetricSet) (mb.MetricSet, error) {


exported function MetricSetBuilder should have comment or be unexported

ruflin · 2018-05-11T09:51:04Z

metricbeat/module/kubernetes/apiserver/apiserver_test.go

+		prometheus.TestCases{
+			{
+				MetricsFile:  "./_meta/test/metrics",
+				ExpectedFile: "./_meta/test/metrics.expected",


This reminds me a lot of what @simitt was doing here with the approvals: https://github.com/elastic/apm-server/blob/master/tests/approvals.go I wanted to look into it to also do some comparison on json docs for Elasticsearch. Perhaps we can sync up on this.

Thats the approvals script that you can run from cmd line: https://github.com/elastic/apm-server/blob/master/tests/scripts/approvals.go

Would be great to put this in its own small library for more generic usage.

Agreed, we can discuss this and come up with a standardized way to deal with expected output in these tests, also pinging @jsoriano here, as he is working on some ideas to improve our testing.

The ideas I'm working on are more focused to improve and scale integration tests, but any abstraction like these ones are more than welcome of course :)

exekias · 2018-05-14T20:55:00Z

jenkins, retest this please

jsoriano

Pretty cool abstraction to collect prometheus metrics, surely lots things can be monitored this way.

jsoriano · 2018-05-15T07:39:22Z

metricbeat/helper/prometheus/module.go

+		events, err := f.Fetch()
+		assert.NoError(t, err)
+
+		if update {


What about using a flag for update as we do with go test -data?

jsoriano · 2018-05-15T07:43:36Z

metricbeat/module/kubernetes/apiserver/apiserver_test.go

+		prometheus.TestCases{
+			{
+				MetricsFile:  "./_meta/test/metrics",
+				ExpectedFile: "./_meta/test/metrics.expected",


The ideas I'm working on are more focused to improve and scale integration tests, but any abstraction like these ones are more than welcome of course :)

jsoriano · 2018-05-15T07:46:11Z

metricbeat/module/kubernetes/apiserver/apiserver.go

+	if err := mb.Registry.AddMetricSet("kubernetes", "apiserver",
+		prometheus.MetricSetBuilder(mapping), prometheus.HostParser); err != nil {
+		panic(err)
+	}


Use mb.Registry.MustAddMetricSet() here?

exekias · 2018-05-15T09:37:46Z

@jsoriano I've incorporated your suggestions, thanks!

exekias · 2018-05-15T11:32:57Z

jenkins, retest this please

ruflin

The main thing I'm worried is that a testing flag shows up in the production binary. Haven't tested if this is the case.

Seems with api-server we add one more thing to the kubenetes module which seems to be an other service?

ruflin · 2018-05-15T11:49:10Z

metricbeat/helper/prometheus/module.go

+		DefaultPath:   defaultPath,
+	}.Build()
+
+	expectedFlag = flag.Bool("update_expected", false, "Update prometheus expected files")


Is this only used for testing? If yes, should it really be registered here?

You are right, if it is defined here it will appear in the production binary. Testing code should be in a submodule to avoid that.

As we use cobra, common flags are not really "leaked" to the binary, I'm ok with moving this though

ruflin · 2018-05-15T11:49:44Z

metricbeat/helper/prometheus/module.go

+	mapping    *MetricsMapping
+}
+
+func (m *prometheusMetricSet) Fetch() ([]common.MapStr, error) {


Can happen in a later PR, but would be great to also use here the ReporterV2 interface.

ruflin · 2018-05-15T11:50:34Z

metricbeat/helper/prometheus/module.go

+// TestMetricSet goes over the given TestCases and ensures that source Prometheus metrics gets converted into the expected
+// events when passed by the given metricset.
+// If -update_expected flag is passed, the expected JSON file will be updated with the result
+func TestMetricSet(t *testing.T, module, metricset string, cases TestCases) {


I'm thinking we should separate the testing parts and the module parts.

ruflin · 2018-05-15T11:53:01Z

metricbeat/module/kubernetes/apiserver/apiserver.go

@@ -0,0 +1,27 @@
+package apiserver


That is probably the most compact metricset we have. I like it.

exekias · 2018-05-15T16:13:48Z

I converted the helper to use ReporterV2

exekias · 2018-05-15T22:33:04Z

jenkins, test this again

ruflin · 2018-05-16T12:00:07Z

@exekias Will need a rebase and a make update.

ruflin · 2018-05-16T12:06:53Z

metricbeat/helper/prometheus/ptest/ptest.go

@@ -0,0 +1,106 @@
+package ptest


I wonder if we should call these packages always testing as we do in some other places or if this is always and issue because there is a testing package.

Note: Just for discussion, no change in the PR needed.

I had the same thought, but as this is always used along with the Go testing package clash is guaranteed.

This already happens with the mb/testing package, we end up renaming it on import to mbtest, with the annoyance of breaking automatic imports.

There is another example in the latest changes on RabbitMQ module, all modules end up being renamed, but I think this is not so bad (https://github.com/elastic/beats/pull/7106/files#diff-7d67ada6eac5ae4109019e586c93d3a1R8)

I'm slightly in favour of having the same name for all test modules, though not sure if testing is the best option as it collides with the standard library.

For the module testing packages I think all must have the same name or structure, as we need to somehow modify the imports script to skip them. In general what i worry about directories in module is that they can always conflict with metricset names.

This component serves the Kubernetes API, used both internally by other components and externally to query/modify objects state (for instance by using kubectl)

Summaries are not aggregatable, and, while we don't have support to display histograms I still prefer to keep them over summaries.

exekias · 2018-05-16T15:07:54Z

jenkins, retest this please

exekias · 2018-05-17T08:56:59Z

jenkins, retest this please

This new metricset gathers metrics from Kubernetes API servers, giving visibility on number of requests and latency. This change adds support for `summary` and `histogram` metric type to our Prometheus helper, histogram is used to retrieve latency info. I also simplified the process of creating a new metricset based on this helper, to the point you only need [this](https://github.com/exekias/beats/blob/5f4568208ac273d9b759343b22f783184951a477/metricbeat/module/kubernetes/apiserver/apiserver.go) to create it and [this](https://github.com/exekias/beats/blob/5f4568208ac273d9b759343b22f783184951a477/metricbeat/module/kubernetes/apiserver/apiserver_test.go) to test it.

exekias added enhancement in progress Pull request is currently in progress. review containers Related to containers use case labels May 9, 2018

exekias mentioned this pull request May 9, 2018

[Meta Issue] Increase Kubernetes monitoring coverage #7058

Closed

23 tasks

houndci-bot reviewed May 9, 2018

View reviewed changes

exekias force-pushed the k8s-apiserver branch from b8ebe57 to d0b8b1d Compare May 9, 2018 16:45

ruflin reviewed May 11, 2018

View reviewed changes

exekias force-pushed the k8s-apiserver branch 2 times, most recently from b6ef84d to a64eb74 Compare May 14, 2018 18:07

exekias removed the in progress Pull request is currently in progress. label May 14, 2018

jsoriano reviewed May 15, 2018

View reviewed changes

exekias force-pushed the k8s-apiserver branch from 4643e12 to ce6ba2c Compare May 15, 2018 09:48

jsoriano approved these changes May 15, 2018

View reviewed changes

ruflin reviewed May 15, 2018

View reviewed changes

exekias force-pushed the k8s-apiserver branch 2 times, most recently from 3594513 to 52b2c0b Compare May 15, 2018 16:35

ruflin reviewed May 16, 2018

View reviewed changes

Carlos Pérez-Aradros Herce added 5 commits May 16, 2018 14:58

Add Kubernetes API server metricset

a31af5b

This component serves the Kubernetes API, used both internally by other components and externally to query/modify objects state (for instance by using kubectl)

Add summary support to Prometheus helper

5776720

Add latency summary metrics

e439d37

Fix comments

4682e58

Add fields.yml

6f95ed6

Carlos Pérez-Aradros Herce added 10 commits May 16, 2018 14:58

Add config

a026c60

Use histogram instead of summaries

09f3e9a

Summaries are not aggregatable, and, while we don't have support to display histograms I still prefer to keep them over summaries.

Add dashboard

a429995

Fix reference format

81a875e

Add -update_expected flag

ffff3d5

Use MustAddMetricSet

0ecdd65

Update fields.go

06f8248

Add CHANGELOG

d270833

Move testing code to its own package

5a4a97a

Convert to MetricsReporterV2

ffa4308

exekias force-pushed the k8s-apiserver branch from 52b2c0b to ffa4308 Compare May 16, 2018 12:59

jsoriano mentioned this pull request May 16, 2018

Move common RabbitMQ testing code to its own package #7106

Merged

ruflin merged commit b1f1b5f into elastic:master May 17, 2018

This was referenced Jan 30, 2019

Add support to monitor kubernetes kube-proxy #10431

Closed

Add support to monitor CoreDNS #10432

Closed

ioandr mentioned this pull request Feb 19, 2019

Coredns metricbeat module #10585

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kubernetes apiserver metricset #7059

Add kubernetes apiserver metricset #7059

exekias commented May 9, 2018 •

edited

Loading

houndci-bot May 9, 2018

houndci-bot May 9, 2018

ruflin May 11, 2018

simitt May 11, 2018

exekias May 14, 2018

jsoriano May 15, 2018

exekias commented May 14, 2018

jsoriano left a comment

jsoriano May 15, 2018

jsoriano May 15, 2018

jsoriano May 15, 2018 •

edited

Loading

exekias commented May 15, 2018

exekias commented May 15, 2018

ruflin left a comment

ruflin May 15, 2018

jsoriano May 15, 2018

exekias May 15, 2018

ruflin May 15, 2018

ruflin May 15, 2018

ruflin May 15, 2018

exekias commented May 15, 2018

exekias commented May 15, 2018

ruflin commented May 16, 2018

ruflin May 16, 2018

exekias May 16, 2018

jsoriano May 16, 2018

ruflin May 17, 2018

exekias commented May 16, 2018

exekias commented May 17, 2018

Add kubernetes apiserver metricset #7059

Add kubernetes apiserver metricset #7059

Conversation

exekias commented May 9, 2018 • 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

exekias commented May 14, 2018

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

jsoriano May 15, 2018 • edited Loading

Choose a reason for hiding this comment

exekias commented May 15, 2018

exekias commented May 15, 2018

ruflin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias commented May 15, 2018

exekias commented May 15, 2018

ruflin commented May 16, 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

exekias commented May 16, 2018

exekias commented May 17, 2018

exekias commented May 9, 2018 •

edited

Loading

jsoriano May 15, 2018 •

edited

Loading