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

Adding xpack code for ES index recovery metricset #8106

Conversation

ycombinator
Copy link
Contributor

This PR teaches the elasticsearch/index_recovery metricset to index index_recovery documents into .monitoring-es-6-mb-* indices. These documents should be compatible in structure to index_recovery documents in the current .monitoring-es-6-* indices indexed via the internal monitoring method.

@ycombinator ycombinator added review Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. v7.0.0-alpha1 monitoring v6.5.0 labels Aug 28, 2018
// specific language governing permissions and limitations
// under the License.

package index_recovery

Choose a reason for hiding this comment

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

don't use an underscore in package name

@@ -50,7 +50,7 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
config := struct {
ActiveOnly bool `config:"index_recovery.active_only"`
}{
ActiveOnly: true,
ActiveOnly: false,
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 made this change to be consistent with the current default value in XPack Monitoring:

https://github.com/elastic/elasticsearch/blob/ed0571e16caac689a77362bfe4c45c79dcba4d82/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/collector/indices/IndexRecoveryCollector.java#L41-L45

However, it means that even for the non-xpack-monitoring case we will collect all index recoveries, not just active ones. WDYT of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore this comment, I was able to make this flag smarter in 4817f48.

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Code overall looks good to me. Some minor comments.


"github.com/elastic/beats/metricbeat/helper/elastic"
"github.com/elastic/beats/metricbeat/module/elasticsearch"

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove newline

continue
}

for _, value = range shards {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried that we reuse the name value here.

continue
}

value, ok = indexData["shards"]
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason here that shards etc. could be missing?

Also worried about reuse of value here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the reason here that shards etc. could be missing?

It should never happen. I was just being safe with the ok check.

Copy link
Member

Choose a reason for hiding this comment

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

+1 on the ok check. Was mainly curious if in "normal" behaviour of the endpoint we expect this to happen. They way I understand your answer it's not the case.

@ycombinator
Copy link
Contributor Author

@ruflin Addressed all feedback from last review. Ready for your 👀 again. Thanks!

@ruflin
Copy link
Member

ruflin commented Aug 29, 2018

jenkins, test this

@ycombinator ycombinator merged commit f6b3e78 into elastic:master Aug 29, 2018
@ycombinator ycombinator added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Aug 29, 2018
@ycombinator ycombinator deleted the metricbeat/elasticsearch/index-recovery/x-pack branch August 29, 2018 15:02
@ycombinator ycombinator added needs_backport PR is waiting to be backported to other branches. and removed needs_backport PR is waiting to be backported to other branches. labels Sep 5, 2018
ycombinator added a commit that referenced this pull request Sep 7, 2018
* Adding index_recovery xpack monitoring documents

* Get all recoveries if xpack.enabled is true

* Removing newline (which also reordered imports)

* Don't reuse value so much

(cherry picked from commit f6b3e78)
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.

3 participants