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

[Fleet] Support dynamic_template mappings from object field #137772

Merged
merged 8 commits into from
Aug 3, 2022

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Aug 1, 2022

Summary

Related to #129344

Fleet generate mappings from fields yaml provided by integrations, we were not handling field with object and object_type correctly as these fields should not result in properties mappings but in dynamic templates.

That PR fix that, if a field is of type object and contains an object_type we generate a dynamic template, if the path do not contains a * we add .* at the end of the path.

For example this fields:

- name: cockroachdb.status.*.histogram
  type: object
  object_type: histogram
  object_type_mapping_type: '*'
  description: >-
    Prometheus histogram metric

Will result in the following mappings

"dynamic_templates": [{
      "cockroachdb.status.*.histogram": {
        "mapping": {
          "type": "histogram"
        },
        "match_mapping_type": "*",
        "path_match": "cockroachdb.status.*.histogram"
      }
    }
  ]

How this was implemented in beats https://github.com/elastic/beats/blob/ea207346d651448b8917b0791b2b117b9f9b9212/libbeat/template/processor.go#L444

In a second PR, we will introduce a way to generate dynamic template just by adding a * in the property name.

@jsoriano Does it make sense to you?

I tested this by installing all the integrations (to verify all the integrations still install) and looked a few mappings to see if they seemed correct to me.

Should we target 8.4 with that change?

@nchaulet nchaulet added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team labels Aug 1, 2022
@nchaulet nchaulet self-assigned this Aug 1, 2022
@nchaulet nchaulet force-pushed the feature-dynamic-template-mappings branch 2 times, most recently from 0cfbb09 to b658868 Compare August 1, 2022 20:22
@nchaulet nchaulet force-pushed the feature-dynamic-template-mappings branch from b658868 to 3fad1e9 Compare August 2, 2022 14:03
@@ -0,0 +1,48 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

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

I created that file to start moving mapping generation to his own module.

@nchaulet nchaulet added v8.5.0 bug Fixes for quality problems that affect the customer experience labels Aug 2, 2022
@nchaulet nchaulet marked this pull request as ready for review August 2, 2022 14:10
@nchaulet nchaulet requested a review from a team as a code owner August 2, 2022 14:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@kpollich kpollich self-requested a review August 2, 2022 14:29
@jsoriano
Copy link
Member

jsoriano commented Aug 2, 2022

That PR fix that, if a field is of type object and contains an object_type we generate a dynamic template, if the path do not contains a * we add .* at the end of the path.

How this was implemented in beats https://github.com/elastic/beats/blob/ea207346d651448b8917b0791b2b117b9f9b9212/libbeat/template/processor.go#L444

In a second PR, we will introduce a way to generate dynamic template just by adding a * in the property name.

@jsoriano Does it make sense to you?

Makes sense, yes, keep this separated as this is something new. Thanks!

"labels": {
"properties": {}
},
"*": {
Copy link
Member

Choose a reason for hiding this comment

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

This static field shouldn't be here, right? This creates a mapping for a field called literally cockroachdb.status.labels.*.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you are correct, I just pushed a fix for that we do not create mappings is there only dynamic template for that path.

Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Not an area of the Fleet codebase I'm too familiar with, but I think I understand everything happening here for the most part. 🚀

@nchaulet
Copy link
Member Author

nchaulet commented Aug 2, 2022

@kpollich Do you think we should backport this one to 8.4?

@kpollich
Copy link
Member

kpollich commented Aug 2, 2022

@nchaulet - Yes let's backport to 8.4

@nchaulet nchaulet added the v8.4.0 label Aug 2, 2022
@nchaulet
Copy link
Member Author

nchaulet commented Aug 2, 2022

@elasticmachine merge upstream

@jen-huang jen-huang added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Aug 2, 2022
@nchaulet nchaulet force-pushed the feature-dynamic-template-mappings branch from 2a22f69 to 4589dd9 Compare August 2, 2022 21:18
@nchaulet
Copy link
Member Author

nchaulet commented Aug 2, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @nchaulet

@nchaulet nchaulet merged commit 76c55a2 into elastic:main Aug 3, 2022
@nchaulet nchaulet deleted the feature-dynamic-template-mappings branch August 3, 2022 00:49
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 3, 2022
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Aug 3, 2022
…#137929)

(cherry picked from commit 76c55a2)

Co-authored-by: Nicolas Chaulet <nicolas.chaulet@elastic.co>
@jsoriano
Copy link
Member

jsoriano commented Aug 3, 2022

Thanks!

@andrewkroh
Copy link
Member

This is adding features and behavior that is not defined in the package-spec which undermines the value of working from a specification as our source of truth. Please go through the process to update package-spec so that everyone knows how to benefit from these useful changes.

@jsoriano
Copy link
Member

Thanks @andrewkroh for raising this up, I have created an issue to follow on this elastic/package-spec#393.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.4.0 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants