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 couchbase cluster & pool metricset to new data generation #11112

Merged
merged 2 commits into from
Mar 7, 2019

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Mar 6, 2019

The data.json is now generated based on example JSON documents. In case they change, it means the code change in some way. This will make it much easier to detect changes.

Also the basic tests for the cluster are now covered by the new testing.

The error handling moved over to the new error interface for simplicity.

@ruflin ruflin added module review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 6, 2019
@ruflin ruflin requested review from a team as code owners March 6, 2019 15:44
@ruflin
Copy link
Contributor Author

ruflin commented Mar 6, 2019

@berfinsari Thanks to your work, this is now possible 🎉

@kaiyan-sheng
Copy link
Contributor

@ruflin LGTM, just one question: data.json is generated by _meta/testdata/docs.json here, so does docs.json-expected.json still needed/useful here?

@fearful-symmetry
Copy link
Contributor

Sorta going off what @kaiyan-sheng asked, should we handle the docs.json files differently? Considering how many json files we'll have floating around as we migrate more tests to this, I wonder if there's a more clever solution to data.json in general? That's probably a longer term question, though.

Otherwise LGTM, but do we want to update the ReporterV2 error handling in the same PR?

@ruflin
Copy link
Contributor Author

ruflin commented Mar 7, 2019

@kaiyan-sheng I would say yes. There is a difference here. If the data.json is not correct, we will get a diff but unit tests do not fail. If the expected is wrong, tests will fail. Also data.json only takes the first event if their are multiple, in the expected-json there are all events produced.

@fearful-symmetry I'm sure there are better ways. Perhaps you can open an issue to discuss it with a suggestion?

For the error handling: If you prefer, I can also put it into a different PR.

ruflin added 2 commits March 7, 2019 09:07
…eneration

The data.json is now generated based on example JSON documents. In case they change, it means the code change in some way. This will make it much easier to detect changes.

Also the basic tests for the cluster are now covered by the new testing.

The error handling moved over to the new error interface for simplicity.
@ruflin ruflin force-pushed the couchbase-update branch from c8f9880 to 0a551a7 Compare March 7, 2019 08:10
@ruflin
Copy link
Contributor Author

ruflin commented Mar 7, 2019

@fearful-symmetry I remove the logging change, we can tackle it in a separate PR.

@sayden
Copy link
Contributor

sayden commented Mar 7, 2019

Tested locally. Everything ok

@ruflin ruflin merged commit af4c0f9 into elastic:master Mar 7, 2019
@ruflin ruflin deleted the couchbase-update branch March 7, 2019 11:03
@ruflin ruflin self-assigned this Mar 18, 2019
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.

4 participants