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

ycombinator
Copy link
Contributor

@ycombinator ycombinator 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 added review Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. v7.0.0-alpha1 v6.6.0 labels Nov 22, 2018
@@ -1,15 +1,16 @@
{
"@timestamp": "2017-10-12T08:05:34.853Z",
"beat": {
"agent": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@ycombinator
Copy link
Contributor Author

jenkins, test this

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.

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
Copy link
Member

Choose a reason for hiding this comment

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

Was this a "bug" before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@ycombinator
Copy link
Contributor Author

jenkins, test this

@ycombinator
Copy link
Contributor Author

jenkins, test this

2 similar comments
@ycombinator
Copy link
Contributor Author

jenkins, test this

@ycombinator
Copy link
Contributor Author

jenkins, test this

@ycombinator ycombinator force-pushed the metricbeat-elasticsearch-node-node-id branch from d3750ad to f0491a2 Compare November 27, 2018 05:10
@ycombinator
Copy link
Contributor Author

jenkins, test this

@ycombinator ycombinator force-pushed the metricbeat-elasticsearch-node-node-id branch from f0491a2 to 9977852 Compare November 27, 2018 17:26
@ycombinator ycombinator merged commit 355d08d into elastic:master Nov 28, 2018
@ycombinator ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Nov 28, 2018
ycombinator added a commit to ycombinator/beats that referenced this pull request Nov 29, 2018
* 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

* 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
…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
@ycombinator ycombinator deleted the metricbeat-elasticsearch-node-node-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.

2 participants