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

Merged
merged 5 commits into from
Apr 3, 2019
Merged

[Metricbeat] Migrate Ceph monitor_health to use ReporterV2 interface #11012

merged 5 commits into from
Apr 3, 2019

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Mar 1, 2019 •
edited
Loading

Refer to #10774 for more info

@sayden sayden added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 1, 2019
@sayden sayden self-assigned this Mar 1, 2019
@sayden sayden requested a review from a team as a code owner March 1, 2019 10:30
@sayden sayden mentioned this pull request Mar 1, 2019
Closed
62 tasks
@sayden sayden force-pushed the migration/mb/reporterv2/ceph/monitor_health branch from 2bab5b0 to 22500a7 Compare March 1, 2019 12:35
@sayden sayden force-pushed the migration/mb/reporterv2/ceph/monitor_health branch 2 times, most recently from d16c47d to d173fee Compare March 7, 2019 13:08
@sayden sayden added the review label Mar 7, 2019
ruflin
ruflin reviewed Mar 7, 2019
View reviewed changes
metricbeat/module/ceph/monitor_health/monitor_health.go Outdated Show resolved Hide resolved
metricbeat/module/ceph/monitor_health/monitor_health_test.go
@@ -32,6 +32,7 @@ import (

func TestFetchEventContents(t *testing.T) {
absPath, err := filepath.Abs("../_meta/testdata/")
assert.NoError(t, err)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@ruflin ruflin added in progress Pull request is currently in progress. and removed review labels Mar 13, 2019
@ruflin
Copy link
Member

ruflin commented Mar 13, 2019

Set to in progress.

@sayden sayden force-pushed the migration/mb/reporterv2/ceph/monitor_health branch from d173fee to dbe9a2b Compare March 13, 2019 11:23
@sayden sayden requested a review from a team as a code owner March 13, 2019 11:23
@sayden sayden added review and removed in progress Pull request is currently in progress. labels Mar 13, 2019
@sayden
Copy link
Contributor Author

sayden commented Mar 13, 2019

I've added new testing framework in the last commit

ruflin
ruflin reviewed Mar 15, 2019
View reviewed changes
Copy link
Member

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

LGTM except the data.json part.

metricbeat/module/ceph/monitor_health/_meta/data.json Outdated
@@ -1,51 +0,0 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We still need / want this. To have one generated again you need to have a docs.json in the testdata directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recovered data.json original file with v2 events but I'm not fully understand how to work with docs.json in testdata. Do you mean to create an empty file there and the run with -generate? At least I tried this and nothing happened 😕

Copy link
Member

Choose a reason for hiding this comment

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

The docs.json must contain an even and will generate the data.json out of it when you run -generate.

@sayden sayden force-pushed the migration/mb/reporterv2/ceph/monitor_health branch from eba5443 to da61c26 Compare March 26, 2019 15:06
@sayden sayden added in progress Pull request is currently in progress. and removed review labels Mar 26, 2019
ruflin
ruflin reviewed Mar 27, 2019
View reviewed changes
metricbeat/module/ceph/monitor_health/_meta/testdata/input.plain Outdated
@@ -0,0 +1 @@
{"status": "OK", "output": {"detail": [], "timechecks": {"round_status": "finished", "epoch": 3, "round": 0}, "health": {"health_services": [{"mons": [{"last_updated": "2019-03-13 11:21:24.667025", "name": "26c372192772", "avail_percent": 82, "kb_total": 936145620, "kb_avail": 773013080, "health": "HEALTH_OK", "kb_used": 115509168, "store_stats": {"bytes_total": 4195407, "bytes_log": 4128768, "last_updated": "0.000000", "bytes_misc": 65552, "bytes_sst": 1087}}]}]}, "overall_status": "HEALTH_OK", "summary": []}}
Copy link
Member

Choose a reason for hiding this comment

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

Why is the suffix of this .plain? Looks like json to me.

If you rename this to docs.json and remove the suffix config in config.yml and then run -generate, this should directly als update the data.json file.

Copy link
Member

Choose a reason for hiding this comment

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

@sayden I see you went now with input.json instead of docs.json as recommended above to have the data.json generated. Could you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that you mean docs.json like foo.json and that name didn't matter 😅 In any case, input or doc, it won't generate a data.json named file but a docs.json-expected.json, right? Now I'm not sure if I'm missing something else

Copy link
Member

Choose a reason for hiding this comment

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

yes: https://github.com/elastic/beats/blob/master/metricbeat/mb/testing/data/data_test.go#L179

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Now I understand, I misunderstood completely at the beginning. Thanks for the clarification :)

ruflin
ruflin approved these changes Apr 2, 2019
View reviewed changes
Copy link
Member

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

LGTM

@sayden
Copy link
Contributor Author

sayden commented Apr 2, 2019

jenkins, test this

@sayden
Copy link
Contributor Author

sayden commented Apr 3, 2019

Error seems unrelated, merging

@sayden sayden merged commit 9f3aede into elastic:master Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

Assignees

@sayden sayden

Labels
in progress Pull request is currently in progress. Metricbeat Metricbeat Team:Integrations Label for the Integrations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sayden @ruflin @exekias