[Metricbeat] Migrate Kubernetes state_statefulset Metricset to use ReporterV2 interface #10976
Conversation
sayden
added
Metricbeat
sayden
changed the title
|
jenkins, test this |
sayden
force-pushed
the
migration/mb/reporterv2/kubernetes/state_statefulset
branch
from
d400c9e
to
f04c535
Compare
sayden
dismissed
ruflin’s stale review
Won't merge this yet until I test properly the "namespace issue"
sayden
added
in progress
sayden
force-pushed
the
migration/mb/reporterv2/kubernetes/state_statefulset
branch
from
f04c535
to
b526aac
Compare
sayden
added
review
and removed
in progress
| }, | ||
| "kubernetes": { | ||
| "statefulset": { | ||
| "_module": { |
There was a problem hiding this comment.
If I read the code correct, fields under _module should go on the module level. This means this would become kubernetes.namespace: default. This would also align well with https://github.com/elastic/beats/blob/master/libbeat/processors/add_kubernetes_metadata/_meta/fields.yml#L21
Please double check with data you receive when running 6.6. The same applies to all the other modules which use this labels magic.
| @@ -50,10 +49,6 @@ var ( | |||
| "statefulset": p.KeyLabel("name"), | |||
| "namespace": p.KeyLabel(mb.ModuleDataKey + ".namespace"), | |||
There was a problem hiding this comment.
Here is the magic I mentioned above. The part I don't know how you can access this label data below. You probably have to fetch it and delete the key and we change the "framework" later?
| "host": "192.168.99.100:18080", | ||
| "module": "kubernetes", | ||
| "name": "state_statefulset", | ||
| "namespace": "statefulset", |
There was a problem hiding this comment.
@exekias We seem to loose this field here but I would argue it's not an issue. WDYT?
There was a problem hiding this comment.
Is this the kubernetes namespace? that field would be needed to identify the statefulset we are referring to
There was a problem hiding this comment.
nevermind, I see this is a different namespace, we can drop it 👍
There was a problem hiding this comment.
You'll see this "dropping field" pattern in more metricsets in Kubernetes. Just to give you some context @exekias the main idea was to achieve the migration, even dropping few things that weren't critical. In this case I have to admit that I was getting confused with the namespace thing (it seems there are 2 namespaces, the one of kubernetes and the other 😄 ) but I'd need to change quite a lot of code to test the module fields and the root fields (current code only tests metricset fields)
So, now that we have the new testing framework of @ruflin it seemed like the way to go and maybe delete current testing code if it doesn't achieve much more than what we get with the contents of testdata folder.
I know it seems confusing so just ping me if you need a longer explanation 😉
There was a problem hiding this comment.
I think I got it, I'm ok with the change 👍
sayden
force-pushed
the
migration/mb/reporterv2/kubernetes/state_statefulset
branch
from
d86a997
to
cf20619
Compare
|
jenkins, test this |
|
Error seems unrelated. Merging |
Refer to #10774 for more info