Make elasticsearch/index_summary metricset work for Stack Monitoring without xpack.enabled flag by sayden · Pull Request #20615 · elastic/beats

sayden · 2020-08-14T17:22:00Z

Code should be working 😅

Missing steps:

Add description to new fields (marked as TODO)
Fix some comments

…ases and new metricset fields. No code changes yet.

metricbeat/module/elasticsearch/index_summary/data.go

ycombinator · 2020-08-14T17:24:55Z

metricbeat/module/elasticsearch/index_summary/data.go

+					},
+				},
+				// following field is not included in the Stack Monitoring UI mapping
+				"is_throttled": c.Bool("is_throttled"),


If the field is not needed by the Stack Monitoring UI (@chrisronline may want to confirm) and it wasn't a field we were already collecting in data.go, then it's safe to not collect it at all. So I would remove this line and others like it.

This is correct. We need to not only collect the mappings, but also continue to collect and index any other fields we used to because we often read values from the source document that weren't considered when the finalized mappings were produced.

@chrisronline So just to clarify, you're saying we should collect this field, is_throttled, but we should not include it in the fields.yml so it doesn't show up in the mappings, right?

Understood 🙂

We don't really even want to index fields that aren't used and we could take another pass on which fields we read from source directly, but we're also currently in talks about potentially indexing as much data as possible (even if the stack monitoring UI doesn't use it) to enable users to build custom dashboards. Nothing is decided but I'd rather leave the functionality as is in case we do go down that path

Wait a sec 😄 as far as I understood, we should map this fields in elasticsearch fields.yml mapping. But should we still map it in the metricset fields.yml file? As a Metricbeat convention, it must be mapped, AFAIK 100% of Metricbeat fields are map in fields.yml of their respective metricsets. Metricbeat is not going to fail though, we however have a commonly used python test (like this one in MySQL module https://github.com/elastic/beats/blob/master/metricbeat/module/mysql/test_mysql.py#L47) to check if all fields are map in fields.yml but fortunately it's not included in the python test code for the elasticsearch module.

.monitoring-* indices have dynamic mapping set to false, but perhaps metricbeat-* handles this differently? In that case, maybe we should add every field we are indexing to fields.yml but if the field isn't contained in the used mappings list, we set the mapping to enabled: false?

@sayden I can try to help clarify about when and where to map fields for stack monitoring metricsets. 😅

Consider a field, say elasticsearch.<foo>.bar.baz that is newly (i.e. not already in master) being collected by the elasticsearch/<foo> metricset in data.go. For this field:

Go ahead and define the field in the module/elasticsearch/<foo>/_meta/fields.yml file.

Now, check if the field has a corresponding field in the .monitoring-es-* index mappings.

If yes: let's say the corresponding field in .monitoring-es-* is some_thing.some_thing_else.qux.boom. Go ahead and define a field alias from some_thing.some_thing_else.qux.boom => elasticsearch.<foo>.bar.baz in the module/elasticsearch/_meta/fields.yml file.

If no: edit the field mapping you created in step 1 in module/elasticsearch/<foo>/_meta/fields.yml and add enabled: false to it. This will ensure that the field is mapped (so the assert_fields_are_documented test assertion you are talking about in your comment will pass) but not be unnecessarily indexed.

Hopefully this now clear as mud 😃.

Good idea, @chrisronline re: enabled: false. I will edit my comment above to reflect this step.

ycombinator · 2020-08-14T17:28:22Z

@sayden Just a heads up that I made a mistake in the node_stats PR, which I'm trying to fix in #20613. As you can see, tests are failing on that PR right now which I need to address. I'm not sure yet what those fixes are going to look like. But just wanted to let you know so you are aware that you might need similar fixes in this PR (and future PRs for other stack monitoring metricsets) as well.

elasticmachine · 2020-08-14T17:35:45Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: [Pull request #20615 updated]
Start Time: 2020-11-06T16:43:32.091+0000
Duration: 69 min 37 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2250
Skipped	500
Total	2750

- Rename index_summary to index.summary in elasticsearch fields.yml - Remove call to eventsMappingXPack

- Add a missing comment about a non used field in data.go - Run make update

ycombinator · 2020-08-14T23:23:36Z

@sayden As part of this PR you will also need to make this metricset one of the metricsets that's enabled by default when the user runs metricbeat modules enable elasticsearch. This way, it's minimal work for a user to use the elasticsearch module for Stack Monitoring.

sayden · 2020-08-20T12:41:17Z

I'm trying to make CI work but it's complaining about field elasticsearch.index.summary.primaries.store.size.bytes not documented, but it's here https://github.com/elastic/beats/pull/20615/files#diff-3c4851b3e8837e18ba25dc267cf10492R18

ycombinator · 2020-08-20T13:49:22Z

@sayden Can you check whether the field shows up if you run the following?

mage update build
./metricbeat export template | jq '.mappings.properties.elasticsearch.properties.index.properties.summary'

If it doesn't, then there's something wrong in the fields.yml for the metricset. Being YAML it could even be a slight mis-formatting that can cause this problem. One way would be to comment out most of your fields in fields.yml, repeat the above commands to make sure what you have uncommented shows up in the template, then uncomment some more fields, and so on, until you've isolated the problem area and can fix it.

sayden · 2020-08-24T21:21:35Z

Okay, I fixed the missing field thing thanks to the comment of @ycombinator I just need to fix the Go integration test now

elasticmachine · 2020-08-25T17:18:46Z

Pinging @elastic/integrations-services (Team:Services)

sayden · 2020-08-25T17:19:10Z

Integration test finished and current CI error is not related! 🎉

metricbeat/module/elasticsearch/index_summary/data.go

ycombinator

Code looks good, just left a few minor comments.

chrisronline · 2020-08-25T18:12:55Z

I'm seeing an issue here.

After running this PR, this query:

POST metricbeat-*/_search?filter_path=hits.hits._source.elasticsearch.index.summary.total.search
{
  "size": 1,
  "sort": [
    {
      "timestamp": {
        "order": "desc"
      }
    }
  ],
  "query": {
    "term": {
      "metricset.name": {
        "value": "index_summary"
      }
    }
  }
}

Returns an empty block:

{
  "hits" : {
    "hits" : [
      {
        "_source" : {
          "elasticsearch" : {
            "index" : {
              "summary" : {
                "total" : {
                  "search" : {
                    "query" : {
                      "time" : { }
                    }
                  }
                }
              }
            }
          }
        }
      }
    ]
  }
}

However, indices_stats._all.total.search.query_total aliases into elasticsearch.index.summary.total.search.query.count but it doesn't seem any data is being indexed in that location.

Maybe I'm missing something?

sayden · 2020-08-25T19:07:12Z

@chrisronline you are right. After taking a look I have just realized that there's something missing. https://github.com/elastic/beats/pull/20615/files#diff-ecd66d9e3da80b956adc12f104330b4cR90 I'll take a look

sayden · 2020-08-25T21:00:29Z

Okay! Thanks for the help @chrisronline ! Now it should be ok.

{
  "hits" : {
    "hits" : [
      {
        "_source" : {
          "elasticsearch" : {
            "index" : {
              "summary" : {
                "total" : {
                  "search" : {
                    "query" : {
                      "count" : 56,
                      "time" : {
                        "ms" : 585
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }
    ]
  }
}

chrisronline

LGTM! All the necessary fields have data populated and the ES cluster overview charts look right!

metricbeat/module/elasticsearch/index_summary/data.go

ycombinator · 2020-08-27T10:10:06Z

It looks like we are no longer using elasticsearch.BulkStatsDict. Remove it?

sayden · 2020-08-27T12:11:15Z

I have moved elasticsearch.BulkStatsDict to data.go file :)

ycombinator

LGTM. Thanks for taking this on and iterating through it, @sayden!

chrisronline · 2020-09-08T18:59:18Z

Hey folks, any update on this? It looks ready to go but hasn't been merged yet.

ycombinator · 2020-09-09T09:00:41Z

@chrisronline @sayden was out all last week so he's probably catching up on backlog.

@sayden CI failures look unrelated but Beats master branch CI has been passing recently. So maybe rebase this on master and see if that gets CI on this PR to green before merging?

elasticmachine · 2020-09-11T00:58:12Z

Pinging @elastic/stack-monitoring (Stack monitoring)

sayden · 2020-11-04T15:14:39Z

@ycombinator do you mean to rebase the feature branch to master I guess?

ycombinator · 2020-11-05T04:17:49Z

@ycombinator do you mean to rebase the feature branch to master I guess?

Yeah, I guess you'll need to do both:

Rebase the elastic:feature-stack-monitoring-mb-ecs on master so it's up to date, then
Rebase this PR's branch on elastic:feature-stack-monitoring-mb-ecs so it's up to date as well.

…csearch/index_summary-xpack-flag

elasticmachine · 2020-11-05T12:52:19Z

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	2250
Skipped	500
Total	2750

…without xpack.enabled flag (#20615)

…without xpack.enabled flag (elastic#20615)

sayden added 2 commits August 14, 2020 19:18

Bring into eventsMapping all fields in eventsMappingXpack, create ali…

153b386

…ases and new metricset fields. No code changes yet.

Code modified. No changes in eventMapping function seemed necessary

faa8ea1

sayden added the Team:Services (Deprecated) Label for the former Integrations-Services team label Aug 14, 2020

sayden requested review from chrisronline and ycombinator August 14, 2020 17:22

sayden self-assigned this Aug 14, 2020

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 Aug 14, 2020

sayden commented Aug 14, 2020

View reviewed changes

metricbeat/module/elasticsearch/index_summary/data.go Show resolved Hide resolved

sayden commented Aug 14, 2020

View reviewed changes

metricbeat/module/elasticsearch/index_summary/data.go Outdated Show resolved Hide resolved

ycombinator reviewed Aug 14, 2020

View reviewed changes

sayden added 2 commits August 14, 2020 19:39

- Fix indentation issue

209462a

- Rename index_summary to index.summary in elasticsearch fields.yml - Remove call to eventsMappingXPack

- Remove aliases from non used fields in the elasticsearch mapping

b6e407c

- Add a missing comment about a non used field in data.go - Run make update

sayden marked this pull request as ready for review August 25, 2020 17:18

ycombinator reviewed Aug 25, 2020

View reviewed changes

metricbeat/module/elasticsearch/index_summary/data.go Outdated Show resolved Hide resolved

ycombinator reviewed Aug 25, 2020

View reviewed changes

metricbeat/module/elasticsearch/index_summary/data.go Outdated Show resolved Hide resolved

ycombinator reviewed Aug 25, 2020

View reviewed changes

metricbeat/module/elasticsearch/index_summary/data.go Outdated Show resolved Hide resolved

ycombinator reviewed Aug 25, 2020

View reviewed changes

chrisronline approved these changes Aug 26, 2020

View reviewed changes

ycombinator reviewed Aug 27, 2020

View reviewed changes

metricbeat/module/elasticsearch/index_summary/data.go Outdated Show resolved Hide resolved

ycombinator approved these changes Aug 27, 2020

View reviewed changes

ycombinator added the Feature:Stack Monitoring label Sep 11, 2020

sayden added 12 commits November 5, 2020 12:31

Merge branch 'feature-stack-monitoring-mb-ecs' into feature/mb/elasti…

b43e1f2

…csearch/index_summary-xpack-flag

Reorder fields.yml?

22c52f6

Fix missing field

031d91c

Add another missing field

78165c4

Updated data.json

b57b32d

Fix go integration test

1993d99

mage fmt update

05cc97d

Fix incorrect dict

fd2e40b

Update data.json

64635db

Remove bulkstats dict from elasticsearch.go file

b67ca49

Remove data-xpack.json file

35a7209

Fix conflict in fields.go

c4934af

sayden force-pushed the feature/mb/elasticsearch/index_summary-xpack-flag branch from c2ba106 to c4934af Compare November 5, 2020 11:32

Remove comment

2abc9ae

Fix lint

ab25c29

sayden merged commit 58e2a7d into elastic:feature-stack-monitoring-mb-ecs Nov 10, 2020

sayden added a commit that referenced this pull request Nov 12, 2020

Make elasticsearch/index_summary metricset work for Stack Monitoring …

9de5959

…without xpack.enabled flag (#20615)

sayden mentioned this pull request Apr 27, 2021

[Metricbeat] Remove xpack enabled flag on ES, Logstash, Beats and Kibana #24427

Merged

6 tasks

leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023

Make elasticsearch/index_summary metricset work for Stack Monitoring …

e5e28b4

…without xpack.enabled flag (elastic#20615)

Conversation

sayden commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator commented Aug 14, 2020

Uh oh!

elasticmachine commented Aug 14, 2020 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Uh oh!

ycombinator commented Aug 14, 2020

Uh oh!

sayden commented Aug 20, 2020

Uh oh!

ycombinator commented Aug 20, 2020

Uh oh!

sayden commented Aug 24, 2020

Uh oh!

elasticmachine commented Aug 25, 2020

Uh oh!

sayden commented Aug 25, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ycombinator left a comment

Choose a reason for hiding this comment

Uh oh!

chrisronline commented Aug 25, 2020

Uh oh!

sayden commented Aug 25, 2020

Uh oh!

sayden commented Aug 25, 2020

Uh oh!

chrisronline left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ycombinator commented Aug 27, 2020

Uh oh!

sayden commented Aug 27, 2020

Uh oh!

ycombinator left a comment

Choose a reason for hiding this comment

Uh oh!

chrisronline commented Sep 8, 2020

Uh oh!

ycombinator commented Sep 9, 2020

Uh oh!

elasticmachine commented Sep 11, 2020

Uh oh!

sayden commented Nov 4, 2020

sayden commented Aug 14, 2020 •

edited

Loading

ycombinator Aug 14, 2020 •

edited

Loading

ycombinator Aug 14, 2020 •

edited

Loading

elasticmachine commented Aug 14, 2020 •

edited by jenkins-beats-ci bot

Loading