[Metricbeat] Zookeeper ecs #10286
[Metricbeat] Zookeeper ecs #10286
Conversation
ruflin
added
in progress
| @@ -5,6 +5,7 @@ | |||
| command. | |||
| release: ga | |||
| fields: | |||
| # TODO: Shoudl also be migrated? | |||
There was a problem hiding this comment.
@webmat As we don't have service.hostname you would say this should go into host.hostname
There was a problem hiding this comment.
I don't see this field refered anywhere else in the code, we can probably remove it from here.
There was a problem hiding this comment.
If I were managing a Zk cluster, I would definitely want to know which hostname an event is associated to. I say we keep this field.
I agree with using host.hostname.
There was a problem hiding this comment.
The problem that this will cause now is that host.name will be filled in by Beats and host.hostname will be filled by the service (which could be on a remote host).
Theoretically if host.hostname != host.name, the metadata from the host where Beats is running on should not be added or added in the observer, correct?
There was a problem hiding this comment.
For me, events observed on one of my Kafka nodes should have host.* about the host of the Kafka node, whether it's remote or local.
The data about the host running Metricbeat should either be under observer or agent (e.g. agent.hostname), not host. The agent's details are pipeline metadata, not "userland" event data. So I'd rather have Metricbeat's host metadata processor not populate host.
For your host.name concern, seeing how is this value set when configuring a Beat, I'd say the name: Beat option should populate observer instead.
Note that I'm starting to see concrete problems with the agent/observer dichotomy, in relationship to host. Too late to open that can of worm for now, though...
There was a problem hiding this comment.
Sounds like we are on the same page here for all the above you mentioned. Unfortunately not something we can clean up right now.
To not have confusion but still this value in one place, WDYT about service.hostname? We already have service.address from Metricbeat and we could add this on the Metricbeat side. Does not require to be in ECS.
There was a problem hiding this comment.
I removed the comment for now so this PR can be merged. Lets still continue this discussion as I think it applies broader then just to zookeeper.
There was a problem hiding this comment.
Yeah let's go with service.hostname for now
|
|
||
| t.Logf("%s/%s event: %+v", f.Module().Name(), f.Name(), event) | ||
|
|
||
| e, _ := events[0].BeatEvent("zookeeper", "mntr").Fields.GetValue("zookeeper.mntr") |
There was a problem hiding this comment.
events[0].BeatEvent("zookeeper", "mntr").Fields could be stored in a variable as it is being used two times.
There was a problem hiding this comment.
Agreed, would make the logic a little more obvious.
| "service": { | ||
| "address": "localhost:2181", | ||
| "type": "zookeeper", | ||
| "version": "3.4.8--1, built on 02/06/2016 03:18 GMT" |
There was a problem hiding this comment.
Oh jeez 🤦♂️
Well, I didn't think our keyword version field would get versions that detailed 😆I guess that's a testament to the flexibility of ECS
There was a problem hiding this comment.
The nice part is that 3.4.8 is at the beginning, so prefix queries for 3.4 still work.
| @@ -5,6 +5,7 @@ | |||
| command. | |||
| release: ga | |||
| fields: | |||
| # TODO: Shoudl also be migrated? | |||
There was a problem hiding this comment.
If I were managing a Zk cluster, I would definitely want to know which hostname an event is associated to. I say we keep this field.
I agree with using host.hostname.
|
|
||
| t.Logf("%s/%s event: %+v", f.Module().Name(), f.Name(), event) | ||
|
|
||
| e, _ := events[0].BeatEvent("zookeeper", "mntr").Fields.GetValue("zookeeper.mntr") |
There was a problem hiding this comment.
Agreed, would make the logic a little more obvious.
* Rename `zookeeper.mntr.version` to `service.version` The bigger refactoring in this PR to the reporter interface was needed to allow publishing version on the top level.
No description provided.