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.

Sign up for GitHub

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

Jump to bottom

Updating Metricbeat stack modules to ECS #10350

Merged
merged 22 commits into from
Jan 31, 2019
Merged

Updating Metricbeat stack modules to ECS #10350

merged 22 commits into from
Jan 31, 2019

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Jan 26, 2019 •
edited
Loading

This PR updates a few fields in the Elastic stack modules in Metricbeat, viz. elasticsearch, logstash, and kibana to their ECS names and types.

Based on discussions happening in #10218, but only scoped to Elastic stack modules.

@ycombinator ycombinator requested review from a team as code owners January 26, 2019 15:29
@elasticmachine
Copy link
Collaborator

Pinging @elastic/stack-monitoring

@ycombinator ycombinator mentioned this pull request Jan 26, 2019
Closed
@ycombinator ycombinator added review and removed in progress Pull request is currently in progress. labels Jan 26, 2019
@ycombinator ycombinator requested review from webmat and ruflin January 26, 2019 15:54
@ycombinator ycombinator changed the title [WIP] Updating Metricbeat stack modules to ECS Updating Metricbeat stack modules to ECS Jan 26, 2019
ruflin
ruflin suggested changes Jan 28, 2019
View reviewed changes
Copy link
Contributor

@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.

You also need to update the ecs-migration.yml file.

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/kibana/stats/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/kibana/stats/_meta/fields.yml Outdated Show resolved Hide resolved
@ruflin ruflin mentioned this pull request Jan 28, 2019
Closed
@ycombinator
Copy link
Contributor Author

@ruflin This is ready for another review pass. I addressed feedback from your previous review and left a couple of questions as well. Thanks!

webmat
webmat reviewed Jan 28, 2019
View reviewed changes
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Looking pretty good. Only need to resolve on the PID inconsistency. Also noticed a few service.version that haven't been migrated yet.

Once these are settled, this LGTM

metricbeat/module/elasticsearch/index_recovery/_meta/fields.yml Show resolved Hide resolved
metricbeat/module/logstash/node/_meta/data.json Outdated Show resolved Hide resolved
metricbeat/module/logstash/node/_meta/fields.yml Show resolved Hide resolved
metricbeat/module/kibana/stats/_meta/data.json Outdated Show resolved Hide resolved
metricbeat/module/kibana/status/_meta/data.json Outdated Show resolved Hide resolved
metricbeat/module/logstash/node/_meta/data.json Show resolved Hide resolved
@ycombinator
Copy link
Contributor Author

@webmat @ruflin Addressed latest round of feedback. This is ready for your 👀 again, when you get a chance. Thanks!

@ycombinator
Copy link
Contributor Author

jenkins, test this

ruflin
ruflin reviewed Jan 29, 2019
View reviewed changes
dev-tools/ecs-migration.yml Outdated Show resolved Hide resolved
ruflin
ruflin reviewed Jan 29, 2019
View reviewed changes
Copy link
Contributor

@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. One minor comment.

I think we need to figure out what we do with the hostname fields. What we could do is get this PR in without the hostname fields and figure it out as a follow up.

metricbeat/module/logstash/node_stats/_meta/data.json Show resolved Hide resolved
@ycombinator
Copy link
Contributor Author

@ruflin Other than your second thoughts comments that require third thoughts from @webmat 😉 I've addressed all your other feedback.

@webmat
Copy link
Contributor

webmat commented Jan 29, 2019

@ruflin @ycombinator I think for both transport_address and hostname we could have them under service.*. This would neatly solve the problem of monitoring services from the outside vs locally.

webmat
webmat reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

Great! Thanks @ycombinator

LGTM

@webmat
Copy link
Contributor

webmat commented Jan 30, 2019

Of course I had to click too fast and not make this "Approved" 😆

@ycombinator
Updating field names to ECS in field.yml and generated files
0eaf78a
@ycombinator
Updating kibana module parsing code
98e368f
@ycombinator
Updating logstash module parsing code
3527606
@ycombinator
Adding CHANGELOG entries
f70619c
@ycombinator
Fixing location of CHANGELOG entries
54ef869
@ycombinator
Fixing up fields per ECS
91ae2ca
@ycombinator
Fixing up kibana stats parsing and regenerating data.json file
c3d53ed
@ycombinator
Kibana stats metricset changes
69a43ad
@ycombinator
Updating Kibana status metricset
9f577dd
@ycombinator
Updating Logstash metricsets
9c06874
@ycombinator
Adding to ecs-migration.yml
313190e
@ycombinator
Setting service.version for kibana metricsets
7ada215
@ycombinator
Keeping only global process.pid for logstash/node metricset
341954b
@ycombinator
Reverting transport_address change in kibana/stats metricset
8981925
@ycombinator
Adding service.id for logstash/node metricset
54e4917
@ycombinator
Adding host.hostname, service.id, and service.version for logstash/no…
d8ef445 
…de_stats metricset
@ycombinator
Review feedback
2cb76a5
@ycombinator
Reverting changes leftover from testing
0c54748
@ycombinator
Adding service.hostname to fields.common.yml
5f16418
@ycombinator
Updating language
85bce07
@ycombinator
Regenerating generated files
dfce436
@ycombinator
Regenerating generated files
b389f7d
@ycombinator ycombinator merged commit ae83e46 into elastic:master Jan 31, 2019
@ycombinator ycombinator deleted the mb-stack-ecs branch December 25, 2019 11:15
@matschaffer matschaffer mentioned this pull request May 25, 2022
Closed
@Kerry350 Kerry350 mentioned this pull request Jun 17, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin requested changes

@cachedout cachedout cachedout approved these changes

@webmat webmat webmat approved these changes

Assignees
No one assigned
Labels
breaking change ecs Feature:Stack Monitoring Metricbeat Metricbeat module review v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ycombinator @elasticmachine @webmat @cachedout @ruflin