Skip to content

Elasticsearch Package #1365

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

Merged
merged 20 commits into from
Aug 12, 2021
Merged

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Jul 28, 2021

This PR migrates the elasticsearch filebeat and metricbeat module into a package.

I'll open it for review but I wouldn't be surprised if some thing aren't totally correct because the migration process for this particular package was very error prone with many manual steps.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.

Reviewers checklist

The migration of this package required some big amount of custom stuff, I'm gonna write it here:

  • This module had a huge amount of aliases at the module level. Those that weren't relevant for a particular metricset have been manually removed.
  • Aliases are related to Stack Monitoring UI stuff so they have been removed from the docs as they are irrelevant for users. The downside is that I had to manually write them in the _dev/build/docs/README.md to avoid that they are rewritten during the build proccess. This means they'll need to be updated manually.
  • All references to metricbeat, filebeat, metricsets and filesets have been removed from the docs

Screenshots

image

image

image

@sayden sayden added enhancement New feature or request Team:Integrations Label for the Integrations team Team:Services (Deprecated) Label for the former Integrations-Services team labels Jul 28, 2021
@sayden sayden self-assigned this Jul 28, 2021
@elasticmachine
Copy link

elasticmachine commented Jul 28, 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 preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-12T10:01:26.321+0000

  • Duration: 12 min 32 sec

  • Commit: 8d8cd47

Test stats 🧪

Test Results
Failed 0
Passed 31
Skipped 0
Total 31

Trends 🧪

Image of Build Times

Image of Tests

@sayden sayden marked this pull request as ready for review August 4, 2021 12:05
@elasticmachine
Copy link

Pinging @elastic/integrations (Team:Integrations)

@elasticmachine
Copy link

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

@mtojek mtojek self-requested a review August 5, 2021 08:35
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Left few nit-picks to fine-tune. I guess you can pull it in and add more changes/tests in iterations.

Comment on lines 26 to 27
title: Collect Elasticsearch ccr, cluster_stats, enrich, index, index_recovery, index_summary, ml_job, node, node_stats, pending_tasks and shard metrics
description: Collecting ccr, cluster_stats, enrich, index, index_recovery, index_summary, ml_job, node, node_stats, pending_tasks and shard metrics from Elasticsearch instances
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to transform it to a more human readable form and rephrase titles and descriptions :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left simple titles now, like "Collect elasticsearch logs" or "Collect elasticsearch metrics". The description has a longer text but still human readable and I wrote some descriptions on the data_streams themselves 👍

license: basic
categories: ["elastic_stack"]
conditions:
kibana.version: ^7.10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please bump up the constraint to 7.15. We don't want this package to be available for earlier versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I wrote ^7.15.0 now, I really didn't know which was is the correct.

name: elasticsearch
title: Elasticsearch
version: 0.1.0
release: beta
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe mark it as experimental as it's still early stage and lot of stuff to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I read somewhere that all packages must be released as beat and that's why I changed it from experimental to beta. No prob, I revert it back to experimental.

Bump version to 7.15
Rewrite title and descriptions for logs and metrics
@sayden
Copy link
Contributor Author

sayden commented Aug 10, 2021

I have also updated the screenshot to show the changes requested

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

A couple of nit picks

BTW I saw that every data stream starts with "Elasticsearch ..." prefix. I'm not sure if it isn't redundant as all these data streams are only related to Elasticsearch.

@@ -0,0 +1,15 @@
type: metrics
title: Elasticsearch index_recovery metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: index recovery

@@ -0,0 +1,7 @@
type: metrics
title: Elasticsearch cluster_stats metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: cluster stats

@sayden
Copy link
Contributor Author

sayden commented Aug 11, 2021

A couple of nit picks

BTW I saw that every data stream starts with "Elasticsearch ..." prefix. I'm not sure if it isn't redundant as all these data streams are only related to Elasticsearch.

Good point I was mostly following the naming of other modules I took as example.

@sayden sayden merged commit b338600 into elastic:master Aug 12, 2021
@sayden sayden deleted the migration/sm/elasticsearch branch August 26, 2021 16:09
@andrewkroh andrewkroh added Integration:elasticsearch Elasticsearch New Integration Issue or pull request for creating a new integration package. labels Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:elasticsearch Elasticsearch New Integration Issue or pull request for creating a new integration package. Team:Integrations Label for the Integrations team 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