-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.id, node.id, and node.name in elasticsearch/node_stats metricset #9168
Index cluster.id, node.id, and node.name in elasticsearch/node_stats metricset #9168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding a changelog entry?
@@ -140,6 +141,10 @@ func eventsMapping(r mb.ReporterV2, content []byte) error { | |||
errs = append(errs, event.Error) | |||
} | |||
|
|||
name, _ := event.MetricSetFields.GetValue("name") | |||
event.ModuleFields.Put("node.name", name.(string)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably check the type conversion here? Also what happens if above returns and error and name
does not exist? Will name
be nil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add a check here because it will get caught in the schema conversion itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Agree that it should not break here but what if someone modifies the other part of the code and does not thing about this part? I normally prefer to be on the safe side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in bcd8191.
CI test failure is valid. Fixing... |
@ruflin I added error handling and fixed the CI test failure. Ready for your 👀 again. Thanks! |
release: beta | ||
fields: | ||
- name: indices | ||
type: group | ||
- name: id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would assume this is now a duplicated field as it's also in node
metricset? I general I would recommend to configure "shared" fields on the module level.
jenkins, test this |
1 similar comment
jenkins, test this |
@ruflin Moved node fields to module level. Ready for your 👀 again. Thanks! |
release: beta | ||
fields: | ||
- name: indices | ||
- name: stats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why you introduce the mapping here (was it for the other fields)? In the end both should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, this change should've been reverted when I moved the node fields to the module level. Sorry about that. Fixing now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted in b5912d8.
… elasticsearch/node_stats metricset (#9255) Cherry-pick of PR #9168 to 6.x branch. Original message: This PR teaches the `elasticsearch/node_stats` metricset to index the Elasticsearch `cluster_uuid` as the module-level `cluster.id` field, as well as the node ID and node `name` fields as the metricset-level `node.id` and `node.name` fields, respectively.
This PR teaches the
elasticsearch/node_stats
metricset to index the Elasticsearchcluster_uuid
as the module-levelcluster.id
field, as well as the node ID and nodename
fields as the metricset-levelnode.id
andnode.name
fields, respectively.