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 cluster_disk to use ReporterV2 interface #10990

Merged

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Feb 28, 2019

Refer to #10774 for more info

@sayden sayden added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Feb 28, 2019
@sayden sayden self-assigned this Feb 28, 2019
@sayden sayden requested a review from a team as a code owner February 28, 2019 14:59
@sayden sayden changed the title [Metricbeat] Migrate ETCD leader to use ReporterV2 interface [Metricbeat] Migrate Ceph cluster_disk to use ReporterV2 interface Feb 28, 2019
@sayden
Copy link
Contributor Author

sayden commented Mar 1, 2019

Error in Kibana/status module seems unrelated

@sayden sayden added the review label Mar 1, 2019
@sayden sayden force-pushed the migration/mb/reporterv2/ceph/cluster_disk branch from a8df0bb to d13577f Compare March 1, 2019 12:33
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

It LGTM in principle, could you regenerate the data.json file?

@sayden
Copy link
Contributor Author

sayden commented Mar 5, 2019

https://github.com/elastic/beats/blob/master/metricbeat/module/ceph/cluster_disk/_meta/data.json does not need to be regenerated (in other words, when you regenerate it, nothing changes)

f := mbtest.NewEventFetcher(t, getConfig())
err := mbtest.WriteEvent(f, t)
if err != nil {
t.Skip("Skipping data generation test")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you skip it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What a nice timing 😄 #10990 (comment) and more info here #10993 (comment)

f := mbtest.NewEventFetcher(t, getConfig())
err := mbtest.WriteEvent(f, t)
if err != nil {
t.Skip("Skipping data generation test")
Copy link
Contributor Author

@sayden sayden Mar 5, 2019

Choose a reason for hiding this comment

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

CI test aren't launched with -data option but this is checked in mbtest.WriteEventsReporterV2 in line 42. An error can still be raised in line 36 if ceph is not started by compose, which was producing flaky tests.

The problem is that Ceph start with Compose is buggy and it was omitted consistently in all Metricsets

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. As soon as #10648 is merged we should switch over Ceph to generate the data file based on an http server as the current tests already use a http server.

Let's skip it for now but open a follow up issue to track:

@sayden sayden force-pushed the migration/mb/reporterv2/ceph/cluster_disk branch from d86447c to 2401d09 Compare March 6, 2019 14:55
@sayden sayden force-pushed the migration/mb/reporterv2/ceph/cluster_disk branch from ede70fe to 11031ce Compare March 7, 2019 10:02
@sayden
Copy link
Contributor Author

sayden commented Mar 7, 2019

Pushed few things I saw on new commits, mainly 30f809b and 45074a3 : New logger interface and reporting interface with errors

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

Successfully merging this pull request may close these issues.

5 participants