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

Move parts of metricset code to Elasticsearch module #7103

Merged
merged 2 commits into from
May 16, 2018

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented May 15, 2018

This is inspired by #7074. More work will follow in other PR's.

@ruflin ruflin added in progress Pull request is currently in progress. module Metricbeat Metricbeat labels May 15, 2018
@ruflin
Copy link
Member Author

ruflin commented May 15, 2018

jenkins, test this

@ruflin ruflin added review and removed in progress Pull request is currently in progress. labels May 15, 2018
This is inspired by elastic#7074. More work is needed.
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

It looks mostly good to me, only some comments. I also wonder if we should mention somewhere that paths configuration behaviour changes, like the breaking change mentioned in #7074.


const (
defaultScheme = "http"
pathConfigKey = "management_path_prefix"
Copy link
Member

@jsoriano jsoriano May 16, 2018

Choose a reason for hiding this comment

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

The pathConfigKey was path before. I used management_path_prefix in #7074 because this is the terminology in RabbitMQ.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ups, this is a leftover. Changed to path.


http, err := helper.NewHTTP(base)
// Get the stats from the local node
ms, err := elasticsearch.NewMetricSet(base, "/_nodes/_local/stats")
Copy link
Member

Choose a reason for hiding this comment

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

Move this path also to its own constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

move to a constant. I normally only use constant if it's used more then at least once.

@ruflin
Copy link
Member Author

ruflin commented May 16, 2018

For the path change you mentioned: It's not something that is documented in the Elasticsearch module and I hope / assume it's not used as it's not something that can be changed itself in ES. I wonder if we should remove it completely?

@jsoriano
Copy link
Member

Merging it now, and letting the path option removal by now.

@jsoriano jsoriano merged commit 1dfd073 into elastic:master May 16, 2018
stevea78 pushed a commit to stevea78/beats that referenced this pull request May 20, 2018
stevea78 pushed a commit to stevea78/beats that referenced this pull request May 20, 2018
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.

2 participants