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

Improved Mysql plugin #633

Closed
wants to merge 4 commits into from
Closed

Improved Mysql plugin #633

wants to merge 4 commits into from

Conversation

maksadbek
Copy link
Contributor

Improved mysql plugin as discussed in #403

@sparrc
Copy link
Contributor

sparrc commented Feb 3, 2016

Thanks @maksadbek, looks like you got a test failure:

--- FAIL: TestMysqlDefaultsToLocal (0.02s)
    Error Trace:    mysql_test.go:25
    Error:      Received unexpected error "Error 1381: You are not using binary logging"


FAIL
FAIL

Could you also squash your commits into one? I'll do a full review tomorrow

@@ -24,6 +30,10 @@ var sampleConfig = `
#
# If no servers are specified, then localhost is used as the host.
servers = ["tcp(127.0.0.1:3306)/"]
PerfEventsStatementsDigestTextLimit = 120
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add comments for all of these? and also change them to match the general underscore style of the config file,

ie:

perf_events_statement_digest_text_limit = 120

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, for each of these, can you explain the use-case of someone wanting to adjust these? Seems like a few of these could just be hardcoded constants within the config file, but I'm not as familiar with these metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I will add brief explanation of each metric and fix the names of config settings.
This config settings are changed frequently, that is why I did not hard code.
The test fails is because there is metric that tries to gather replication stats, and it only works if the database is slave. I will add the configuration settings to switch it on/off.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to modify the MySQL docker image to be a slave? Or at least act like one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the configurations file(my.cnf) must be changed as follows:

bind-address = 0.0.0.0
server-id = 2
master-host =  [private-IP-of-master]
master-user = [replication-username]
master-password = [replication-password]
master-connect-retry = 60

But this requires another MySQL to be a master for this slave.

@sparrc
Copy link
Contributor

sparrc commented Feb 12, 2016

@maksadbek Is this ready to go now? Please ping me when you've made updates as I don't monitor all the commits that go into PRs

@sparrc sparrc mentioned this pull request Feb 16, 2016
@maksadbek
Copy link
Contributor Author

Hi @sparrc , Sorry for late reply. Which version of MySQL server is used to test ? Is there any way to install MySQL server 5.6.27 ?

@daviesalex
Copy link

We have been testing a version of this internally for a few weeks now, and it works very well for our specific version (which is MariaDB 10.0 and 10.1) on 50+ nodes (we found a few edge cases, which have been integrated - for example we had some databases with thousands of tables that initially caused a crazy number of metrics to be produced). I think it also works well on the main developers test environment (which is upstream MySQL 5.6).

My guess is that to get this merged, all versions of MySQL that are currently active (i.e. 5.5+, MariaDB, Percona and Oracle) will need to work although some of the metrics wont be available on older versions. @sparrc your advice on that is appreciated, but we are going to start thinking about how to make that work.

@sparrc
Copy link
Contributor

sparrc commented Feb 29, 2016

One option may be to specify the various queries in the config file, which can then have versions, tags, queries, etc. associated with them, like this contributor is doing for the postgres plugin: #705

@daviesalex
Copy link

@sparrc we will look at that (if we cant make it elegantly figure out what is available on its own - my preference is to not have to put huge amounts of JSON on every box if at all possible). My hope is MySQL is a little bit easier than PSQL - some metrics work on all versions, and for others we can test on startup if they are available or not. Expect an update later in the week from our team when we have tried some things. Thanks!

	shows global variables
	shows slave statuses
	shows size and count of binary log files
	shows information_schema.processlist stats
	shows perf table stats
	shows auto increments stats from information schema
	shows perf index stats
	shows table lock waits summary by table
	shows time and operations of event waits
	shows file event statuses
	shows events statements stats from perf_schema
	shows schema statistics
	refactored plugin, provided multiple fields per insert
@menardorama
Copy link
Contributor

@daviesalex I did that because Postgresql have a small amount of metrics available and imply to use some additionnal extensions.
Another thing is that you may want to implement some functional metrics to be collected; that's why I've worked on external query storage.

By the way, the queries are now stored directly in the telegraf.conf as a toml structure

@sparrc
Copy link
Contributor

sparrc commented Mar 16, 2016

@maksadbek just closing this to cleanup stale PRs, when you're ready please re-open as a separate PR

@sparrc sparrc closed this Mar 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants