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

Migrate add_docker_metadata to ECS #9412

Merged
merged 1 commit into from
Dec 27, 2018

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Dec 6, 2018

Migrate the docker fields to ECS container fields.

  • docker.container.id -> container.id
  • docker.container.image -> container.image.name
  • docker.container.name -> container.name
  • docker.container.labels -> container.labels

@ruflin ruflin added in progress Pull request is currently in progress. libbeat ecs labels Dec 6, 2018
alias6: true
alias: true

- from: docker.container.labels # TODO: How to map these?
Copy link
Member Author

Choose a reason for hiding this comment

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

@graphaelli Have you found a good way to migrate such fields?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can. I'm assuming this is user-populated, including key names? If that's the case, then the list of fields to migrate is unknowable.

Perhaps if some fields are often present by default, we can create aliases for those, though.

Copy link
Member

@graphaelli graphaelli Dec 13, 2018

Choose a reason for hiding this comment

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

I have not found a good way. Can the migration assistant create those aliases dynamically? That is, iterate over the source labels and create aliases on the old indices for each one? Another option is to inform users to include both fields with an or clause in searches and aggregations.

APM has the same issue where context.tags are moving to labels. It may be possible there to introduce an APM UI-only concept of "tags" "labels" and make the context.tags.$key=$value or labels.$key=value query on behalf of the user - we have not discussed this possibility but I'd be curious what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

@graphaelli Indeed that is something that the migration assistant could potentially do. I think in this case it's more important for apm-server as you probably need it for the 6.x compatiblity layer and in our case it's nice to have as we can also break it in 7 and inform the user about this one. If we can use then the same feature to, that is great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think the migration assistant will be our best bet.

Copy link
Member Author

Choose a reason for hiding this comment

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

@graphaelli Can you take this up with the Kibana team working on the migration assistant to see what they think?

Copy link
Member

Choose a reason for hiding this comment

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

They've added it to their use cases for the migration assistant, there will be hooks for us to handle it there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great news. Thanks for following up.

@@ -88,7 +88,7 @@ func buildDockerMetadataProcessor(cfg *common.Config, watcherConstructor docker.
"field": "source",
"separator": string(os.PathSeparator),
"index": config.SourceIndex,
"target": "docker.container.id",
"target": "container.id",
Copy link
Member Author

Choose a reason for hiding this comment

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

Was not sure why here the constant was not used. @exekias ok to change this one or could this have other side effects?

Copy link
Contributor

Choose a reason for hiding this comment

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

uhm, probably they were introduced at different points in time, I don't see a reason for it to not use the constant

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, replaced it by the constant then.

@@ -166,10 +166,6 @@ func (d *addDockerMetadata) Run(event *beat.Event) (*beat.Event, error) {
container := d.watcher.Container(cid)
if container != nil {
meta := common.MapStr{}
metaIface, ok := event.Fields["docker"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove this one as I felt it's not needed anymore when using Put with the DeepUpdate. Please double check if that is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

while that's true, I think that will create more unneeded allocations? this is the kind of code that runs lots of times

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I can fully follow here. You are mostly worried about the DeepUpdate part?

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to merge for now. @exekias Let's revisit this next year to make sure we have the correct optimisations in place.

@ruflin ruflin mentioned this pull request Dec 6, 2018
@ruflin ruflin added the Team:Integrations Label for the Integrations team label Dec 10, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/infrastructure

@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch from d02919b to 309df10 Compare December 10, 2018 09:21
@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch 2 times, most recently from 0e4e3d3 to d93cca7 Compare December 12, 2018 13:43
@ruflin
Copy link
Member Author

ruflin commented Dec 13, 2018

I suggest we move forward with this PR even though alias for docker.container.labels do not exist as it's not straight forward. I added a note about it to our ECS issue so we can track it there.

@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch from b671461 to 275de21 Compare December 13, 2018 08:50
@ruflin ruflin added review and removed in progress Pull request is currently in progress. labels Dec 13, 2018
@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch 2 times, most recently from 0dbc2f5 to 90746ae Compare December 13, 2018 16:00
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

One small issue in ecs-migration.yml, and two comments that will help inform next steps.

dev-tools/ecs-migration.yml Outdated Show resolved Hide resolved
alias6: true
alias: true

- from: docker.container.labels # TODO: How to map these?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can. I'm assuming this is user-populated, including key names? If that's the case, then the list of fields to migrate is unknowable.

Perhaps if some fields are often present by default, we can create aliases for those, though.

@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch 2 times, most recently from ce274f3 to 763e16b Compare December 14, 2018 14:34
@ruflin ruflin requested a review from a team as a code owner December 14, 2018 14:34
@ruflin
Copy link
Member Author

ruflin commented Dec 17, 2018

@webmat Could you take a look again to get this in?

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch from 763e16b to c66ed45 Compare December 18, 2018 08:06
@@ -8,5 +8,6 @@ This file is generated! See scripts/docs_collector.py

--

include::./modules/.DS_Store.asciidoc[]
Copy link
Member Author

Choose a reason for hiding this comment

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

@adriansr @cwurm This probably needs some fixing in the collection script. Will fix it in this PR manually.

@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch 2 times, most recently from d32ddf1 to cb8f556 Compare December 18, 2018 12:46
@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch from cb8f556 to d2e260d Compare December 27, 2018 08:11
@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch from d2e260d to 3578b0f Compare December 27, 2018 09:16
Migrate the docker fields to ECS container fields.

* docker.container.id -> container.id
* docker.container.image -> container.image.name
* docker.container.name -> container.name
* docker.container.labels -> container.labels

make image fix

update generator script
@ruflin ruflin force-pushed the migrate-add-docker-metadata-to-ecs branch from 3578b0f to 0a3caab Compare December 27, 2018 12:02
@ruflin ruflin merged commit f43683f into elastic:master Dec 27, 2018
@ruflin ruflin deleted the migrate-add-docker-metadata-to-ecs branch December 27, 2018 15:12
jsoriano added a commit that referenced this pull request Feb 25, 2019
Fields injected by docker autodiscover provider were being placed
in alias fields introduced for ECS, change them to the new location
and add selectors accordingly. 

This PR includes #10862 and #10758

As a summary:
    * Autodiscover selectors using ECS structure are added to
      autodiscover events, old selectors are kept for backwards compatibility
    * Autodiscover generated metadata follows ECS
    * Dedotting of labels is added, enabled by default, will be backported for 6.7,
      but disabled

`docker.containers.labels` is not migrated, as it wasn't for `add_docker_metadata`
(see #9412)

Fixes #10757

Co-Authored-By: kaiyan-sheng <kaiyan.sheng@elastic.co>
Co-Authored-By: Nicolas Ruflin <spam@ruflin.com>
jsoriano added a commit to jsoriano/beats that referenced this pull request Feb 25, 2019
Fields injected by docker autodiscover provider were being placed
in alias fields introduced for ECS, change them to the new location
and add selectors accordingly. 

This PR includes elastic#10862 and elastic#10758

As a summary:
    * Autodiscover selectors using ECS structure are added to
      autodiscover events, old selectors are kept for backwards compatibility
    * Autodiscover generated metadata follows ECS
    * Dedotting of labels is added, enabled by default, will be backported for 6.7,
      but disabled

`docker.containers.labels` is not migrated, as it wasn't for `add_docker_metadata`
(see elastic#9412)

Fixes elastic#10757

Co-Authored-By: kaiyan-sheng <kaiyan.sheng@elastic.co>
Co-Authored-By: Nicolas Ruflin <spam@ruflin.com>
(cherry picked from commit 1bf8087)
jsoriano added a commit that referenced this pull request Feb 27, 2019
Fields injected by docker autodiscover provider were being placed
in alias fields introduced for ECS, change them to the new location
and add selectors accordingly. 

This PR includes #10862 and #10758

As a summary:
    * Autodiscover selectors using ECS structure are added to
      autodiscover events, old selectors are kept for backwards compatibility
    * Autodiscover generated metadata follows ECS
    * Dedotting of labels is added, enabled by default, will be backported for 6.7,
      but disabled

`docker.containers.labels` is not migrated, as it wasn't for `add_docker_metadata`
(see #9412)

Fixes #10757

(cherry picked from commit 1bf8087)

Co-Authored-By: kaiyan-sheng <kaiyan.sheng@elastic.co>
Co-Authored-By: Nicolas Ruflin <spam@ruflin.com>
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
Migrate the docker fields to ECS container fields.

* docker.container.id -> container.id
* docker.container.image -> container.image.name
* docker.container.name -> container.name
* docker.container.labels -> container.labels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecs libbeat review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants