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

Use new json type for labels and annotations #9286

Closed
wants to merge 1 commit into from

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Nov 29, 2018

Uses new type for queryable object fields for docker and kubernetes labels and annotations, so any label name is allowed without modifications. This should fix mapping issues we are having with some combinations of labels.

I haven't removed the dedotting options in case some user wants to continue using it, but probably we could remove it also for 7.0.

It depends on this feature to be merged in ES and on its support for terms aggregations

@jsoriano jsoriano added review libbeat Metricbeat Metricbeat blocked containers Related to containers use case labels Nov 29, 2018
@jsoriano jsoriano requested a review from exekias November 29, 2018 13:09
@jsoriano jsoriano added v7.0.0 Team:Integrations Label for the Integrations team labels Nov 29, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/infrastructure

@jsoriano
Copy link
Member Author

Added issue in ECS to discuss the introduction of this type also there elastic/ecs#198

@jsoriano jsoriano added the discuss Issue needs further discussion. label Nov 29, 2018
@@ -150,7 +150,7 @@ func generateMapSubset(input map[string]string, keys []string) common.MapStr {
for _, key := range keys {
value, ok := input[key]
if ok {
safemapstr.Put(output, key, value)
output[key] = value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we would put it still in the same format?

Copy link
Member Author

@jsoriano jsoriano Dec 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean with Put? Put creates the intermediate maps, something we don't want here. In this case we want to use directly the labels as keys, no matter if they have dots or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was more or less my question. So if the tree is created inside a json type the keys can't be used directly? This is mainly for my understanding.

Copy link
Member Author

@jsoriano jsoriano Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they can be used too, but this can be problematic, for example for these labels:

docker.container.labels.foo=something
docker.container.labels.foo.response=42

Using Put there can be two different possible results depending on what label is "put" first, this happens with the json type or without it, because this is caused by how we build the event:

{
  "docker": {
    "container": {
      "labels": {
        "foo": "something",
      }
    }
  }
}

Or

{
  "docker": {
    "container": {
      "labels": {
        "foo": {
          "response": "42"
        }
      }
    }
  }
}

With safemapstr this was resolved storing this like:

{
  "docker": {
    "container": {
      "labels": {
        "foo": {
          "response": "42",
          "value": "something"
        }
      }
    }
  }
}

Assigning directly (without Put) and using the new json type, the labels keep their format in the stored event:

{
  "docker": {
    "container": {
      "labels": {
        "foo.response": "42",
        "foo": "something"
      }
    }
  }
}

In all cases the keys can be used directly, but they can be different, and only with the last option they are kept as they are.

@jsoriano
Copy link
Member Author

jsoriano commented Dec 10, 2018

Closing this by now as this wouldn't support grouping based on terms aggregations on initial implementations, something we want to maintain.

@jsoriano jsoriano closed this Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked containers Related to containers use case discuss Issue needs further discussion. libbeat Metricbeat Metricbeat review Team:Integrations Label for the Integrations team v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants