-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Libbeat add_kubernetes_metadata processor: Missing fields replicaset.name, etc. in fields.yml #11134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mention in #11133 that replicaset.name is missing but here you introduce an object replicaset
. Why not directly introduce the field?
The other part which is not clear to me is why our tests on CI are not failing. Where do you have the message from that you posted in #11133
General note: If in your PR's you use the URL of issues / pr's instead of {pull}11134[11134]
it will link them automatically together and will make it easier to navigate. Also you can use keywords in your PR's to close issues directly: https://help.github.com/en/articles/closing-issues-using-keywords
There might be other fields that will be added by the processor. I am simply following the existing practice where it uses map for labels and other fields. See below:
Regarding why the CI test is not failing, I assume that it is not testing k8s deployment with anything or filebeat deployed as daemonset. |
jenkins, test this |
In the original issue you posted:
So it seems somehow you got this error? |
I can see this in libbeat/kibana/testdata/extensive/fields.yml:
Why are they not included in all fields? Since the field is not added by filebeat coredns module, we should find out whether there are more fields like this, so that the problem does not come back later when some other fields appear under replicaset. |
Because of time constraints, I am going to just add the missing replicaset.name field for now. But it must be worth looking into why the replicaset fields are missing. |
jenkins, test this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR LGTM. Let's get it in as it already solves part of the problem.
Thanks for the investigation. The fields you linked above are from a test suite but someone must have copied them from somewhere where we potentially will need them. Could you open a follow up Github issue for this so we don't forget about tracking the missing fields?
Sorry, but I cannot follow 🤔
|
It is added by the libbeat add_kubernetes_metadata processor. Please take a look at the issue #11133 |
It seems to be added here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@jsoriano Could you also have a quick look if that is what you expected?
Oh I see, good catch. But now these fields are defined in two places for metricbeat, should we remove them from the kubernetes metricsets? |
@jsoriano Looks like they are removed here? https://github.com/elastic/beats/pull/11134/files#diff-caf37f855a3380a66e838f4e45e7777dL7 |
I assume that this is fine to go now. Thanks. |
…name, etc. in fields.yml (elastic#11134) * Add Kubernetes missing fields: replica-set.name, statefulset.name and deployment.name, to libbeat add_kubernetes_metadata processor fields.yml and add a few test cases. * Remove duplicate fields in metricbeat fields.yml
Address issue #11133:
Now testing works
test_fileset_file_0_coredns (test_xpack_modules.XPackTest) ... ok
test_fileset_file_1_coredns (test_xpack_modules.XPackTest) ... ok
Ran 2 tests in 3.653s
OK