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

Index cluster_uuid in elasticsearch/node metricset #8771

Conversation

ycombinator
Copy link
Contributor

This PR teaches the elasticsearch/node metricset to index the Elasticsearch cluster_uuid as the module-level cluster.id field.

@ycombinator ycombinator added review Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. v7.0.0-alpha1 v6.6.0 labels Oct 26, 2018
@ruflin
Copy link
Member

ruflin commented Oct 29, 2018

There still seems to be some troubles with CI

@ycombinator ycombinator force-pushed the metricbeat-elasticsearch-node-cluster-id branch 2 times, most recently from e73e3ee to 7929f3d Compare November 1, 2018 08:18
@ycombinator ycombinator force-pushed the metricbeat-elasticsearch-node-cluster-id branch from 7929f3d to 4d0e6f2 Compare November 20, 2018 02:14
@ycombinator
Copy link
Contributor Author

@ruflin I addressed the CI troubles. Ready for your review again. Thanks!

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.

Overall LGTM. Should we add a changelog entry as we added a new field?

@@ -48,7 +48,6 @@ func TestFetch(t *testing.T) {
w.WriteHeader(200)
w.Header().Set("Content-Type", "application/json;")
w.Write([]byte(response))
assert.Equal(t, "/_nodes/_local", r.RequestURI)
Copy link
Member

Choose a reason for hiding this comment

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

Is this not correct anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's correct but I removed it because none of the other tests that spin up a HTTP server like this have an assertion in them. Also, for some bizzare reason this test works locally, but only fails in CI.

Copy link
Member

Choose a reason for hiding this comment

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

That it works locally and fails on CI makes me worry now because I think it passed previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, digging into it some more...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I must've been mixing up PRs last night. This test clearly failed locally as well just now (several times) and the issue was obvious. Fixed it in 34310c1.

@ycombinator
Copy link
Contributor Author

jenkins, test this

1 similar comment
@ycombinator
Copy link
Contributor Author

jenkins, test this

None of the other test HTTP servers used in the Metricbeat codebase use an assertion like this so I thought it was safe to get rid of this one.
@ycombinator ycombinator force-pushed the metricbeat-elasticsearch-node-cluster-id branch from e01a5c5 to 0a00aa1 Compare November 22, 2018 10:16
@ycombinator
Copy link
Contributor Author

ycombinator commented Nov 22, 2018

@ruflin All the things are green now on this PR and review feedback has been addressed. Please take another gander whenever you get a chance. Thanks!

[EDIT] As mentioned in another PR comment, I'll add an overall CHANGELOG entry for all these fields in various ES metricsets in the final PR (yet to be put up).

@ycombinator ycombinator merged commit 9bd2499 into elastic:master Nov 22, 2018
@ycombinator ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Nov 22, 2018
ycombinator added a commit to ycombinator/beats that referenced this pull request Nov 26, 2018
This PR teaches the `elasticsearch/node` metricset to index the Elasticsearch `cluster_uuid` as the module-level `cluster.id` field.

(cherry picked from commit 9bd2499)
ycombinator added a commit that referenced this pull request Nov 26, 2018
This PR teaches the `elasticsearch/node` metricset to index the Elasticsearch `cluster_uuid` as the module-level `cluster.id` field.

(cherry picked from commit 9bd2499)
@ycombinator ycombinator deleted the metricbeat-elasticsearch-node-cluster-id branch December 25, 2019 11:13
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.

None yet

2 participants