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

[Metricbeat] Move redis.info metricset to ECS #10319

Merged
merged 4 commits into from
Jan 28, 2019
Merged

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Jan 24, 2019

No description provided.

@ruflin ruflin added in progress Pull request is currently in progress. module review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Jan 24, 2019
@ruflin ruflin requested a review from a team as a code owner January 24, 2019 14:42
@ruflin ruflin requested a review from webmat January 24, 2019 14:46
@ruflin ruflin requested a review from a team as a code owner January 24, 2019 15:35
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 good, minor wording nits for the AWS docs.

Missing the field changes and ecs-migration?

@@ -33,6 +33,9 @@ see https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_temp.html[Te
aws> sts get-session-token --serial-number arn:aws:iam::1234:mfa/test@elastic.co --token-code 456789 --duration-seconds 129600
----

Specific permissions needs to be added into the IAM user's policy to allow Metricbeat collecting AWS monitoring metrics. Please
Copy link
Contributor

Choose a reason for hiding this comment

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

"to allow Metricbeat collecting" => "to authorize Metricbeat to collect", or something to that effect?

Copy link
Contributor

Choose a reason for hiding this comment

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

And directly below, "metric set" => "metricset"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a left over which made it in this PR. Rebase will remove this part.

@kaiyan-sheng That is for you ^

Copy link
Contributor

Choose a reason for hiding this comment

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

@webmat @ruflin Thanks for the comment. This is addressed in #10334.

@ruflin ruflin changed the title Move Redis.info metricset to ECS [Metricbeat] Move eedis.info metricset to ECS Jan 25, 2019
@ruflin ruflin removed the in progress Pull request is currently in progress. label Jan 25, 2019
@ruflin
Copy link
Contributor Author

ruflin commented Jan 25, 2019

Ready for review.

@ruflin ruflin self-assigned this Jan 25, 2019
@webmat webmat changed the title [Metricbeat] Move eedis.info metricset to ECS [Metricbeat] Move Redis.info metricset to ECS Jan 25, 2019
@webmat webmat changed the title [Metricbeat] Move Redis.info metricset to ECS [Metricbeat] Move redis.info metricset to ECS Jan 25, 2019
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, once rebased & tests are green 👍

beat: metricbeat

- from: redis.info.server.os
to: os.full
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, if the service is remote, can't put under host.os. Agreed.

I like at the top level like this (rather than at service.os)

@ruflin ruflin merged commit 1ad2347 into elastic:master Jan 28, 2019
@ruflin ruflin deleted the redis-ecs branch January 28, 2019 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat module review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants