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

Verify ECS alignment in container fields reported by kubernetes module and meta processor #23585

Closed
ChrsMark opened this issue Jan 20, 2021 · 13 comments · Fixed by #24380
Closed
Assignees
Labels
containers Related to containers use case ecs kubernetes Enable builds in the CI for kubernetes Team:Integrations Label for the Integrations team

Comments

@ChrsMark
Copy link
Member

ChrsMark commented Jan 20, 2021

We need to make sure that we have consistent way of aligning common ECS fields across integrations reporting same resource information. An example can be container.image.name and kubernetes.container.image (added at #20984) which is actually the same field but it not aliased like what is happening with Docker:

docker.container.image
type: alias
alias to: container.image.name

Update:
Verify that state_* metricsets follow ECS. For instance testing state_container datastream of k8s package I get:

kubernetes/state_container default:
[0] field "container.runtime" is undefined
[1] field "kubernetes.container.image" is undefined
[2] field "kubernetes.container.name" is undefined
[3] field "kubernetes.namespace" is undefined
[4] field "kubernetes.node.name" is undefined
[5] field "kubernetes.pod.name" is undefined

So kubernetes.container.* seem to need to be changed.

@ChrsMark ChrsMark added the Team:Integrations Label for the Integrations team label Jan 20, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@ChrsMark
Copy link
Member Author

ChrsMark commented Feb 2, 2021

state_container ECS fix PR: #23802

@ChrsMark
Copy link
Member Author

ChrsMark commented Feb 3, 2021

Summary

Right now it seems that we have not been consistent on the way we report container ECS fields. There are 2 ways we report container ECS fields:

  1. using alias
  2. explicitly copying the fields in the code while preserving the original ones until 8.0

alias

add_docker_metadata uses the alias approach:

docker.container.image
type: alias
alias to: container.image.name

copy explicitly

So far the kubernetes module, add_kubernetes_metadata processor and k8s metadata for autodiscover copy ECS containers fields explicitly in the code while:

  1. state_container metricset: Add container image and container name ECS fields for state_container #23802, [Metricbeat]Kubernetes: make state_container metricset ECS compliant #13884
  2. k8s metadata: Add container ECS fields in kubernetes metadata #20984

We keep track of the original fields we need to remove at #13911

At the same time docker module (and most probably fargate) reports directly to ECS fields: docker module

Open issue

The question here is: what approach we should follow in k8s part specifically and in any similar case in general in the future. Using alias or copying directly within the code?

@ruflin @kaiyan-sheng @jsoriano let's continue the discussion here.

@ChrsMark ChrsMark self-assigned this Feb 3, 2021
@ChrsMark ChrsMark added ecs containers Related to containers use case kubernetes Enable builds in the CI for kubernetes labels Feb 3, 2021
@ChrsMark
Copy link
Member Author

ChrsMark commented Mar 1, 2021

Heads-up on this (cc: @kaiyan-sheng @jsoriano ).

I did a simple test to verify that using alias for container meta collected through add_docker_metadata for Filebeat, gives us the option to build visualisations on both container.name and docker.container.name as well as perform searches on these both fields:

Screenshot 2021-03-01 at 3 57 08 PM

Screenshot 2021-03-01 at 3 56 55 PM

Screenshot 2021-03-01 at 3 57 33 PM

Screenshot 2021-03-01 at 3 51 54 PM

It seems that we are not able to search/visualise with reference to the alias fields like docker.container.name.
Didn't check what happens with prebuilt visualisations and what will happen when change the kubernetes.container.name and making an alias of container.name, but the first two examples shouldn't work for us so as to make use of alias right?

@exekias does it happen to remember why we preferred to just copy kubernetes.container.* fields back then (#13884) and we didn't use alias option?

@jsoriano
Copy link
Member

jsoriano commented Mar 1, 2021

It seems that we are not able to search/visualise with reference to the alias fields like docker.container.name.

Could you check if the alias is actually in the index pattern?

This alias is defined with migration: true, so this is probably only included in the index pattern if the migration options are used. This option was included for the migration to ECS in 7.0, see these docs: https://www.elastic.co/guide/en/beats/libbeat/7.11/upgrading-6-to-7.html#enable-ecs-compatibility

        - name: container.name
          type: alias
          path: container.name
          migration: true

@ChrsMark
Copy link
Member Author

ChrsMark commented Mar 2, 2021

You are right @jsoriano 👍🏼 !!!

With migration: true it is not shown in index_template.
I changed to:

"docker": {
          "properties": {
            "attrs": {
              "type": "object"
            },
            "container": {
              "properties": {
                "id": {
                  "type": "alias",
                  "path": "container.id"
                },
                "image": {
                  "type": "alias",
                  "path": "container.image.name"
                }
   }
}

And now it seems to be possible to both search and visualise using the alias:
Screenshot 2021-03-02 at 12 21 56 PM

@exekias
Copy link
Contributor

exekias commented Mar 2, 2021

@exekias does it happen to remember why we preferred to just copy kubernetes.container.* fields back then (#13884) and we didn't use alias option?

I don't really remember this, but I think we can revisit this if needed!

@jsoriano
Copy link
Member

jsoriano commented Mar 2, 2021

I don't really remember this, but I think we can revisit this if needed!

+1 to use alias more for deprecated fields if they work well with visualizations.

@ChrsMark
Copy link
Member Author

ChrsMark commented Mar 2, 2021

Using alias sounds good to me!

I will work on a PR to change the "copy" approach we have for k8s fields so as to further test the alias approach and use it for future reference.

@ChrsMark
Copy link
Member Author

ChrsMark commented Mar 10, 2021

So after some investigation on #24380, we end up to change how we handle kubernetes.container.image field. Before the change this field was set in kubernetes.container.image and container.image.name programatically. This means that both fields are stored. With the use of alias though (at #24380) we only store container.image.name and then we can refer to kubernetes.container.image through an alias in searches and visualisations.

We didn't do the same for kubernates.container.name and conatiner.name since these are actually different fields (#24380 (comment)).

In 8.0 we can completely remove the alias field kubernates.container.image and move on with only the ECS field, if we verify that it's not used in visualisations/searches/dashboards.

We can use this issue as reference in the future when it comes to ECS fields and deprecation plan.

cc: @ruflin

@MichaelKatsoulis
Copy link
Contributor

In 8.0 we can completely remove the alias field kubernates.container.name and move on with only the ECS field, if we verify that it's not used in visualisations/searches/dashboards.

Discussing with @ChrsMark the removal of kubernetes.container.image in 8.0 we reached to the point that we could still keep that field as it is an alias to ECS field container.image.name and is not an extra field in the database. That way users can leverage both in case they want to.
About kubernetes.container.name and kubernates.container.id they have actually different values than ECS fields container.id and container.name.
container.name is the name of the container that the runtime populates while kubernates.container.name is the name of the container in the kubernetes manifest.
I believe that there is no point to delete those kubernetes fields as their values do not relate 100% to any ECS field.

@ruflin, @exekias do you agree on this?

@ruflin
Copy link
Contributor

ruflin commented Sep 13, 2021

What did during the migration to 7.0 is to offer users a flag to enable the alias if they want it, but it was opt in. The problem with keep it by default is that you will keep the fields forever. Having 2 fields with the same meaning is not great and if we keep having more and more aliases, the overall number of fields keeps growing. It is here only about 2 fields it will set a precedence and we will use it in other places too.

For the fields that have separate values, agree we should keep them if these provide value.

@MichaelKatsoulis
Copy link
Contributor

Ok @ruflin I will remove kubernetes.container.image in 8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Related to containers use case ecs kubernetes Enable builds in the CI for kubernetes Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants