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

Collect bulk indexing stats for Elasticsearch metricsets #17992

Merged
merged 9 commits into from
May 5, 2020

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Apr 25, 2020

What does this PR do?

Collects new bulk indexing metrics being reported by Elasticsearch in the index, index_summary, and node_stats metricsets for the Stack Monitoring code path (i.e. when xpack.enabled: true is set in these metricsets' configurations).

These metricsets correspond to type:index_stats, type:indices_stats, and type:node_stats documents, respectively, in .monitoring-es-* indices used by the Stack Monitoring UI.

Why is it important?

To restore parity between legacy internal collection and Metricbeat collection for Elasticsearch monitoring. The changes corresponding to this PR for legacy internal collection were made in elastic/elasticsearch#52208.

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.
    • In addition to the unit test added in this PR, parity tests already exist that are currently failing and should start passing once the changes in this PR are released.
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

  1. Run the latest 8.0.0-SNAPSHOT build of Elasticsearch so it exposes the new bulk indexing metrics being collected by the code in this PR.

    docker pull docker.elastic.co/elasticsearch/elasticsearch:8.0.0-SNAPSHOT
    docker run --name elasticsearch -p 9200:9200 -e "discovery.type=single-node" docker.elastic.co/elasticsearch/elasticsearch:8.0.0-SNAPSHOT
    
  2. Build Metricbeat with the changes in this PR.

    cd $GOPATH/src/github.com/elastic/beats/metricbeat
    mage build
    
  3. Enable the elasticsearch-xpack module.

    metricbeat modules enable elasticsearch-xpack
    
  4. Create a minimal Metricbeat configuration for testing.

    cat <<EOF > ./metricbeat.test.yml
    metricbeat.config.modules:
      path: \${path.config}/modules.d/*.yml
      reload.enabled: false
    
    output.console.enabled: true
    
    monitoring.elasticsearch:
      hosts: [ "localhost:9200" ]
    EOF
    

    Here, we only enable monitoring.elasticsearch to generate some bulk indexing requests to Elasticsearch so we can see non-zero bulk metrics.

  5. Run Metricbeat with the minimal configuration and look for the new bulk indexing metrics' fields. You may need to run these for ~30 seconds for some bulk indexing to happen and corresponding metrics to be generated by Elasticsearch.

    1. For the index metricset, i.e.type:index_stats documents:
      ./metricbeat -c metricbeat.test.yml | jq 'select(.type == "index_stats") | .index_stats.total.bulk'
      
      ./metricbeat -c metricbeat.test.yml | jq 'select(.type == "index_stats") | .index_stats.primaries.bulk'
      
    2. For the index_summary metricset, i.e.type:indices_stats documents:
      ./metricbeat -c metricbeat.test.yml | jq 'select(.type == "indices_stats") | .indices_stats._all.total.bulk'
      
      ./metricbeat -c metricbeat.test.yml | jq 'select(.type == "indices_stats") | .indices_stats._all.primaries.bulk'
      
    3. For the node_stats metricset, i.e.type:node_stats documents:
      ./metricbeat -c metricbeat.test.yml | jq 'select(.type == "node_stats") | .node_stats.indices.bulk'
      

Related issues

@ycombinator ycombinator added Feature:Stack Monitoring Team:Services (Deprecated) Label for the former Integrations-Services team labels Apr 25, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/stack-monitoring (Stack monitoring)

@elasticmachine
Copy link
Collaborator

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

@ycombinator ycombinator added Metricbeat Metricbeat module needs_backport PR is waiting to be backported to other branches. v7.8.0 v8.0.0 [zube]: In Review labels Apr 25, 2020
@ycombinator
Copy link
Contributor Author

@igoristic I've requested your review on this PR as a functional reviewer. Please see the "How to test this PR locally" section in the PR description for instructions on how to functionally test this PR.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 30, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview stats

Expand to view the summary

Build stats

Test stats 🧪

Test Results
Failed 0
Passed 3008
Skipped 744
Total 3752

@jsoriano
Copy link
Member

Failure in statsd tests should be fixed by #18130, the other failures may be related.

@ycombinator
Copy link
Contributor Author

Looking at the CI errors from the elasticsearch module system tests, they are legitimate and need to be addressed in this PR.

This PR adds bulk to the list of metrics requested from the Elasticsearch Stats API. This metric is only available starting Elasticsearch 7.8.0. However, our tests use Elasticsearch 7.5.2. Which is why the tests are failing.

To address this correctly, we need to add bulk to the list of metrics requested from the Elasticsearch Stats API if the Elasticsearch version is >= 7.8.0. This will be similar to the work done recently in the logstash module in #17497.

Accordingly, I'm moving this PR back into draft state. Once I've implemented the necessary changes, I'll put it back into review.

@ycombinator ycombinator marked this pull request as draft April 30, 2020 19:20
@ycombinator ycombinator added in progress Pull request is currently in progress. and removed [zube]: In Review labels Apr 30, 2020
@ycombinator ycombinator marked this pull request as ready for review April 30, 2020 21:33
@igoristic
Copy link

Looking at the CI errors from the elasticsearch module system tests, they are legitimate and need to be addressed in this PR ...

Yeah I was wondering if I should wait for the CI to turn green before I can test it

@ycombinator
Copy link
Contributor Author

Yeah I was wondering if I should wait for the CI to turn green before I can test it

That would be ideal but there might be unrelated test failures. I'll ping you here where it's ready to test.

@ycombinator
Copy link
Contributor Author

@igoristic CI is looking good now. There are failures but they are unrelated to this PR. Please go ahead and test it. Thanks!

Copy link

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

Tested all case scenarios mentioned in step 5, and those fields are no longer null (like they are in master):
Screen Shot 2020-05-05 at 2 42 42 AM

Thank you for fixing this @ycombinator 🙇

@ycombinator ycombinator merged commit c87c8e7 into elastic:master May 5, 2020
@ycombinator ycombinator deleted the mb-es-xp-fix-parity branch May 5, 2020 08:52
ycombinator added a commit that referenced this pull request May 5, 2020
…8229)

* Collecting new index_stats bulk metrics

* Collecting new indices_stats bulk metrics

* Collecting new node_stats bulk metrics

* Adding CHANGELOG entry

* Request bulk stats

* Request bulk metrics only if supported

* Fixing code and tests

* Fixing code so only service URI path is replaced

* Updating unit tests
TotalOperations int `json:"total_operations"`
TotalTimeInMillis int `json:"total_time_in_millis"`
TotalSizeInBytes int `json:"total_size_in_bytes"`
AvgTimeInMillis int `json:"throttle_time_in_millis"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@ycombinator Is this a typo? Should it be json:"avg_time_in_millis"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should 😞. Good eye, nice catch! I assume parity tests failed? I'll put up a PR to fix it. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR is up: #18420

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Stack Monitoring in progress Pull request is currently in progress. Metricbeat Metricbeat module needs_backport PR is waiting to be backported to other branches. Team:Services (Deprecated) Label for the former Integrations-Services team v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metricbeat][parity-tests] Missing some newly added ES fields
6 participants