Automatically enrich Kubernetes module events #7470

exekias · 2018-06-28T21:40:31Z

This PR adds automatically enriching of kubernetes module metricsets. It will behave as add_kubernetes_metadata, but enrich all events coming out of this module by default. This will not only add labels annotations to Pods but any other Resource, like nodes, containers, deployments...

It will be on by default, some configurations are allowed:

- module: kubernetes
  metricsets:
    - pod
    - node
    ...
  include_labels:
    - tier
  include_annotations:
    - ...

Closes #7148

TODO:

houndci-bot · 2018-06-28T21:40:38Z

metricbeat/module/kubernetes/util/kubernetes.go

+	return str
+}
+
+func BuildMetadataEnricher(


exported function BuildMetadataEnricher should have comment or be unexported

houndci-bot · 2018-06-28T21:40:38Z

metricbeat/module/kubernetes/util/kubernetes.go

+	return kubernetes.NewWatcher(client, resource, options)
+}
+
+func NewResourceMetadataEnricher(


exported function NewResourceMetadataEnricher should have comment or be unexported

houndci-bot · 2018-06-28T21:40:39Z

libbeat/common/kubernetes/metadata.go

 	// PodMetadata generates metadata for the given pod taking to account certain filters
 	PodMetadata(pod *Pod) common.MapStr

 	// Containermetadata generates metadata for the given container of a pod
 	ContainerMetadata(pod *Pod, container string) common.MapStr
 }

-type metaGenerator struct {
+// Config for MetaGenerator


comment on exported type MetaGeneratorConfig should be of the form "MetaGeneratorConfig ..." (with optional leading article)

houndci-bot · 2018-06-29T15:04:16Z

metricbeat/module/kubernetes/util/kubernetes.go

+	return strings.Join(fields, ":")
+}
+
+func BuildMetadataEnricher(


exported function BuildMetadataEnricher should have comment or be unexported

houndci-bot · 2018-06-29T15:04:16Z

metricbeat/module/kubernetes/util/kubernetes.go

+	return enricher
+}
+
+func NewContainerMetadataEnricher(


exported function NewContainerMetadataEnricher should have comment or be unexported

jsoriano · 2018-07-05T15:43:37Z

metricbeat/docs/fields.asciidoc

@@ -6890,12 +6890,12 @@ Kubernetes pod name

 --

-*`kubernetes.pod.uid`*::
+*`kubernetes.uid`*::


Breaking change? should we keep both fields till 7.0?

kubernetes.pod.uid is still unreleased, so this change should be ok

Oh, it's not released yet, then my comment above may apply :-)

For my education: This is unique for 1 kubernetes "cluster". Is there also such a thing as a unique pod it?

UIDs are assigned to all resources, by implementation, they are UUIDs. I struggled to either put them under kubernetes.uid or create kubernetes.*.uid for all resources. My reasoning was towards using the global namespace, as we do with labels or annotations. On the other hand, fields like name are not globally unique, hence pod.name makes sense for sure.

I'm open for opinions here, as this is semantically tricky

Can there be cases where more then one of the id's from 2 different resources is in the same event? For example having the pod.uid and the kubernetes.uid which describes the overall cluster in one event? If yes, I would probably put each uid under it's resource. Also correlation wise it probably only makes sense to correlate pod.uid but not correlated it with any other resources?

That case can happen, since #7231

But same would apply to labels/annotations. There is always a main resource for the event, and we put the labels & annotations for that one. Would it make sense to move those under pod.labels?

I so far assume labels is a more generic thing and similar labels apply to different resources. A query could be "give me all resources with label a". Based on this assumption I think the current handling of labels and tags make still sense. But if the labels don't overlap like in the case of the uid the above would seem to make more sense.

Pushed b6d8f76 to move it back to pod.uid

jsoriano · 2018-07-05T15:46:48Z

libbeat/common/kubernetes/metadata.go

 	IncludeLabels          []string `config:"include_labels"`
 	ExcludeLabels          []string `config:"exclude_labels"`
 	IncludeAnnotations     []string `config:"include_annotations"`
-	IncludePodUID          bool     `config:"include_pod_uid"`
+	IncludeUID             bool     `config:"include_uid"`
 	IncludeCreatorMetadata bool     `config:"include_creator_metadata"`


Are these config settings documented?

No, I struggle a bit with these, I wonder if that means we should just hardcode them in. They were kept just in case someone wants to disable them, but I'm not sure

Is it currently disabled by default?

As this doesn't look so costly to collect, maybe we can just add them, and they can be removed with drop_fields.

yes, so far I have left them undocumented to see how much pushback they get once released, I can leave a note here to say we want to remove these settings

jsoriano · 2018-07-05T15:57:43Z

metricbeat/module/kubernetes/_meta/config.yml

+
+  # Enriching parameters:
+  add_metadata: true
+  in_cluster: true


Commented out or removed here as this is the default?

jsoriano · 2018-07-05T16:07:28Z

metricbeat/module/kubernetes/node/node.go

@@ -82,5 +89,7 @@ func (m *MetricSet) Fetch() (common.MapStr, error) {
 		return nil, err
 	}

+	m.enricher.Enrich([]common.MapStr{event})
+


With current abstractions, metric sets have basically the same Fetch (and New) implementation for all resources, maybe we could have an only metricset for them, parametrized with the eventMapper and the enricher.

Yep, I had the same feeling, I think it should be done in a follow up PR

Perhaps time to also convert it to v2 API.

jsoriano · 2018-07-05T16:09:10Z

metricbeat/module/kubernetes/util/kubernetes.go

+		}
+		m.watcherStarted = true
+	}
+}


And a Stop() to release the watcher resources?

Added, thank you for bringing this in, it was in my TODO 👍

jsoriano · 2018-07-05T16:17:01Z

metricbeat/module/kubernetes/util/kubernetes.go

+	"github.com/elastic/beats/metricbeat/mb"
+)
+
+var nilEnricher = &enricher{}


We could define a nilEnricher type that implements the interface with empty methods, to avoid needing the if e == nilEnricher in all methods of enricher.

jsoriano · 2018-07-05T16:25:27Z

metricbeat/module/kubernetes/util/kubernetes.go

+		func(m map[string]common.MapStr, r kubernetes.Resource) {
+			pod := r.(*kubernetes.Pod)
+			for _, container := range append(pod.GetSpec().GetContainers(), pod.GetSpec().GetInitContainers()...) {
+				id := join(r.GetMetadata().GetNamespace(), r.GetMetadata().GetName(), container.GetName())


You could also have a struct type for the ids, so the map is map[resourceID]common.MapStr, and id := resourceID{r.GetMetadata().GetNamespace(), ....
I have had bad experiences in the past building map keys by joining strings 🙂

I would say in general I can do the same mistakes with both approaches, join method is syntactically similar to your resourceID, in both cases I can pass parameters in the wrong order.

exekias · 2018-07-06T08:18:59Z

Thank you for the review @jsoriano!! It should be ready for a second pass. I'll wait for that before rebasing master

jsoriano

LGTM!

This also moves current (unreleased) `pod.uid` to `uid`, as this is a unique id across the kubernetes cluster.

exekias · 2018-07-06T08:43:43Z

👍 rebased master

ruflin · 2018-07-06T12:02:26Z

libbeat/common/kubernetes/metadata_test.go

-				"node":      common.MapStr{"name": "test"},
-				"labels":    common.MapStr{"a": common.MapStr{"value": "bar", "key": "foo"}},
+				"pod":    common.MapStr{"name": ""},
+				"uid":    "005f3b90-4b9d-12f8-acf0-31020a840133",


As uid has be already around before, we should not change anything here so this is only a note from my side.

I often use id instead of uuid or uid for consistency. And as long as there is only one id I expect it to be unique.

UID is a well-known field in Kubernetes, changing it to id could confuse some users IMO

ruflin

Left a few questions. Most can also be addressed in a follow up PR.

ruflin · 2018-07-06T12:04:39Z

metricbeat/docs/fields.asciidoc

@@ -6890,12 +6890,12 @@ Kubernetes pod name

 --

-*`kubernetes.pod.uid`*::
+*`kubernetes.uid`*::


For my education: This is unique for 1 kubernetes "cluster". Is there also such a thing as a unique pod it?

ruflin · 2018-07-06T12:06:46Z

metricbeat/module/kubernetes/container/container.go

 	return events, nil
 }
+
+// Close stops this metricset
+func (m *MetricSet) Close() error {


Did you test if Close is called?

Yes, surprisingly it's called 😺 here: https://github.com/elastic/beats/blob/master/metricbeat/mb/module/wrapper.go#L132

ruflin · 2018-07-06T12:07:27Z

metricbeat/module/kubernetes/node/node.go

@@ -82,5 +89,7 @@ func (m *MetricSet) Fetch() (common.MapStr, error) {
 		return nil, err
 	}

+	m.enricher.Enrich([]common.MapStr{event})
+


Perhaps time to also convert it to v2 API.

ruflin · 2018-07-06T12:09:00Z

metricbeat/module/kubernetes/util/kubernetes.go

+	return kubernetes.NewWatcher(client, resource, options)
+}
+
+// NewResourceMetadataEnricher returns a Enricher configured for kubernetes resource events


Nit: an Enricher

ruflin · 2018-07-06T12:11:00Z

metricbeat/module/kubernetes/util/kubernetes.go

+
+	watcher, err := GetWatcher(base, resource, nodeScope)
+	if err != nil {
+		logp.Warn("Error initializing Kubernetes metadata enricher: %s", err)


I would prefer not to use Warn but either Info or Error. Seems more like Info in this case as even if the enricher fails, the metricset still starts?

It starts, but something is wrong. I used warn to call user attention on this, as it's unexpected. Can switch to info or error if you prefer

We tried to get rid of all Warn messages in the past. The reason is that for Warn it's not clear if users have to take again or not. It sounds like in the above case they have to take again so probably move it to error.

ruflin · 2018-07-06T12:11:08Z

metricbeat/module/kubernetes/util/kubernetes.go

+
+	metaConfig := kubernetes.MetaGeneratorConfig{}
+	if err := base.Module().UnpackConfig(&metaConfig); err != nil {
+		logp.Warn("Error initializing Kubernetes metadata enricher: %s", err)


See comment above.

ruflin · 2018-07-06T12:11:30Z

metricbeat/module/kubernetes/util/kubernetes.go

+
+	watcher, err := GetWatcher(base, &kubernetes.Pod{}, nodeScope)
+	if err != nil {
+		logp.Warn("Error initializing Kubernetes metadata enricher: %s", err)


See comment above, applies also further below.

ruflin · 2018-07-06T12:13:44Z

metricbeat/modules.d/kubernetes.yml.disabled

@@ -28,6 +37,8 @@
 #    - state_container
 #  period: 10s
 #  hosts: ["kube-state-metrics:8080"]
+#  add_metadata: true


Not introduced in this PR, but I think commenting with # should be smae as first part of config (line 16 etc).

exekias · 2018-07-11T15:46:53Z

@ruflin let me know if this one is good to go, I can rebase again once reviewed

ruflin · 2018-07-12T07:27:31Z

metricbeat/module/kubernetes/container/container.go

 	}, nil
 }

 // Fetch methods implements the data gathering and data conversion to the right format
 // It returns the event which is then forward to the output. In case of an error, a
 // descriptive error must be returned.
 func (m *MetricSet) Fetch() ([]common.MapStr, error) {
+	m.enricher.Start()


I wonder if this should be moved to New instead of start. Like this you would not needed the bool isStarted inside start and it would be called only once.

Also I'm a bit worried about potential race conditions for the isStarted variable. What if Fetch() and Stop are called almost at the same time? Or Fetch is for some reason called twice in a very short period?

Moving it to new is not probably a bad option, some processes instantiate a module without launching it. I will add a mutex for the variable

What modules are these?

ruflin · 2018-07-12T07:28:14Z

metricbeat/module/kubernetes/util/kubernetes.go

+}
+
+func (m *enricher) Start() {
+	if !m.watcherStarted {


I think combined with Stop watcherStarted could lead to a race condition.

exekias · 2018-07-13T05:48:10Z

Should be ready for another look

ruflin · 2018-07-13T11:08:07Z

metricbeat/module/kubernetes/container/container.go

 	}, nil
 }

 // Fetch methods implements the data gathering and data conversion to the right format
 // It returns the event which is then forward to the output. In case of an error, a
 // descriptive error must be returned.
 func (m *MetricSet) Fetch() ([]common.MapStr, error) {
+	m.enricher.Start()


What modules are these?

exekias added enhancement in progress Pull request is currently in progress. Metricbeat Metricbeat containers Related to containers use case labels Jun 28, 2018

houndci-bot reviewed Jun 28, 2018

View reviewed changes

exekias force-pushed the kubernetes-module-metadata branch 2 times, most recently from a4feab7 to a5db15b Compare June 29, 2018 11:52

houndci-bot reviewed Jun 29, 2018

View reviewed changes

exekias force-pushed the kubernetes-module-metadata branch 3 times, most recently from d79ab0b to ac1c226 Compare June 29, 2018 15:23

exekias added the review label Jun 29, 2018

exekias force-pushed the kubernetes-module-metadata branch 3 times, most recently from 5f2e824 to 1513b76 Compare July 3, 2018 10:05

exekias requested review from jsoriano and ruflin July 4, 2018 09:48

exekias removed the in progress Pull request is currently in progress. label Jul 4, 2018

exekias force-pushed the kubernetes-module-metadata branch from 2ac2ea6 to 18258a9 Compare July 4, 2018 10:45

jsoriano reviewed Jul 5, 2018

View reviewed changes

exekias force-pushed the kubernetes-module-metadata branch from 4e9a754 to 3c74a22 Compare July 6, 2018 08:31

jsoriano approved these changes Jul 6, 2018

View reviewed changes

Carlos Pérez-Aradros Herce added 5 commits July 6, 2018 10:43

Allow metadata generation for any k8s resource

0a08eab

This also moves current (unreleased) `pod.uid` to `uid`, as this is a unique id across the kubernetes cluster.

Add kubernetes module metadata enricher

392bfbb

Enrich node events with Kubernetes metadata

776cf83

Enrich state_node events with Kubernetes metadata:

b280707

Enrich state_pod events with Kubernetes metadata

138440d

Carlos Pérez-Aradros Herce added 6 commits July 6, 2018 10:43

Update metricbeat role to get access to watch needed resources

4870722

Enrich state_statefulset with Kubernetes metadata

280884b

Implement Closer interface to stop watcher on shutdown

d28aae2

Move nilEnricher to its own implementation

eb72c5a

Update config.yml

a2707a9

Add a note to meta generator config settings

7d3d613

exekias force-pushed the kubernetes-module-metadata branch from 3c74a22 to 7d3d613 Compare July 6, 2018 08:43

ruflin reviewed Jul 6, 2018

View reviewed changes

Carlos Pérez-Aradros Herce added 5 commits July 6, 2018 14:29

fix typo

c2b0454

Move warnings to errors

18d509f

Move uid back to pod.uid

b6d8f76

make update

99b9f84

Merge branch 'master' into kubernetes-module-metadata

68929d0

exekias mentioned this pull request Jul 11, 2018

Merge state_ and common metricsets for node, pod & container #7381

Closed

ruflin reviewed Jul 12, 2018

View reviewed changes

Add locking to start/stop

307734b

exekias force-pushed the kubernetes-module-metadata branch from 96125fd to c282e67 Compare July 12, 2018 10:01

Merge branch 'master' into kubernetes-module-metadata

d582e8b

exekias force-pushed the kubernetes-module-metadata branch from c282e67 to d582e8b Compare July 12, 2018 10:04

Carlos Pérez-Aradros Herce added 2 commits July 12, 2018 12:08

Add missing license headers

cb88d1e

Merge branch 'master' into kubernetes-module-metadata

5dcb137

ruflin approved these changes Jul 13, 2018

View reviewed changes

ruflin merged commit 98e488a into elastic:master Jul 13, 2018

exekias mentioned this pull request Jan 7, 2019

Consider combining "<type>" and "state_<type>" kubernetes metricsets into a single metricset/event. #6637

Closed

dedemorton mentioned this pull request Feb 4, 2020

add_metadata setting in Kubernetes metricbeat module is not documented #16047

Closed

Automatically enrich Kubernetes module events #7470

Automatically enrich Kubernetes module events #7470

Conversation

exekias commented Jun 28, 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

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

jsoriano Jul 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias commented Jul 6, 2018

jsoriano left a comment

Choose a reason for hiding this comment

exekias commented Jul 6, 2018

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

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 Jul 11, 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 Jul 13, 2018

Choose a reason for hiding this comment

exekias commented Jun 28, 2018 •

edited

Loading

jsoriano Jul 5, 2018 •

edited

Loading