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

Make elasticsearch/shard metricset work for Stack Monitoring without xpack.enabled flag #21389

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Sep 29, 2020

Shard metricset maintains most of the Xpack flag functionality (indexing by the hash id created in the event) but includes now non-xpack things too.

@sayden sayden added Metricbeat Metricbeat Feature:Stack Monitoring Team:Services (Deprecated) Label for the former Integrations-Services team labels Sep 29, 2020
@sayden sayden self-assigned this Sep 29, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/stack-monitoring (Stack monitoring)

@elasticmachine
Copy link
Collaborator

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

@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 Sep 29, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 29, 2020

💚 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: Pull request #21389 updated

  • Start Time: 2020-12-15T11:52:31.526+0000

  • Duration: 66 min 17 sec

Test stats 🧪

Test Results
Failed 0
Passed 2312
Skipped 532
Total 2844

Steps errors 2

Expand to view the steps failures

Terraform Apply on x-pack/metricbeat/module/aws
  • Took 0 min 15 sec . View more details on here
Terraform Apply on x-pack/metricbeat/module/aws
  • Took 0 min 15 sec . View more details on here

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 2312
Skipped 532
Total 2844

@sayden
Copy link
Contributor Author

sayden commented Oct 28, 2020

Not sure if I understand. In the mapping it is called shard https://gist.github.com/chrisronline/3b123c85d03b97ec3cbf63d9d72b3e2c#file-mb_mappings-json-L1246 and in the past it was shard too https://github.com/elastic/beats/blob/master/metricbeat/module/elasticsearch/shard/_meta/fields.yml#L1

I don't mind to change it, I just don't know where to change it 😄

@chrisronline
Copy link
Contributor

I don't really know how the code works, but when I run the elasticsearch-xpack metricbeat module, and run this query:

POST .monitoring-es-*/_search?filter_path=aggregations.types.buckets
{
  "size": 0,
  "aggs": {
    "types": {
      "terms": {
        "field": "type",
        "size": 10
      }
    }
  }
}

I see:

{
  "aggregations" : {
    "types" : {
      "buckets" : [
        {
          "key" : "index_stats",
          "doc_count" : 4902
        },
        {
          "key" : "ccr_auto_follow_stats",
          "doc_count" : 961
        },
        {
          "key" : "cluster_stats",
          "doc_count" : 961
        },
        {
          "key" : "indices_stats",
          "doc_count" : 961
        },
        {
          "key" : "enrich_coordinator_stats",
          "doc_count" : 941
        },
        {
          "key" : "index_recovery",
          "doc_count" : 620
        },
        {
          "key" : "shards",
          "doc_count" : 294
        }
      ]
    }
  }
}

@sayden
Copy link
Contributor Author

sayden commented Nov 3, 2020

Ok, thanks for explaining. I think I understand what the problem is. If I understood it correctly, I see 2 options:

  1. use event.dataset == "elasticsearch.shard" which will return all shard documents. Similar to the current "type": "shards".
  2. I create an key in elasticsearch.type: "shards" field in the module to achieve the same thing.

Maybe @ycombinator has something to add too.

In general, no root "type": "shards" key is allowed in Metricbeat. I don't know if any of those ideas are an option in the UI

@elasticmachine
Copy link
Collaborator

🐛 Flaky test report

❕ There are test failures but not known flaky tests.

Expand to view the summary

Test stats 🧪

Test Results
Failed 3
Passed 2241
Skipped 478
Total 2722

Genuine test errors 3

💔 There are test failures but not known flaky tests, most likely a genuine test failure.

    * **Name**: `Build&Test / metricbeat-goIntegTest / TestXPackEnabled – elasticsearch`
    * **Name**: `Build&Test / metricbeat-goIntegTest / TestXPackEnabled/shard – elasticsearch`
    * **Name**: `Build&Test / metricbeat-pythonIntegTest / test_metricsets_8_shard – metricbeat.module.elasticsearch.test_elasticsearch.Test`

@ycombinator
Copy link
Contributor

Re: the type: shards vs. type: shard discussion, I was looking at a discussion we had in the past about aliasing the root-level type field: #19747 (comment). In that discussion we concluded that the UI would not be relying on the type field but instead look at metricset.name.

The problem, of course, is that in the case of the elasticsearch/shard metricset, the value of metricset.name is set to shard when the UI really wants it to be shards. I guess up until this PR we just hadn't encountered such a discrepancy so looking at metricset.name in the UI code was okay.

To solve this issue in this PR but also in general, I like this idea:

I create an key in elasticsearch.type: "shards" field in the module to achieve the same thing.

This would mean:

  • In the elasticsearch module-level fields.yml (somewhere under here), we would define a type field.
  • Every elasticsearch module's metricset would need to set a value for this new elasticsearch.type field. In many/most cases this would end up being the same value as metricset.name but in some cases (like this PR), it would differ. The main thing is that the value of this field should be what the Stack Monitoring UI expects in the type field today (with xpack.enabled: true set).
  • The UI code should stop looking at metricset.name and instead start looking at elasticsearch.type.

@sayden @chrisronline WDYT?

@chrisronline
Copy link
Contributor

Would it be easier if the UI code could just adapt to the documents being either shard or shards?

@ycombinator
Copy link
Contributor

Would it be easier if the UI code could just adapt to the documents being either shard or shards?

If that's not difficult to do in the UI then sure! Also, before you do that, we should probably check all Stack Monitoring metricsets to see where else you might need to play this game. And then check how easy those would be to adapt in the UI as well. @sayden can you come up this information — basically the value of type vs. metricset.name for each Stack Monitoring metricset, highlighting the ones that are different.

@chrisronline
Copy link
Contributor

And then check how easy those would be to adapt in the UI as well.

I can already tell you this will be easy. We can easily adapt https://github.com/elastic/kibana/blob/master/x-pack/plugins/monitoring/server/lib/create_query.js#L59 to support an array, just like we do now with type and metricbeat.name. We'll have to handle structural changes individually, but the shards structure change is do-able on the UI side.

@ycombinator
Copy link
Contributor

@chrisronline thanks for the link. I understand now — you'll just add more clauses to the should as needed for special cases like type: shards. I know this isn't strictly your domain so feel free to pull someone else in if needed, but do you think there might be any issues with Telemetry code? Also, what about Cluster Alert watches, or are those gone now with the switch to Kibana Alerting?

Assuming all of the above checks out, I think the impact on this PR is a noop. That is, Metricbeat can keep sending metricset.name: shard.

@chrisronline
Copy link
Contributor

but do you think there might be any issues with Telemetry code?

AFAIK, the telemetry code only concerns itself with cluster_stats documents (see here)

Also, what about Cluster Alert watches, or are those gone now with the switch to Kibana Alerting?

These should be going away in 7.11, with the ES team providing an API to permanently remove and disable the creation of new ones. However, we currently do not have a cluster alert that looks at this document type.

I'll work on the Kibana PR to handle both shard and shards.

@chrisronline
Copy link
Contributor

One question - should I be seeing alias fields for this? I don't see anything setup for shard or shards yet.

@sayden
Copy link
Contributor Author

sayden commented Nov 11, 2020

Yes, I will also add an alias for this one, sorry for the delay!

"source_node": {
"name": "1fb2aa83efac",
"uuid": "hx-oJ1-aT_-5pRG22JMI1Q"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not see the source_node.* fields mentioned in the metricset's fields.yml. Should they be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I completely forgot that I have to deactivate the test to check if all fields were documented.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

Left one comment/question about fields potentially needing to be defined in the metricset's fields.yml.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

…csearch/shard_xpack_flag

# Conflicts:
#	metricbeat/module/elasticsearch/fields.go
#	metricbeat/module/elasticsearch/ml_job/data.go
@sayden sayden merged commit 2c5742f into elastic:feature-stack-monitoring-mb-ecs Dec 15, 2020
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Stack Monitoring Metricbeat Metricbeat Team:Services (Deprecated) Label for the former Integrations-Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants