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 node.id in elasticsearch/node metricset #9209

Merged

Conversation

Projects
None yet
2 participants
@ycombinator
Copy link
Contributor

commented Nov 22, 2018

This PR teaches the elasticsearch/node metricset to index the Elasticsearch node's id as the metricset-level id field.


This is the final PR in a series of recent PRs that ensured that all elasticsearch metricsets had module-level cluster.id and cluster.name fields and that the node and node_stats metricsets also had the node.id and node.name fields.

As such, this final PR also adds a CHANGELOG entry.

@ycombinator ycombinator changed the title Metricbeat elasticsearch node node Index node.id in elasticsearch/node metricset Nov 22, 2018

@ycombinator ycombinator requested a review from ruflin Nov 22, 2018

@@ -1,15 +1,16 @@
{
"@timestamp": "2017-10-12T08:05:34.853Z",
"beat": {
"agent": {

This comment has been minimized.

Copy link
@ycombinator

ycombinator Nov 22, 2018

Author Contributor

Note to self: Remember that this needs to be beat in the 6.x backport PR.

@ycombinator

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

jenkins, test this

@ruflin

ruflin approved these changes Nov 22, 2018

Copy link
Collaborator

left a comment

Left on minor comment. Besides that LGTM.

@@ -93,8 +93,7 @@ func eventsMapping(r mb.ReporterV2, info elasticsearch.Info, content []byte) err
continue
}

// Write name here as full name only available as key
event.MetricSetFields["name"] = name
event.MetricSetFields["id"] = id

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 22, 2018

Collaborator

Was this a "bug" before?

This comment has been minimized.

Copy link
@ycombinator

ycombinator Nov 22, 2018

Author Contributor

Yeah, it was. We were actually assigning the node ID to the name field in the event. It's easy to get them confused because by default ES will set the node name as the node ID.

This comment has been minimized.

Copy link
@ruflin

ruflin Nov 22, 2018

Collaborator

this is potentially a breaking change or a bug fix, so we should probably mention it in the changelog.

This comment has been minimized.

Copy link
@ycombinator

ycombinator Nov 22, 2018

Author Contributor

Good point. Added CHANGELOG entry in breaking changes section in d3750ad.

@ycombinator

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

jenkins, test this

@ruflin

ruflin approved these changes Nov 22, 2018

@ycombinator

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

jenkins, test this

2 similar comments
@ycombinator

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

jenkins, test this

@ycombinator

This comment has been minimized.

Copy link
Contributor Author

commented Nov 26, 2018

jenkins, test this

@ycombinator ycombinator force-pushed the ycombinator:metricbeat-elasticsearch-node-node-id branch from d3750ad to f0491a2 Nov 27, 2018

@ycombinator

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

jenkins, test this

@ycombinator ycombinator force-pushed the ycombinator:metricbeat-elasticsearch-node-node-id branch from f0491a2 to 9977852 Nov 27, 2018

@ycombinator ycombinator merged commit 355d08d into elastic:master Nov 28, 2018

4 checks passed

CLA Commit author is a member of Elasticsearch
Details
Hound No violations found. Woof!
beats-ci Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ycombinator added a commit to ycombinator/beats that referenced this pull request Nov 28, 2018

Index node.id in elasticsearch/node metricset (elastic#9209)
* Index node.id in elasticsearch/node metricset

* Adding CHANGELOG entries

* Fixing PR number in CHANGELOG entry

* Adding breaking change entry for node.name field correction

(cherry picked from commit 355d08d)

ycombinator added a commit to ycombinator/beats that referenced this pull request Nov 29, 2018

Index node.id in elasticsearch/node metricset (elastic#9209)
* Index node.id in elasticsearch/node metricset

* Adding CHANGELOG entries

* Fixing PR number in CHANGELOG entry

* Adding breaking change entry for node.name field correction

(cherry picked from commit 355d08d)

ycombinator added a commit to ycombinator/beats that referenced this pull request Nov 29, 2018

Index node.id in elasticsearch/node metricset (elastic#9209)
* Index node.id in elasticsearch/node metricset

* Adding CHANGELOG entries

* Fixing PR number in CHANGELOG entry

* Adding breaking change entry for node.name field correction

(cherry picked from commit 355d08d)

ycombinator added a commit to ycombinator/beats that referenced this pull request Dec 18, 2018

Index node.id in elasticsearch/node metricset (elastic#9209)
* Index node.id in elasticsearch/node metricset

* Adding CHANGELOG entries

* Fixing PR number in CHANGELOG entry

* Adding breaking change entry for node.name field correction

(cherry picked from commit 355d08d)

ycombinator added a commit to ycombinator/beats that referenced this pull request Dec 18, 2018

Index node.id in elasticsearch/node metricset (elastic#9209)
* Index node.id in elasticsearch/node metricset

* Adding CHANGELOG entries

* Fixing PR number in CHANGELOG entry

* Adding breaking change entry for node.name field correction

ycombinator added a commit that referenced this pull request Dec 19, 2018

Cherry-pick #9209 to 6.x: Index node.id in elasticsearch/node metrics…
…et (#9281)

* Index node.id in elasticsearch/node metricset (#9209)

* Index node.id in elasticsearch/node metricset

* Adding CHANGELOG entries

* Fixing PR number in CHANGELOG entry

* Adding breaking change entry for node.name field correction

* Fixing field name for 6.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.