Add `kube-state-metrics` based metricsets #4253

exekias · 2017-05-08T16:53:45Z

Add k8s metrics extracted from kube-state-metrics services. This adds info specially useful about:

Status of deployments, pods and replicasets
Resource limits, requests and usage by pods
Node available resources

I have updated existing dashboard with this kind of info

ruflin

Looking forward to have this module in. We should have a conversation about the end result of the data model.

ruflin · 2017-05-08T18:29:47Z

metricbeat/module/kubernetes/state_container/_meta/data.json

+          }
+        }
+      },
+      "status": {


The name implies state and here it is status?

I'm using kube-state-metrics nomenclature here, check https://github.com/elastic/beats/pull/4253/files/0f5f98da5eb5acf11055a8d8cf551f7b4eb1ed0e#diff-c70b508c24452e40e23c98c944027e25R54 to see the metric family names

This is how I would explain it:

State (from kube-state-metrics) means that we get access to the global state of the cluster (think of it as a snapshot in time). While this status metrics are one of the specific metrics of a container.

got it. thanks.

If we namespace this under state it probably becomes more obvious (see data structure comments).

ruflin · 2017-05-08T18:30:45Z

metricbeat/module/kubernetes/state_container/_meta/data.json

+      "resource": {
+        "requests": {
+          "cpu": {
+            "nanocores": 5000000


Is this something that will change over time? If we call it state_container all these fields which are not shared should be under one namespace.

ruflin · 2017-05-08T18:31:37Z

metricbeat/module/kubernetes/state_container/data.go

+func eventMapping(families []*dto.MetricFamily) ([]common.MapStr, error) {
+	eventsMap := map[string]common.MapStr{}
+	for _, family := range families {
+		//fmt.Println(family)


Should be removed.

ruflin · 2017-05-08T18:32:55Z

metricbeat/module/kubernetes/state_container/data.go

+	nanocores = 1000000000
+)
+
+func eventMapping(families []*dto.MetricFamily) ([]common.MapStr, error) {


I would kind of hope instead of adding dependency on prometheus here, that this can "depend" on the prometheus module / metricset.

That was my initial intention, but I have 2 issues with that:

Prometheus module mangles original data on it's way, and it's not really aware of the format of these metrics, it does something generic. So I need to go over the resulting MapStr again

If we do a change in prometheus module it may break this, having that I'm not using its format

Something I thought about was to move all prometheus code to a common helper (like HTTP), WDYT about that? At the end it looked like overkill but I'm ok with doing it

+1 on the helper. Can also be done in a follow up PR. But I think it's good to have it around as this will not be the last metricset using prometheus.

ruflin · 2017-05-08T18:33:20Z

metricbeat/module/kubernetes/state_container/data.go

+				}
+
+			default:
+				// Ignore unknown metric


Sounds like an error or at least worth logging as debug?

ruflin · 2017-05-08T18:39:26Z

metricbeat/module/kubernetes/state_pod/_meta/data.json

+        "scheduled": "true"
+      }
+    },
+    "state_pod": {}


empty namespace

ruflin · 2017-05-08T18:41:18Z

metricbeat/module/kubernetes/state_replicaset/data.go

@@ -0,0 +1,59 @@
+package state_replicaset


could we call this replicaset

I wanted to keep the same format for all kube-state-metrics metricsets for one reason, the hosts setting is incompatible between kubelet and kube-state-metrics, you have to choose, and it's nice to have that clear from the metricset name, to avoid confusion.

Check https://github.com/exekias/beats/blob/d091ce76089c44e5fd872661d529932aa92633df/metricbeat/module/kubernetes/_meta/config.yml for an example of possible configurations

I see the point. Leads me to think it should be it's own module but we concluded this discussion :-) Don't have a better solution at the moment :-(

Yeah, I'm not 100% happy with this either, we can see how it goes and perhaps improve it in the future

ruflin · 2017-05-08T18:41:36Z

metricbeat/module/kubernetes/state_replicaset/state_replicaset.go

+
+	return &MetricSet{
+		BaseMetricSet: base,
+		counter:       1,


remove counter

ruflin · 2017-05-08T18:43:18Z

metricbeat/module/kubernetes/util/prometheus.go

+	"github.com/prometheus/common/expfmt"
+)
+
+func GetFamilies(http *helper.HTTP) ([]*dto.MetricFamily, error) {


Should this be a prometheus helper under the helper namespace? Or can you use the prometheus.collector code here?

ok to the helper, prometheus collector issue explained in a previous comment, but I can update it to share the helper

ruflin · 2017-05-08T18:44:35Z

metricbeat/module/kubernetes/vendor/vendor.json

@@ -0,0 +1,43 @@
+{
+	"comment": "",
+	"ignore": "test",


you need to add github.com/elastic/beats here as otherwise when you remove all the beats dependencies will be added to when doing an update.

andrewkroh · 2017-05-08T18:35:36Z

metricbeat/module/kubernetes/state_container/data.go

+func eventMapping(families []*dto.MetricFamily) ([]common.MapStr, error) {
+	eventsMap := map[string]common.MapStr{}
+	for _, family := range families {
+		//fmt.Println(family)


This should be removed.

andrewkroh · 2017-05-08T18:43:08Z

metricbeat/module/kubernetes/state_container/state_container.go

+
+	logp.Beta("The kubernetes state_container metricset is experimental")
+
+	if err := base.Module().UnpackConfig(&config); err != nil {


This can be dropped since no config is used.

andrewkroh · 2017-05-08T18:44:22Z

metricbeat/module/kubernetes/state_pod/state_pod.go

+func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
+	config := struct{}{}
+
+	logp.Beta("The kubernetes state_pod metricset is experimental")


There is a logp.Experimental().

andrewkroh · 2017-05-08T18:46:44Z

metricbeat/module/kubernetes/state_container/_meta/fields.yml

+            - name: cpu.nanocores
+              type: long
+              description: >
+                Container CPU requested nanocores


So what is a nanocore? Nanoseconds of CPU time per core?

It's the other way around, in this case it's number of requested (reserved) full CPU cores. So 1 Core would mean the container has 1 full core reserved for itself, as long as it's alive.

BTW this metric is also present as a usage metric: https://github.com/elastic/beats/blob/master/metricbeat/module/kubernetes/container/_meta/fields.yml#L21

In that case it's the accumulated value over time (I use derivative in graphs to extract the used nancores per bucket)

A common scenario is to check requested vs limits vs real usage. Ideally you would put some sane limits based on real usage of a given container

elasticmachine · 2017-05-08T20:32:36Z

metricbeat/module/kubernetes/util/prometheus.go

+	"github.com/prometheus/common/expfmt"
+)
+
+func GetFamilies(http *helper.HTTP) ([]*dto.MetricFamily, error) {


[golint] _{reported by reviewdog 🐶}
exported function GetFamilies should have comment or be unexported

elasticmachine · 2017-05-08T20:32:36Z

metricbeat/module/kubernetes/util/prometheus.go

+	return families, nil
+}
+
+func GetLabel(m *dto.Metric, label string) string {


[golint] _{reported by reviewdog 🐶}
exported function GetLabel should have comment or be unexported

ruflin · 2017-05-11T10:19:17Z

metricbeat/helper/prometheus.go

+	"fmt"
+
+	"github.com/elastic/beats/metricbeat/mb"
+	dto "github.com/prometheus/client_model/go"


Here a newline should be added.

ruflin

During this review I mainly focused on the data structure. I really like that now the output documents are organised with a focus on the consumer, means the person that builds dashboards and queries for it. It decouples configuration namespaces from the output which I think is a good thing.

Assuming we can keep the data structure I'm tempted to say in the future we could split it up again in 2 modules both not called kubernetes but both putting their data into the kubernetes namespace which is kind of an empty module that only has fields and docs, no code.

As now fields construct became not quite tricky to follow I think it is critical that we compare our data.json output against the fields.yml file to make sure all fields are documented.

ruflin · 2017-05-11T10:19:41Z

metricbeat/helper/prometheus.go

+
+// Prometheus helper retrieves prometheus formatted metrics
+type Prometheus struct {
+	HTTP


I like that, @urso will not like it too much ;-)

ruflin · 2017-05-11T10:20:32Z

metricbeat/metricbeat.full.yml

+- module: kubernetes
+  enabled: false
+  metricsets:
+    - state_node


Based on a comment from @andrewkroh in an other PR I'm hoping we can make this state.node etc. in the future (not in this PR).

ruflin · 2017-05-11T10:22:26Z

metricbeat/module/kubernetes/state_container/data.go

+
+func eventMapping(families []*dto.MetricFamily) ([]common.MapStr, error) {
+	eventsMap := map[string]common.MapStr{}
+	for _, family := range families {


Is this still needed or can it be moved now to the helper?

I think it's pretty custom for this case, so we need to keep it.

I want to see what future metricsets using the helper have in common, perhaps we can still extract some helpers there

exekias · 2017-05-11T19:10:41Z

jenkins, retest this please

andrewkroh · 2017-05-11T19:13:47Z

^ You're so polite to the robots 🤖 . I guess you are hoping for favorable treatment when they take over.

exekias · 2017-05-11T19:20:13Z

exekias · 2017-05-15T09:59:10Z

jenkins, test it

ruflin · 2017-05-16T07:31:32Z

libbeat/tests/system/beat/beat.py

@@ -426,7 +426,7 @@ def extract_fields(doc_list, name):

        # TODO: Make fields_doc path more generic to work with beat-generator
        with open(fields_doc, "r") as f:
-            path = os.path.abspath(os.path.dirname(__file__) + "../../../../_meta/fields.common.yml")
+            path = os.path.abspath(os.path.dirname(__file__) + "../../../../_meta/fields.generated.yml")


Does the generated exist in all beats?

I think this could be the reason that the appveyor builds fail.

I've added a fallback to common.yml, let's see if appveyor likes that

it worked, thanks!

ruflin · 2017-05-16T07:32:11Z

metricbeat/docker-compose.yml

@@ -51,6 +52,8 @@ services:
      jolokia:        { condition: service_healthy }
      kafka:          { condition: service_healthy }
      kibana:         { condition: service_healthy }
+      kubernetes:     { condition: service_healthy }
+      kube-state:     { condition: service_started }


Interesting that this one is set to started and not healthy

kube-state-metrics doesn't provide health service, but as soon as it starts it should work so we can live with this it seems

ruflin · 2017-05-16T07:36:50Z

metricbeat/module/kubernetes/_meta/config.yml

+    - state_pod
+    - state_container
+  period: 10s
+  hosts: ["kube-state-metrics:8080"]


Does this have a "common" ip or dns name in a kubernetes environment?

Yes, normally it's that: https://github.com/kubernetes/kube-state-metrics/blob/master/kubernetes/kube-state-metrics-service.yaml#L4

ruflin · 2017-05-16T07:41:00Z

metricbeat/tests/system/test_kubernetes.py

+
+        self.assertItemsEqual(self.de_dot(KUBERNETES_FIELDS), evt.keys(), evt)
+
+        self.assert_fields_are_documented(evt)


This file includes fields from processors, as they are used by tests Fallbacks to `fields.common.yml` when generated is not present

exekias added in progress Pull request is currently in progress. Metricbeat Metricbeat review v6.0.0-alpha2 labels May 8, 2017

ruflin reviewed May 8, 2017

View reviewed changes

andrewkroh reviewed May 8, 2017

View reviewed changes

elasticmachine reviewed May 8, 2017

View reviewed changes

exekias force-pushed the kube-state-metrics branch 8 times, most recently from faf700b to 5589ec9 Compare May 10, 2017 16:58

exekias removed the in progress Pull request is currently in progress. label May 10, 2017

exekias changed the title ~~[WIP] Add kube-state-metrics based metricsets~~ Add kube-state-metrics based metricsets May 10, 2017

exekias force-pushed the kube-state-metrics branch from 5589ec9 to 047c561 Compare May 10, 2017 17:04

ruflin reviewed May 11, 2017

View reviewed changes

exekias force-pushed the kube-state-metrics branch 3 times, most recently from 1bde788 to d7efc3a Compare May 11, 2017 17:20

exekias force-pushed the kube-state-metrics branch from d7efc3a to 071cac3 Compare May 11, 2017 19:17

exekias force-pushed the kube-state-metrics branch 3 times, most recently from 385d5b4 to e7c3361 Compare May 11, 2017 21:10

exekias force-pushed the kube-state-metrics branch 4 times, most recently from 6c773d8 to fb8f972 Compare May 13, 2017 09:17

Add kube-state-metrics based metricsets

e13f217

exekias force-pushed the kube-state-metrics branch 2 times, most recently from 8567fa9 to 0ec0b1d Compare May 14, 2017 16:15

exekias added the in progress Pull request is currently in progress. label May 14, 2017

exekias force-pushed the kube-state-metrics branch 4 times, most recently from 876e39d to 33463ae Compare May 14, 2017 21:45

exekias force-pushed the kube-state-metrics branch 3 times, most recently from c80556f to 0548bb4 Compare May 15, 2017 13:00

Add global Prometheus helper

ef295e1

exekias force-pushed the kube-state-metrics branch 3 times, most recently from 4319e79 to 1519037 Compare May 15, 2017 14:25

exekias removed the in progress Pull request is currently in progress. label May 15, 2017

ruflin reviewed May 16, 2017

View reviewed changes

exekias force-pushed the kube-state-metrics branch from 1519037 to 2fa7015 Compare May 16, 2017 08:55

exekias added 3 commits May 16, 2017 12:20

Use fields.generated.yml when reading common fields

28c68cb

This file includes fields from processors, as they are used by tests Fallbacks to `fields.common.yml` when generated is not present

Add kubelet system tests based on hyperkube

7c8316c

Add kube-state-metrics system tests

1fba4ef

exekias force-pushed the kube-state-metrics branch from 2fa7015 to 1fba4ef Compare May 16, 2017 10:21

ruflin merged commit 5cbd804 into elastic:master May 16, 2017


		logp.Beta("The kubernetes state_container metricset is experimental")

		if err := base.Module().UnpackConfig(&config); err != nil {


		self.assertItemsEqual(self.de_dot(KUBERNETES_FIELDS), evt.keys(), evt)

		self.assert_fields_are_documented(evt)

Add kube-state-metrics based metricsets #4253

Add kube-state-metrics based metricsets #4253

Conversation

exekias commented May 8, 2017

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias May 8, 2017 • 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

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

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

andrewkroh May 8, 2017 • 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

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

exekias commented May 11, 2017

andrewkroh commented May 11, 2017

exekias commented May 11, 2017

exekias commented May 15, 2017

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

Choose a reason for hiding this comment

Add `kube-state-metrics` based metricsets #4253

Add `kube-state-metrics` based metricsets #4253

exekias May 8, 2017 •

edited

Loading

andrewkroh May 8, 2017 •

edited

Loading