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] Migrate Ceph osd_tree to use ReporterV2 interface #11048

Merged
merged 2 commits into from
Mar 13, 2019

Conversation

berfinsari
Copy link
Contributor

@berfinsari berfinsari commented Mar 3, 2019

Refer to #10774 for more info

@berfinsari berfinsari requested a review from a team as a code owner March 3, 2019 05:32
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@sayden sayden added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 4, 2019
@sayden
Copy link
Contributor

sayden commented Mar 4, 2019

jenkins, test this please

Copy link
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

Overall looks good, just one single change is needed to merge PR. Everything else in comments are minor.

@@ -36,6 +36,8 @@ var (
}.Build()
)

var logger = logp.NewLogger("ceph.osd_tree")
Copy link
Contributor

Choose a reason for hiding this comment

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

If you rebase to master, you can remove this line

content, err := m.HTTP.FetchContent()
if err != nil {
return nil, err
logger.Error(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you rebase to master, you can use m.Logger().Error(err) here instead

}

events, err := eventsMapping(content)
if err != nil {
return nil, err
logger.Error(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you rebase to master, you can use m.Logger().Error(err) here instead

err := mbtest.WriteEvents(f, t)
if err != nil {
f := mbtest.NewReportingMetricSetV2(t, getConfig())
events, errs := mbtest.ReportingFetchV2(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this is the only critical change needed here. Check my comments here to avoid a flaky test

@berfinsari berfinsari force-pushed the migration/mb/reportv2/ceph/osd_tree branch from 388b053 to 65c4dbb Compare March 10, 2019 17:10
@berfinsari
Copy link
Contributor Author

Thanks for your review. I believe I've addressed all the feedback now.

@ruflin
Copy link
Member

ruflin commented Mar 13, 2019

jenkins, test this

Copy link
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

Good work!

@sayden sayden merged commit de13893 into elastic:master Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants