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: support explicit dynamic templates #25422

Merged
merged 8 commits into from
May 6, 2021

Conversation

axw
Copy link
Member

@axw axw commented Apr 29, 2021

What does this PR do?

Add the field DynamicTemplate bool to mapping.Field, indicating that the field represents an explicitly named dynamic template. Update Elasticsearch template generation to create dynamic templates from these fields when the target Elasticsearch is 7.13.0+, when support for these dynamic templates was added and the requirement of match criteria was dropped.

Such dynamic templates are suitable only for use in dynamic_templates bulk request parameters and in ingest pipelines; they will have no path or type matching criteria.

Why is it important?

In APM we would like to be able to dynamically map histogram and summary metrics: elastic/apm-server#3195. To do this we will use the new (7.13.0+) feature of Elasticsearch where dynamic_templates can be specified by name at ingest time, to have explicit control over how dynamic mapping takes place. We will need to predefine dynamic templates for each combination of field type and unit that we support in dynamic metrics.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
    - [ ] I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Add the field `DynamicTemplate bool` to `mapping.Field`,
indicating that the field represents an explicitly named
dynamic template. Update Elasticsearch template generation
to create dynamic templates from these fields when the
target Elasticsearch is 7.13.0+, when support for these
dynamic templates was added and the requirement of match
criteria was dropped.

Such dynamic templates are suitable only for use in
dynamic_templates bulk request parameters and in ingest
pipelines; they will have no path or type matching criteria.
@axw axw added enhancement v7.14.0 Team:Elastic-Agent Label for the Agent team backport-v7.14.0 Automated backport with mergify labels Apr 29, 2021
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Apr 29, 2021
@axw axw marked this pull request as ready for review April 29, 2021 09:55
@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

@axw axw requested review from jsoriano and exekias April 29, 2021 09:55
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 29, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: axw commented: /test

  • Start Time: 2021-05-06T01:02:13.111+0000

  • Duration: 72 min 30 sec

  • Commit: 61fd80f

Test stats 🧪

Test Results
Failed 0
Passed 110
Skipped 0
Total 110

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 110
Skipped 0
Total 110

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thank you! 🚀 left a few comments

libbeat/template/processor.go Outdated Show resolved Hide resolved
libbeat/template/processor.go Show resolved Hide resolved
@axw axw requested a review from exekias April 30, 2021 03:20
@jsoriano
Copy link
Member

In APM we would like to be able to dynamically map histogram and summary metrics: elastic/apm-server#3195.

Do we expect differences on these dynamic maps between beats? or custom mappings of this kind per integration? If not, perhaps we could hardcode and always install the dynamic mapping for histograms and summaries (and other types we could need in the future), so we are sure that all collectors use the same approach to store these types of metrics. And we don't add more options to fields definition ymls.

@exekias
Copy link
Contributor

exekias commented Apr 30, 2021

Do we expect differences on these dynamic maps between beats? or custom mappings of this kind per integration? If not, perhaps we could hardcode and always install the dynamic mapping for histograms and summaries (and other types we could need in the future), so we are sure that all collectors use the same approach to store these types of metrics. And we don't add more options to fields definition ymls.

I have opened this conversation as a follow up of our latests metrics discussion: elastic/elasticsearch#72536. I think it's ok to keep going with these PRs as we work on getting better alignment

@jsoriano
Copy link
Member

Do we expect differences on these dynamic maps between beats? or custom mappings of this kind per integration? If not, perhaps we could hardcode and always install the dynamic mapping for histograms and summaries (and other types we could need in the future), so we are sure that all collectors use the same approach to store these types of metrics. And we don't add more options to fields definition ymls.

I have opened this conversation as a follow up of our latests metrics discussion: elastic/elasticsearch#72536. I think it's ok to keep going with these PRs as we work on getting better alignment

I was thinking in the option of including the default dynamic mapping for histograms and summaries in the base index template that libbeat generates, the same way as the strings_as_keyword dynamic mapping is included. This way it would be consistent for all agents, and we wouldn't need to add more options to fields definitions.

@axw
Copy link
Member Author

axw commented May 1, 2021

@jsoriano I think we should standardise, but I'm not sure about forcing it on every beat like that. As @exekias points out in elastic/elasticsearch#72536, we'll end up with many combinations for a completely general solution. So I think I'd rather wait for that conversation to play out before trying to standardise.

You make a good point about not adding yet more options to fields.yml though. Another option I considered was to enable beats to add dynamic templates in code, by adding a non-marshalled field to libbeat/template.TemplateConfig. Then in code, apm-server would do something like:

templateConfig.DynamicTemplates = map[string]mapping.Field{
    "histogram_counter_nanos": mapping.Field{Type: "histogram", MetricType: "counter", Unit: "nanos"},
}

It would not be possible to define these in fields.yml. We would need to come up with another solution for packages, such as Elasticsearch installing a default component template that defines the dynamic templates, and which packages could optionally include in data stream templates.

WDYT?

@mergify
Copy link
Contributor

mergify bot commented May 1, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fields-explicit-dynamic-templates upstream/fields-explicit-dynamic-templates
git merge upstream/master
git push upstream fields-explicit-dynamic-templates

@jsoriano
Copy link
Member

