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: Galera module and status Metricset #6892
Conversation
through the time and effort it would take to make this possible in a Docker container (not to mention it would be not very esthetic). Also this module is nearly the same as the mysql module with the only difference beeing that it queries only galera-specific fields (wsrep_ prefix) and so matching those fields for the event. Added virtualenv param for python2 cause this is needed and (for some reason) not specified by default.
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? |
} | ||
|
||
// Schema for mapping Galera-Status-Variables related to cluster health | ||
schema_small = s.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use underscores in Go names; var schema_small should be schemaSmall
|
||
var ( | ||
// Schema for mapping all Galera-Status-Variables | ||
schema_full = s.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use underscores in Go names; var schema_full should be schemaFull
metricbeat/module/galera/galera.go
Outdated
/* | ||
Package galera is Metricbeat module for Galera Cluster. | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package comment is detached; there should be no blank lines between it and the package statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. Only skimmed through it so far and left some high level comments?
How can we test this? Normally we have a Dockerfile for each service, not sure if this is possible for Galera?
Do you have a link to the full list of metrics this is gathering?
NOTICE.txt
Outdated
@@ -630,9 +630,9 @@ THE SOFTWARE IS PROVIDED “AS IS”, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR I | |||
|
|||
-------------------------------------------------------------------- | |||
Dependency: github.com/go-sql-driver/mysql | |||
Revision: 9dee4ca50b83acdf57a35fb9e6fb4be640afa2f3 | |||
Revision: cd4cb909ce1a31435164be29bf3682031f61539a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you open a separate PR to update the mysql dependency? I think we should do that anyway.
libbeat/scripts/Makefile
Outdated
@@ -68,7 +68,7 @@ GOPACKAGES_COMMA_SEP=$(subst $(space),$(comma),$(strip ${GOPACKAGES})) | |||
PYTHON_ENV?=${BUILD_DIR}/python-env | |||
PIP_INSTALL_PARAMS?= | |||
BUILDID?=$(shell git rev-parse HEAD) ## @Building The build ID | |||
VIRTUALENV_PARAMS?= | |||
VIRTUALENV_PARAMS?=-p /usr/bin/python2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
enabled: false | ||
period: 10s | ||
|
||
# Specifies the status variables to query from the cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The short config should not contain all the config options. Please also create a config.reference.yml
file.
metricbeat/module/galera/_meta/evn
Outdated
@@ -0,0 +1,3 @@ | |||
GALERA_DSN=root:test@tcp(galera:3306)/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the name of the file be env?
Makefile
Outdated
@@ -5,7 +5,7 @@ PROJECTS=libbeat $(BEATS) | |||
PROJECTS_ENV=libbeat filebeat metricbeat | |||
SNAPSHOT?=yes | |||
PYTHON_ENV?=$(BUILD_DIR)/python-env | |||
VIRTUALENV_PARAMS?= | |||
VIRTUALENV_PARAMS?=-p /usr/var/python2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not change the defaults here as it will potentially break other things. Perhaps you can open a separate issue / PR to discuss this in more detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to break travis build
Thank your contribution, the module is looking really good!! Could you please sign the CLA? Also, it will need a PR as it's conflicting with master |
|
||
// New create a new instance of the MetricSet | ||
// Loads query_mode config setting from the config file | ||
func New(base mb.BaseMetricSet) (mb.MetricSet, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should flag this as experimental for the first version, could you add something similar to this?
cfgwarn.Experimental("The ceph pool_disk metricset is experimental") |
Thank your contribution, the module is looking really good!! Could you please sign the CLA? Also, it will need a PR as it's conflicting with master. Also, do you happen to have a dashboard for this module? It would be really nice to add it here 😇 . If you are willing to, you can have a look to docs on how to do it https://www.elastic.co/guide/en/beats/devguide/current/new-dashboards.html |
Awesome contribution 🙂 I think it could fit as a metricset of mysql module, or even as an optionaly-enabled part of the mysql status metricset. |
@xy795 we have talked about this offline, and we think this should be included as a metricset of the mysql module, as all the connection and query logic could be reused, wdyt? Please tell me if you see any problem moving this to a mysql metricset. |
added galera-status metricset to the mysql module moved the vendor go-sql-drivers folder back to the mysql module
http://dev.mysql.com/doc/refman/5.7/en/show-status.html | ||
http://galeracluster.com/documentation-webpages/galerastatusvariables.html | ||
*/ | ||
package galeraStatus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use MixedCaps in package name; galeraStatus should be galerastatus
@@ -0,0 +1,116 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package comment should be of the form "Package galeraStatus ..."
@@ -0,0 +1,144 @@ | |||
package galeraStatus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use MixedCaps in package name; galeraStatus should be galerastatus
@@ -0,0 +1,116 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package comment should be of the form "Package galerastatus ..."
@jsoriano you are absolutely right, I thought about this earlier, but for some reason I decided to make an own module. Now I moved the metricset to the mysql module, since there is no reason to make a own module for galera. @exekias I think we signed the CLA already. You should be able to find it under company "F24 IT-Services GmbH" @ruflin I know there are no tests for this. I tried for several days setting up a Docker test for this, but did not manage to get the tests working with this. I tested the metricset very well with a local Galera-Cluster and found no errors so far. Since this metricset does nearly the same thing as the official mysql/status set I decided it would not be reasonable to invest that much time in creating a functioning test for this. I hope this is acceptable for you. Otherwise do you have an other idea to make tests in this case possible? |
}, | ||
"flow_ctl": s.Object{ | ||
"paused": c.Float("wsrep_flow_control_paused"), | ||
"paused_ns": c.Int("wsrep_flow_control_paused_ns"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These metrics are quite important in galera, they probably deserve to be also in the small metric set 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this as a new metricset! I have added some things, the only important one would be to mark it as experimental by now.
|
||
// New create a new instance of the MetricSet | ||
// Loads query_mode config setting from the config file | ||
func New(base mb.BaseMetricSet) (mb.MetricSet, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add here a line like this one:
cfgwarn.Experimental("The mysql galera-status metricset is experimental.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this already.
} | ||
|
||
// loadStatus loads all status entries from the given database into an array. | ||
func (m *MetricSet) loadStatus(db *sql.DB) (map[string]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is quite similar in the status metricset, we could probably reuse some code. We can leave it for a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed most of the code in fetch and event mapping can also be reused.
- name: status | ||
type: group | ||
description: > | ||
`status` contains the metrics that were obtained by the status SQL-Galera query. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add here also the experimental mark with:
release: experimental
@xy795 could you please update the branch and run |
@jsoriano when I run |
With updating the branch I mean to merge or rebase it with |
Thanks @xy795, this looks better now, the tests failing don't seem related to this change :) |
jenkins, test this please |
@xy795 but there are still issues with the CLA :( could you please check that you have signed it with the same email address that you are using for your commits? |
jenkins, test this again, please |
@jsoriano the CLA should now be complete |
@xy795 sorry, but it still doesn't appear as signed :( are you signing it with the same mail address that the one in your commits? |
@jsoriano I checked it again and now it schoul'd finally all be fine. |
@xy795 I'm sorry but we cannot find any signature with your username, or with the name or email in the commits of this PR (author appears as |
@jsoriano I rechecked all my commits and all of them has the correct author (the one that is in the CLA which was signed today again) |
Thanks @ruflin |
@xy795 Any chance to get an other rebase and |
# Conflicts: # metricbeat/include/fields.go
@xy795 fields need to be updated after updating with master |
jenkins, test this please |
Failing test doesn't seem related, I'm merging this. |
Thanks @xy795 for the effort here! |
Add module and metricset for collecting metrics from a Galera-MySQL-Cluster-Node.
Features:
Other changes in metricbeat: Moved govendor dependency from the "mysql" module to metribeat root, cause it's needed in the "galera" module as well.
Other: Added python virtualenv parameter to use python2 by default because it's needed and caused me various issues and much frustration.
Please let me now if you think I missed something in this module or if something needs further improvements. Otherwise I would love to see this module in the official beats repository!