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 mysql/galera_status to ReporterV2 #11324

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Mar 19, 2019

See #10774

This metricset is a bit odd. There's no tests at all, and it looks like there's a lot of copy and paste from the mysql/status metricset. In the future, maybe someone cat put in a PR to clean up the code reuse. The lack of tests is a little weirder. I'm not sure if we need a new docker container added for galera?

@fearful-symmetry fearful-symmetry added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 19, 2019
@fearful-symmetry fearful-symmetry requested a review from a team March 19, 2019 19:27
@ruflin ruflin requested a review from jsoriano March 20, 2019 08:41
@ruflin
Copy link
Member

ruflin commented Mar 20, 2019

The code change overall LGTM. Did you try to run it locally to see if it still works.

I'm sure @jsoriano can add some more details here.

@jsoriano
Copy link
Member

@fearful-symmetry yes, galera_status is basically a copy of the status metricset and the plan would be to merge one into the other. At the moment this is barely tested and marked as experimental.

Change LGTM, but it'd be good to check in any case that it still works with Galera, try with any docker image available in the docker hub. The Percona one should be maintained (https://hub.docker.com/r/percona/percona-xtradb-cluster/ - XtraDB is the Percona rebranding for Galera).

Btw, I see that we are not printing the experimental warning in New, could you add it here? Something like:

cfgwarn.Experimental("The galera_status metricset is experimental.")

@fearful-symmetry
Copy link
Contributor Author

@jsoriano Added the log line, and it looks like it runs fine against docker.

@fearful-symmetry fearful-symmetry merged commit 8696d2c into elastic:master Mar 21, 2019
@fearful-symmetry fearful-symmetry deleted the migration/mb/reporterv2/mysql/galera_status branch March 21, 2019 13:10
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.

3 participants