jsoriano commented May 3, 2021

You make a good point about not adding yet more options to fields.yml though. Another option I considered was to enable beats to add dynamic templates in code, by adding a non-marshalled field to libbeat/template.TemplateConfig. Then in code, apm-server would do something like:

templateConfig.DynamicTemplates = map[string]mapping.Field{
    "histogram_counter_nanos": mapping.Field{Type: "histogram", MetricType: "counter", Unit: "nanos"},
}

I like this option, this would be similar to what I was thinking of always adding these templates, but without forcing this to every beat. If at some point we find a general solution we can implement it the same way for every beat.

It would not be possible to define these in fields.yml. We would need to come up with another solution for packages, such as Elasticsearch installing a default component template that defines the dynamic templates, and which packages could optionally include in data stream templates.

Are we installing now the strings_as_keyword dynamic mapping in packages? If we are we could follow the same approach.

If we are going to wait to confirm a general solution maybe the approach of using fields.yml for this is better, specially thinking on giving the same solution in beats and in integrations. But if each package and beat can define its own dynamic mappings for these types we have the risk of having too many variants difficult to migrate when we agree on a general solution.

@axw
Copy link
Member Author

axw commented May 4, 2021

Are we installing now the strings_as_keyword dynamic mapping in packages? If we are we could follow the same approach.

Yes we do: https://github.com/elastic/kibana/blob/f736df990c0bf24c78ca02bb3c06d6276b7b706a/x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts#L407-L418

If we are going to wait to confirm a general solution maybe the approach of using fields.yml for this is better, specially thinking on giving the same solution in beats and in integrations. But if each package and beat can define its own dynamic mappings for these types we have the risk of having too many variants difficult to migrate when we agree on a general solution.

I can go either way. Speaking only for APM, I don't think it'll be a problem for us to change from one dynamic template name to another as we'll be doing the mapping in an ingest pipeline. Because the template and ingest pipeline are installed together we'll be able to change them at the same time.

@exekias @ruflin do you have an opinion on doing this in code vs. fields.yml for right now, and how it should be handled in the future?

@ruflin
Copy link
Contributor

ruflin commented May 4, 2021

I did not dig into the full conversation but for the fields.yml support, I think that is preferrable. Will these templates also be specified by packages in the future? If yes, I think fields.yml is our way to go and we need to bring support for it also to Fleet.

@exekias
Copy link
Contributor

exekias commented May 5, 2021

I think it's ok to move forward with this. We only have a few entry points where dynamic templates will be used for now, so things should be easy to maintain. Let's not wait on the other conversation as it may take some time to align and get the features we need.

Also an eventual update on namings will be transparent for the user, as we will be updating our code / mappings, but not the UX.

@axw something that would be interesting is to follow up with adding this to the package spec and Kibana so we can have dynamic templates in packages too. Do you plan to open issues for that?

@axw
Copy link
Member Author

axw commented May 5, 2021

something that would be interesting is to follow up with adding this to the package spec and Kibana so we can have dynamic templates in packages too. Do you plan to open issues for that?

Will do 👍

@axw
Copy link
Member Author

axw commented May 5, 2021

/test

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Ok, let's go on with this approach, thanks!

@axw axw dismissed exekias’s stale review May 5, 2021 09:36

comments addressed

@axw
Copy link
Member Author

axw commented May 5, 2021

/test

@axw
Copy link
Member Author

axw commented May 6, 2021

/test

@axw axw merged commit 7a49ca1 into elastic:master May 6, 2021
@axw axw deleted the fields-explicit-dynamic-templates branch May 6, 2021 02:23
mergify bot pushed a commit that referenced this pull request May 6, 2021
* libbeat: support explicit dynamic templates

Add the field `DynamicTemplate bool` to `mapping.Field`,
indicating that the field represents an explicitly named
dynamic template. Update Elasticsearch template generation
to create dynamic templates from these fields when the
target Elasticsearch is 7.13.0+, when support for these
dynamic templates was added and the requirement of match
criteria was dropped.

Such dynamic templates are suitable only for use in
dynamic_templates bulk request parameters and in ingest
pipelines; they will have no path or type matching criteria.

* Update changelog

* Revert signature change

* Disallow dynamic_template with object_type_params

* Add a constant for min version

* libbeat/mapping: fix field validation

(cherry picked from commit 7a49ca1)
axw added a commit that referenced this pull request May 6, 2021
* libbeat: support explicit dynamic templates

Add the field `DynamicTemplate bool` to `mapping.Field`,
indicating that the field represents an explicitly named
dynamic template. Update Elasticsearch template generation
to create dynamic templates from these fields when the
target Elasticsearch is 7.13.0+, when support for these
dynamic templates was added and the requirement of match
criteria was dropped.

Such dynamic templates are suitable only for use in
dynamic_templates bulk request parameters and in ingest
pipelines; they will have no path or type matching criteria.

* Update changelog

* Revert signature change

* Disallow dynamic_template with object_type_params

* Add a constant for min version

* libbeat/mapping: fix field validation

(cherry picked from commit 7a49ca1)

Co-authored-by: Andrew Wilkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.14.0 Automated backport with mergify enhancement Team:Elastic-Agent Label for the Agent team v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants