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 #889

Closed
wants to merge 11 commits into from
Closed

Improved Mysql plugin #889

wants to merge 11 commits into from

Conversation

maksadbek
Copy link
Contributor

No description provided.

	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
@sparrc
Copy link
Contributor

sparrc commented Mar 21, 2016

I can't comment on individual lines of code because the diff is too large, but this should be changed in the config file:

  PerfEventsStatementsDigestTextLimit = 120
  PerfEventsStatementsLimit           = 250
  PerfEventsStatementsTimeLimit       = 86400
  TableSchemaDatabases                = []
  GatherProcessList                   = false
  GatherInfoSchemaAutoInc             = false
  GatherSlaveStatus                   = false
  GatherBinaryLogs                    = false
  GatherTableIOWaits                  = false
  GatherIndexIOWaits                  = false
  GatherTableSchema                   = false
  GatherFileEventsStats               = false
  GatherPerfEventsStatements          = false
  # Some queries we may want to run less often (such as SHOW GLOBAL VARIABLES)
  IntervalSlow                        = "30m"

to underscore and lower case:

perf_events_statement_limit

@sparrc
Copy link
Contributor

sparrc commented Mar 21, 2016

@maksadbek There are lots of times throughout the code where you declare variables as unsigned integers, and then convert them later to floats. What is the reason for that? Why not instantiate as floats? (see https://github.com/Maksadbek/telegraf/blob/master/plugins/inputs/mysql/mysql.go#L1367 for one example)

@sparrc
Copy link
Contributor

sparrc commented Mar 21, 2016

@sparrc
Copy link
Contributor

sparrc commented Mar 21, 2016

And lastly, you'll need to write a README for this plugin

@maksadbek
Copy link
Contributor Author

@sparrc thanks for codereview, I will fix code asap

@sparrc
Copy link
Contributor

sparrc commented Mar 23, 2016

thanks @maksadbek, just to make sure, will all of the default queries work on a default configured MySQL instance?

The README doesn't need to contain every single possible metric, but there should at least be thorough documentation of what each of these options collects.

@maksadbek
Copy link
Contributor Author

Hi @sparrc, yes the metrics that does not require an extra effort are turned on by defaults. Some of the queries works on MySQL 5.6 version with default configurations, as the testing MySQL is 5.5 such queries turned off by default.

Okay, I will also mention this on README and write briefly descriptions for specific metrics only

}
fields["syncs"] = i
}
// Send any remaining fields
Copy link

Choose a reason for hiding this comment

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

Hey @maksadbek - this will need to move outside the above for loop or it ends up sending metrics multiple times (to match the implementation in gatherGlobalVariables()) 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, thanks, fix it asap

@sparrc
Copy link
Contributor

sparrc commented Apr 6, 2016

@wrigtim @sebito91 This PR is a little hard for me to review, so have you guys been using this plugin already? How is it working for you? Any changes to suggest?

@daviesalex
Copy link

@sparrc , I will ask Tim and Seb to respond directly, but we certainly are using this in prod and have been using it for some time. We are very happy with it; it provides us visibility into vastly more than we got with the previous collector.

I've attached a sample Grafana dashboard we use to give you an idea of the metrics we can extract from this.

We have had a good number of production outages that we have been able to understand as a result of data that we collect now that we did not previously collect; its extremely helpful. Our internal MySQL DBAs are extremely impressed with it, too (and wrote many of the queries in the dashboard).

2016-04-11 17_07_09-grafana - mariadb
2016-04-11 17_06_50-grafana - mariadb

@wrigtim
Copy link

wrigtim commented Apr 11, 2016

@sparrc Hey Cam,

As Alex mentioned we have been running this across a number of MySQL hosts for some time and pinging @maksadbek directly with our findings or any potential issues. As the graphs above are a testament this significantly improves the visibility into MySQL.

We agree with the transpose of the "lock waits" tags/fields (to avoid mutation), and actually makes more sense for our queries (especially until math operators are supported in functions in InfluxDB itself).

Cheers,

Tim

@sebito91
Copy link
Contributor

Agreed with @wrigtim, @daviesalex here, this is currently running within our Production environment and is quite useful, helping avoid confusion and delay troubleshooting our mysql problems.

That being said, any formatting and/or syntax changes you would recommend are indeed welcome to ensure compatibility and sanity moving forward.

@sparrc
Copy link
Contributor

sparrc commented Apr 12, 2016

Okay, thanks for the input. I'm ready to merge this but would like to see more documentation. All input plugins need to have a README going forward based off of this one: https://github.com/influxdata/telegraf/blob/master/plugins/inputs/EXAMPLE_README.md

@maksadbek all metrics do not need to be documented, but you'll need to at least provide some amount of information on each of these config options (ideally you should put comments describing them into the config file too):

  perf_events_statements_digest_text_limit = 120
  perf_events_statements_limit             = 250
  perf_events_statements_time_limit        = 86400
  table_schema_databases                   = []
  gather_process_list                      = true
  gather_info_schema_auto_inc              = true
  gather_slave_status                      = true
  gather_binary_logs                       = false
  gather_table_io_waits                    = false
  gather_index_io_waits                    = false
  gather_table_schema                      = false
  gather_file_events_stats                 = false
  gather_perf_events_statements            = false

@maksadbek
Copy link
Contributor Author

Hi @sparrc , yes I am already writing detailed readme about each metrics, and soon push it. Okay, I will comment configs too

@maksadbek
Copy link
Contributor Author

Hi @sparrc
I have updated readme and sample configurations too. If there anything left, let me know.
Thanks

@sparrc sparrc closed this in b95a90d Apr 18, 2016
@sparrc
Copy link
Contributor

sparrc commented Apr 18, 2016

this will be available in 0.13, thanks again @maksadbek!

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.

5 participants