Skip to content
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

Merged
merged 11 commits into from
Mar 15, 2019
Merged

Libbeat add_kubernetes_metadata processor: Missing fields replicaset.name, etc. in fields.yml #11134

merged 11 commits into from
Mar 15, 2019

Conversation

alakahakai
Copy link

@alakahakai alakahakai commented Mar 7, 2019

Address issue #11133:

  • Libbeat add_kubernetes_metadata processor: Add Kubernetes replica-set map to fields.yml

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

@alakahakai alakahakai requested a review from a team as a code owner March 7, 2019 17:35
@alakahakai alakahakai changed the title Add Kubernetes replica-set map to fields.yml Libbeat: Add Kubernetes replica-set map to fields.yml Mar 7, 2019
@alakahakai alakahakai changed the title Libbeat: Add Kubernetes replica-set map to fields.yml Libbeat add_kubernetes_metadata processor: Add Kubernetes replica-set map to fields.yml Mar 7, 2019
@alakahakai alakahakai added the in progress Pull request is currently in progress. label Mar 7, 2019
@alakahakai alakahakai requested a review from a team as a code owner March 7, 2019 18:19
@alakahakai alakahakai added libbeat :Processors and removed in progress Pull request is currently in progress. labels Mar 7, 2019
@alakahakai alakahakai requested review from a team March 8, 2019 01:11
Copy link
Member

@ruflin ruflin left a 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

@alakahakai
Copy link
Author

alakahakai commented Mar 11, 2019

You mention in #11133 that replicaset.name is missing but here you introduce an object replicaset. Why not directly introduce the field?

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:

        - name: labels
          type: object
          description: >
            Kubernetes labels map
        - name: annotations
          type: object
          description: >
            Kubernetes annotations map

Regarding why the CI test is not failing, I assume that it is not testing k8s deployment with anything or filebeat deployed as daemonset.

@alakahakai alakahakai requested a review from ruflin March 11, 2019 16:47
@alakahakai
Copy link
Author

jenkins, test this

@ruflin
Copy link
Member

ruflin commented Mar 12, 2019

lables and tags are special fields where the users add their own subfields and is why we have this kind of mapping. I don't this applies here as we are in control of the fields and should document each fields. Lets add specifically replicaset.name if we know this is missing.

In the original issue you posted:

raise Exception("Key '{}' found in event is not documented!".format(key))
Exception: Key 'kubernetes.replicaset.name' found in event is not documented!

So it seems somehow you got this error?

@alakahakai
Copy link
Author

alakahakai commented Mar 12, 2019

I can see this in libbeat/kibana/testdata/extensive/fields.yml:

        - name: replicaset
          type: group
          description: >
            kubernetes replica set metrics
          fields:
            - name: name
              type: keyword
              description: >
                Kubernetes replica set name
            - name: replicas
              type: group
              description: >
                Kubernetes replica set paused status
              fields:
                - name: available
                  type: long
                  description: >
                    The number of replicas per ReplicaSet
                - name: desired
                  type: long
                  description: >
                    The number of replicas per ReplicaSet
                - name: ready
                  type: long
                  description: >
                    The number of ready replicas per ReplicaSet
                - name: observed
                  type: long
                  description: >
                    The generation observed by the ReplicaSet controller
                - name: labeled
                  type: long
                  description: >
                    The number of fully labeled replicas per ReplicaSet

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.

@alakahakai
Copy link
Author

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.

@ruflin
Copy link
Member

ruflin commented Mar 13, 2019

jenkins, test this

Copy link
Member

@ruflin ruflin left a 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?

@alakahakai
Copy link
Author

alakahakai commented Mar 13, 2019

@ruflin Sure, thanks

Opened issue #11222

@jsoriano
Copy link
Member

Sorry, but I cannot follow 🤔

kubernetes.replicaset fields are not created by anything in libbeat, they are only created by kubernetes state_replicaset metricset, and they are documented there, why are we adding this to all beats?

@alakahakai
Copy link
Author

It is added by the libbeat add_kubernetes_metadata processor. Please take a look at the issue #11133

@alakahakai
Copy link
Author

alakahakai commented Mar 13, 2019

It seems to be added here
Looks like we need to add statefulset.name and deployment.name as well. Also, metadata_test.go does not cover either one.

@alakahakai alakahakai changed the title Libbeat add_kubernetes_metadata processor: Add Kubernetes replica-set map to fields.yml Libbeat add_kubernetes_metadata processor: Missing fields replica-set.name, etc. in fields.yml Mar 13, 2019
@alakahakai alakahakai changed the title Libbeat add_kubernetes_metadata processor: Missing fields replica-set.name, etc. in fields.yml Libbeat add_kubernetes_metadata processor: Missing fields replicaset.name, etc. in fields.yml Mar 13, 2019
@alakahakai alakahakai requested a review from ruflin March 14, 2019 19:32
Copy link
Member

@ruflin ruflin left a 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?

@jsoriano
Copy link
Member

It seems to be added here
Looks like we need to add statefulset.name and deployment.name as well. Also, metadata_test.go does not cover either one.

Oh I see, good catch.

But now these fields are defined in two places for metricbeat, should we remove them from the kubernetes metricsets?

@ruflin
Copy link
Member

ruflin commented Mar 15, 2019

@jsoriano Looks like they are removed here? https://github.com/elastic/beats/pull/11134/files#diff-caf37f855a3380a66e838f4e45e7777dL7

@alakahakai
Copy link
Author

@jsoriano Please let me know if the PR looks good to you. I do have another PR #11200 that are kinda blocked by this. Thanks!

@alakahakai
Copy link
Author

I assume that this is fine to go now. Thanks.

@alakahakai alakahakai merged commit 357c6cf into elastic:master Mar 15, 2019
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants