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

Metricbeat: Add mapping for docker metrics per cpu #6843

Merged
merged 2 commits into from
Apr 19, 2018

Conversation

jsoriano
Copy link
Member

Fixes #6785

@jsoriano jsoriano requested a review from ruflin April 12, 2018 15:21
@jsoriano jsoriano force-pushed the docker-per-cpu-fields-mapping branch from 2e3ea14 to 2025f9f Compare April 12, 2018 15:23
@ruflin
Copy link
Contributor

ruflin commented Apr 13, 2018

This will probably not work. You have to use dynamic templates for this: https://www.elastic.co/guide/en/elasticsearch/reference/6.2/dynamic-templates.html

So far the way we use dynamic templates is that we specify it as following:

    - name: labels
      type: object
      object_type: keyword
      description: >
        Image labels.

In the template this outputs:

        {
          "docker.image.labels": {
            "mapping": {
              "type": "keyword"
            },
            "match_mapping_type": "string",
            "path_match": "docker.image.labels.*"
          }
        },

I could be that for this case an extension to our template generation is needed. Best have a look at libbeat/template/processor.go and create a test in processor_test.go to see if you get the expected output.

@jsoriano jsoriano added the in progress Pull request is currently in progress. label Apr 13, 2018
@jsoriano jsoriano force-pushed the docker-per-cpu-fields-mapping branch from 2025f9f to b0fcb07 Compare April 13, 2018 17:14
@@ -242,12 +258,16 @@ func addDynamicTemplate(f *common.Field, properties common.MapStr, matchType str
if len(f.Path) > 0 {
path = f.Path + "."
}
path_match := path + f.Name
if !strings.ContainsRune(path_match, '*') {
path_match += ".*"

Choose a reason for hiding this comment

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

don't use underscores in Go names; var path_match should be pathMatch

@@ -242,12 +258,16 @@ func addDynamicTemplate(f *common.Field, properties common.MapStr, matchType str
if len(f.Path) > 0 {
path = f.Path + "."
}
path_match := path + f.Name

Choose a reason for hiding this comment

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

don't use underscores in Go names; var path_match should be pathMatch

@jsoriano jsoriano force-pushed the docker-per-cpu-fields-mapping branch 3 times, most recently from 11b7a99 to e144525 Compare April 13, 2018 17:19
@jsoriano
Copy link
Member Author

Trying to generate the dynamic mapping, still have to test it for real.

@jsoriano jsoriano force-pushed the docker-per-cpu-fields-mapping branch from e144525 to 80a4e3c Compare April 13, 2018 17:28
@@ -208,6 +209,21 @@ func (p *Processor) object(f *common.Field) common.MapStr {
}

switch f.ObjectType {
case "scaled_float":
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we will have a request for float and integer here pretty soon. As soon this pops up we should refactor the code to remove the duplicated parts, if possible

Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw further below that long we already have. float should be rather simple to add in the same manner.

Copy link
Member Author

Choose a reason for hiding this comment

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

For scaled float I could reuse the existing scaledFloat method.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can still clean up the code in a follow up PR.

@ruflin
Copy link
Contributor

ruflin commented Apr 16, 2018

Change looks good to, looking forward to your manual testing. The template can be exported with metricbeat export template.

@jsoriano jsoriano force-pushed the docker-per-cpu-fields-mapping branch 3 times, most recently from 74fe717 to 333aa1c Compare April 16, 2018 15:09
@@ -208,6 +209,9 @@ func (p *Processor) object(f *common.Field) common.MapStr {
}

switch f.ObjectType {
case "scaled_float":
dynProperties = p.scaledFloat(f)
addDynamicTemplate(f, dynProperties, matchType("*"))
Copy link
Member Author

@jsoriano jsoriano Apr 16, 2018

Choose a reason for hiding this comment

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

I use * by default because we want it to match at least all numeric types.

@jsoriano
Copy link
Member Author

jenkins, test it again please, it seems that there has been an issue with pypi

@jsoriano
Copy link
Member Author

jenkins, test it

@jsoriano
Copy link
Member Author

jenkins, test this again

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Did you manage to test this locally? Code LGTM. PR will need need a rebase because of the CHANGELOG.

@@ -208,6 +209,21 @@ func (p *Processor) object(f *common.Field) common.MapStr {
}

switch f.ObjectType {
case "scaled_float":
Copy link
Contributor

Choose a reason for hiding this comment

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

We can still clean up the code in a follow up PR.

@jsoriano jsoriano force-pushed the docker-per-cpu-fields-mapping branch from 333aa1c to 9553a5e Compare April 18, 2018 08:42
@jsoriano
Copy link
Member Author

Yes, I could test it locally, it works fine :)

Branch rebased.

@jsoriano jsoriano removed the in progress Pull request is currently in progress. label Apr 18, 2018
@@ -79,6 +79,8 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di
- Fix Kubernetes calculated fields store. {pull}6564{6564}
- Exclude bind mounts in fsstat and filesystem metricsets. {pull}6819[6819]
- Don't stop Metricbeat if aerospike server is down. {pull}6874[6874]
- Fix Kubernetes calculated fields store. {pull}6564[6564]
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like that line of changelog sneaked in?

Copy link
Member Author

Choose a reason for hiding this comment

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

Umm, I don't know why this happens :? removing it

Copy link
Contributor

Choose a reason for hiding this comment

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

rebase with 3 way merges. Unfortunately a common issue.

@jsoriano jsoriano force-pushed the docker-per-cpu-fields-mapping branch from 9553a5e to b4ee1da Compare April 18, 2018 22:59
@ruflin ruflin merged commit 8e29a8b into elastic:master Apr 19, 2018
@ctindel
Copy link
Contributor

ctindel commented Apr 19, 2018

@ruflin Will this go into any of the 6.x releases as well?

@ruflin
Copy link
Contributor

ruflin commented Apr 19, 2018

@ctindel yes, 6.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants