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

[Metricbeat] Align rabbitmq with ECS and have module fields #10563

Merged
merged 5 commits into from
Feb 5, 2019
Merged

[Metricbeat] Align rabbitmq with ECS and have module fields #10563

merged 5 commits into from
Feb 5, 2019

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Feb 5, 2019

No description provided.

@ruflin ruflin added module review Metricbeat Metricbeat ecs Team:Integrations Label for the Integrations team labels Feb 5, 2019
@ruflin ruflin self-assigned this Feb 5, 2019
@ruflin ruflin requested a review from a team as a code owner February 5, 2019 13:18
@ruflin ruflin mentioned this pull request Feb 5, 2019
Closed
@ruflin ruflin requested a review from a team as a code owner February 5, 2019 13:24
@ruflin ruflin requested a review from webmat February 5, 2019 13:24
webmat
webmat reviewed Feb 5, 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.

Nothing major. Perhaps you had good reasons, but here are my concerns:

  • A field def is deleted instead of being aliased to rabbitmq.node.name
  • Why delete assertions, shouldn't you instead adjust the field names in the assertion? Even if redundant field names, different code paths set these values.

metricbeat/module/rabbitmq/connection/connection_test.go Show resolved Hide resolved
metricbeat/module/rabbitmq/exchange/exchange_test.go Show resolved Hide resolved
metricbeat/module/rabbitmq/node/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/rabbitmq/queue/queue_test.go Show resolved Hide resolved
@webmat
Copy link
Contributor

webmat commented Feb 5, 2019

@ruflin Oh, forgot: ecs-migration.yml for that missing alias

@ruflin
Copy link
Member Author

ruflin commented Feb 5, 2019

@webmat Seems like it's already there?

webmat
webmat approved these changes Feb 5, 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.

LGTM

@ruflin You're right. Didn't see it in your recent commit fixing fields.yml. But it was already in there before that. 👍

@ruflin ruflin merged commit 38d256c into elastic:master Feb 5, 2019
@ruflin ruflin deleted the rabbitmq-ecs2 branch February 5, 2019 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webmat webmat webmat approved these changes

Assignees

@ruflin ruflin

Labels
ecs Metricbeat Metricbeat module review Team:Integrations Label for the Integrations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ruflin @webmat