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.

Sign up for GitHub

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

Jump to bottom

Fetch lock time and cpu time from performance schema #862

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Fetch lock time and cpu time from performance schema #862

merged 1 commit into from
Aug 28, 2024

Conversation

cristiangreco
Copy link
Contributor

@cristiangreco cristiangreco commented Aug 26, 2024 •
edited
Loading

Extend the query on events_statements_summary_by_digest to fetch SUM_LOCK_TIME and SUM_CPU_TIME and return them as counters. These are useful to report time spent running on the cpu (e.g. to sort results or run computations) vs waiting on locks (e.g. to acquire write lock on a table or row), by query digest.

@cristiangreco cristiangreco force-pushed the cristian/add-lock-cpu-time branch from 9841b7b to 4212787 Compare August 26, 2024 14:11
SuperQ
SuperQ reviewed Aug 26, 2024
View reviewed changes
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

I think we originally intentionally left these out for cardinality reasons. Would you mind documenting how they they are additionally useful in the PR description? That way we can have better history of "why" these are added by default.

@cristiangreco
Copy link
Contributor Author

I think we originally intentionally left these out for cardinality reasons. Would you mind documenting how they they are additionally useful in the PR description? That way we can have better history of "why" these are added by default.

Hey @SuperQ thanks for having a look! These metrics would have the same cardinality (in terms of labels values) as the other sibling metrics created out of the query events_statements_summary_by_digest. Or are we worried about adding new metrics in general? (I wanted to submit a followup PR to additionally expose QUANTILE_{95,99,999} so it'd be good to understand what is the approach to take)

@SuperQ
Copy link
Member

SuperQ commented Aug 26, 2024 •
edited
Loading

It's about adding new metrics in general. If you can explain what the metrics are useful for, that's all I'm asking about.

Exposing the quantiles is less useful, as they can't be aggregated between MySQL servers.

@cristiangreco
Copy link
Contributor Author

cristiangreco commented Aug 26, 2024 •
edited
Loading

I've added a short note on the PR description, could you please check that?

@SuperQ
Copy link
Member

SuperQ commented Aug 27, 2024

Needs a rebase.

@cristiangreco
Fetch lock time and cpu time from performance schema
3225ffa 
Extend the query on `events_statements_summary_by_digest` to fetch
`SUM_LOCK_TIME` and `SUM_CPU_TIME` and return them as counters.

Signed-off-by: Cristian Greco <cristian@regolo.cc>
@cristiangreco cristiangreco force-pushed the cristian/add-lock-cpu-time branch from 4212787 to 3225ffa Compare August 27, 2024 09:42
@cristiangreco
Copy link
Contributor Author

Needs a rebase.

Thanks so much! Just rebased.

@SuperQ
Copy link
Member

SuperQ commented Aug 27, 2024 •
edited
Loading

I had a think about the history of this. I wish we had written down the queries we experimented with in production at the time.

I think we tried adding CPU time originally, but we didn't really find it useful compared to total query time.

Lock time is probably a good one to have tho.

If cpu time = total time - lock time, maybe we don't need cpu time.

@cristiangreco
Copy link
Contributor Author

I think we tried adding CPU time originally, but we didn't really find it useful compared to total query time.

@SuperQ fair enough, if you prefer I can close this PR and open a new one adding just lock time?

@SuperQ
Copy link
Member

SuperQ commented Aug 27, 2024

I'm mostly looking for up-to-date expertise in MySQL here. It's been a while since I looked deeply at this data. I assume you've got data on the uselessness of these metrics.

@cristiangreco
Copy link
Contributor Author

I'm mostly looking for up-to-date expertise in MySQL here. It's been a while since I looked deeply at this data. I assume you've got data on the uselessness of these metrics.

So I'm not sure that cpu time = total time - lock time is true but can't disprove it either. I thought it was cheap enough that we could export both metrics anyway.

@SuperQ
Copy link
Member

SuperQ commented Aug 28, 2024

Ok, I guess this is fine as-is. If people complain about the additional cardinality we can look it over again.

@SuperQ SuperQ merged commit d65c5e6 into prometheus:main Aug 28, 2024
13 checks passed
@SuperQ SuperQ mentioned this pull request Aug 29, 2024
Closed
cristiangreco added a commit to grafana/mysqld_exporter that referenced this pull request Nov 8, 2024
@cristiangreco
Release 0.15.2
00db73e 
Changes:
* [FEATURE] Support for prometheus scrape timeout in probe endpoint prometheus#828
* [ENHANCEMENT] Support MySQL 8.4 replicas syntax prometheus#837
* [ENHANCEMENT] Fetch lock time and cpu time from performance schema prometheus#862
* [ENHANCEMENT] Add the instance struct to handle connections prometheus#859
* [ENHANCEMENT] Optimize code by using built-in constants in the standard lib prometheus#844
* [BUGFIX] Fix fetching tmpTables vs tmpDiskTables from performance_schema prometheus#853
* [BUGFIX] Skip SPACE_TYPE column for MariaDB >=10.5 prometheus#860
* [BUGFIX] Fixed parsing of timestamps with non-zero padded days prometheus#841
* [BUGFIX] Fix auto_increment metric collection errors caused by using collation in INFORMATION_SCHEMA searches prometheus#833
* [BUGFIX] Fix race condition in ReloadConfig prometheus#760
* [BUGFIX] Change processlist query to support ONLY_FULL_GROUP_BY sql_mode prometheus#684
* [BUGFIX] replication_applier_status_by_worker requires mysql 8.0 prometheus#683
* [BUGFIX] Update docker registry link in README.md prometheus#813
* [BUGFIX] Fix Docker run command and update documentation for cnf file handling prometheus#843
@cristiangreco cristiangreco mentioned this pull request Nov 8, 2024
Merged
cristiangreco added a commit to grafana/mysqld_exporter that referenced this pull request Nov 8, 2024
@cristiangreco
Release 0.15.2
dc0ff21 
Changes:
* [FEATURE] Support for prometheus scrape timeout in probe endpoint prometheus#828
* [ENHANCEMENT] Support MySQL 8.4 replicas syntax prometheus#837
* [ENHANCEMENT] Fetch lock time and cpu time from performance schema prometheus#862
* [ENHANCEMENT] Add the instance struct to handle connections prometheus#859
* [ENHANCEMENT] Optimize code by using built-in constants in the standard lib prometheus#844
* [BUGFIX] Fix fetching tmpTables vs tmpDiskTables from performance_schema prometheus#853
* [BUGFIX] Skip SPACE_TYPE column for MariaDB >=10.5 prometheus#860
* [BUGFIX] Fixed parsing of timestamps with non-zero padded days prometheus#841
* [BUGFIX] Fix auto_increment metric collection errors caused by using collation in INFORMATION_SCHEMA searches prometheus#833
* [BUGFIX] Fix race condition in ReloadConfig prometheus#760
* [BUGFIX] Change processlist query to support ONLY_FULL_GROUP_BY sql_mode prometheus#684
* [BUGFIX] replication_applier_status_by_worker requires mysql 8.0 prometheus#683
* [BUGFIX] Update docker registry link in README.md prometheus#813
* [BUGFIX] Fix Docker run command and update documentation for cnf file handling prometheus#843

Signed-off-by: Cristian Greco <cristian@regolo.cc>
cristiangreco added a commit to grafana/mysqld_exporter that referenced this pull request Nov 8, 2024
@cristiangreco
Release 0.16.0
2e49a1c 
Changes:
* [FEATURE] Support for prometheus scrape timeout in probe endpoint prometheus#828
* [ENHANCEMENT] Support MySQL 8.4 replicas syntax prometheus#837
* [ENHANCEMENT] Fetch lock time and cpu time from performance schema prometheus#862
* [ENHANCEMENT] Add the instance struct to handle connections prometheus#859
* [ENHANCEMENT] Optimize code by using built-in constants in the standard lib prometheus#844
* [BUGFIX] Fix fetching tmpTables vs tmpDiskTables from performance_schema prometheus#853
* [BUGFIX] Skip SPACE_TYPE column for MariaDB >=10.5 prometheus#860
* [BUGFIX] Fixed parsing of timestamps with non-zero padded days prometheus#841
* [BUGFIX] Fix auto_increment metric collection errors caused by using collation in INFORMATION_SCHEMA searches prometheus#833
* [BUGFIX] Fix race condition in ReloadConfig prometheus#760
* [BUGFIX] Change processlist query to support ONLY_FULL_GROUP_BY sql_mode prometheus#684
* [BUGFIX] replication_applier_status_by_worker requires mysql 8.0 prometheus#683
* [BUGFIX] Update docker registry link in README.md prometheus#813
* [BUGFIX] Fix Docker run command and update documentation for cnf file handling prometheus#843

Signed-off-by: Cristian Greco <cristian@regolo.cc>
cristiangreco added a commit to grafana/mysqld_exporter that referenced this pull request Nov 8, 2024
@cristiangreco
Release 0.16.0
73ace9d 
Changes:
* [CHANGE] Replace logging library go-kit/log with slog prometheus#885
* [FEATURE] Support for prometheus scrape timeout in probe endpoint prometheus#828
* [ENHANCEMENT] Support MySQL 8.4 replicas syntax prometheus#837
* [ENHANCEMENT] Fetch lock time and cpu time from performance schema prometheus#862
* [ENHANCEMENT] Add the instance struct to handle connections prometheus#859
* [ENHANCEMENT] Optimize code by using built-in constants in the standard lib prometheus#844
* [BUGFIX] Fix fetching tmpTables vs tmpDiskTables from performance_schema prometheus#853
* [BUGFIX] Skip SPACE_TYPE column for MariaDB >=10.5 prometheus#860
* [BUGFIX] Fixed parsing of timestamps with non-zero padded days prometheus#841
* [BUGFIX] Fix auto_increment metric collection errors caused by using collation in INFORMATION_SCHEMA searches prometheus#833
* [BUGFIX] Fix race condition in ReloadConfig prometheus#760
* [BUGFIX] Change processlist query to support ONLY_FULL_GROUP_BY sql_mode prometheus#684
* [BUGFIX] replication_applier_status_by_worker requires mysql 8.0 prometheus#683
* [BUGFIX] Update docker registry link in README.md prometheus#813
* [BUGFIX] Fix Docker run command and update documentation for cnf file handling prometheus#843
* [BUGFIX] info_schema_tables: do not collect the sys schema prometheus#879

Signed-off-by: Cristian Greco <cristian@regolo.cc>
cristiangreco added a commit to grafana/mysqld_exporter that referenced this pull request Nov 8, 2024
@cristiangreco
Release 0.16.0
da020b0 
Changes:
* [CHANGE] Replace logging library go-kit/log with slog prometheus#875
* [FEATURE] Support for prometheus scrape timeout in probe endpoint prometheus#828
* [ENHANCEMENT] Support MySQL 8.4 replicas syntax prometheus#837
* [ENHANCEMENT] Fetch lock time and cpu time from performance schema prometheus#862
* [ENHANCEMENT] Add the instance struct to handle connections prometheus#859
* [ENHANCEMENT] Optimize code by using built-in constants in the standard lib prometheus#844
* [BUGFIX] Fix fetching tmpTables vs tmpDiskTables from performance_schema prometheus#853
* [BUGFIX] Skip SPACE_TYPE column for MariaDB >=10.5 prometheus#860
* [BUGFIX] Fixed parsing of timestamps with non-zero padded days prometheus#841
* [BUGFIX] Fix auto_increment metric collection errors caused by using collation in INFORMATION_SCHEMA searches prometheus#833
* [BUGFIX] Fix race condition in ReloadConfig prometheus#760
* [BUGFIX] Change processlist query to support ONLY_FULL_GROUP_BY sql_mode prometheus#684
* [BUGFIX] replication_applier_status_by_worker requires mysql 8.0 prometheus#683
* [BUGFIX] Update docker registry link in README.md prometheus#813
* [BUGFIX] Fix Docker run command and update documentation for cnf file handling prometheus#843
* [BUGFIX] info_schema_tables: do not collect the sys schema prometheus#879

Signed-off-by: Cristian Greco <cristian@regolo.cc>
SuperQ pushed a commit that referenced this pull request Nov 8, 2024
@cristiangreco
Release 0.16.0 (#889)
cac4d10 
Changes:
* [CHANGE] Replace logging library go-kit/log with slog #875
* [FEATURE] Support for prometheus scrape timeout in probe endpoint #828
* [ENHANCEMENT] Support MySQL 8.4 replicas syntax #837
* [ENHANCEMENT] Fetch lock time and cpu time from performance schema #862
* [ENHANCEMENT] Add the instance struct to handle connections #859
* [ENHANCEMENT] Optimize code by using built-in constants in the standard lib #844
* [BUGFIX] Fix fetching tmpTables vs tmpDiskTables from performance_schema #853
* [BUGFIX] Skip SPACE_TYPE column for MariaDB >=10.5 #860
* [BUGFIX] Fixed parsing of timestamps with non-zero padded days #841
* [BUGFIX] Fix auto_increment metric collection errors caused by using collation in INFORMATION_SCHEMA searches #833
* [BUGFIX] Fix race condition in ReloadConfig #760
* [BUGFIX] Change processlist query to support ONLY_FULL_GROUP_BY sql_mode #684
* [BUGFIX] replication_applier_status_by_worker requires mysql 8.0 #683
* [BUGFIX] Update docker registry link in README.md #813
* [BUGFIX] Fix Docker run command and update documentation for cnf file handling #843
* [BUGFIX] info_schema_tables: do not collect the sys schema #879

Signed-off-by: Cristian Greco <cristian@regolo.cc>
SuperQ added a commit that referenced this pull request Nov 8, 2024
@SuperQ
Release v0.16.0
3f29ae4 
* [CHANGE] Replace logging library go-kit/log with slog #875
* [FEATURE] Support for prometheus scrape timeout in probe endpoint #828
* [ENHANCEMENT] Support MySQL 8.4 replicas syntax #837
* [ENHANCEMENT] Fetch lock time and cpu time from performance schema #862
* [ENHANCEMENT] Add the instance struct to handle connections #859
* [ENHANCEMENT] Optimize code by using built-in constants in the standard lib #844
* [BUGFIX] Fix fetching tmpTables vs tmpDiskTables from performance_schema #853
* [BUGFIX] Skip SPACE_TYPE column for MariaDB >=10.5 #860
* [BUGFIX] Fixed parsing of timestamps with non-zero padded days #841
* [BUGFIX] Fix auto_increment metric collection errors caused by using collation in INFORMATION_SCHEMA searches #833
* [BUGFIX] Fix race condition in ReloadConfig #760
* [BUGFIX] Change processlist query to support ONLY_FULL_GROUP_BY sql_mode #684
* [BUGFIX] replication_applier_status_by_worker requires mysql 8.0 #683
* [BUGFIX] Update docker registry link in README.md #813
* [BUGFIX] Fix Docker run command and update documentation for cnf file handling #843
* [BUGFIX] info_schema_tables: do not collect the sys schema #879

Signed-off-by: SuperQ <superq@gmail.com>
@SuperQ SuperQ mentioned this pull request Nov 8, 2024
Merged
SuperQ added a commit that referenced this pull request Nov 8, 2024
@SuperQ
Release v0.16.0 (#891)
c366424 
* [CHANGE] Replace logging library go-kit/log with slog #875
* [FEATURE] Support for prometheus scrape timeout in probe endpoint #828
* [ENHANCEMENT] Support MySQL 8.4 replicas syntax #837
* [ENHANCEMENT] Fetch lock time and cpu time from performance schema #862
* [ENHANCEMENT] Add the instance struct to handle connections #859
* [ENHANCEMENT] Optimize code by using built-in constants in the standard lib #844
* [BUGFIX] Fix fetching tmpTables vs tmpDiskTables from performance_schema #853
* [BUGFIX] Skip SPACE_TYPE column for MariaDB >=10.5 #860
* [BUGFIX] Fixed parsing of timestamps with non-zero padded days #841
* [BUGFIX] Fix auto_increment metric collection errors caused by using collation in INFORMATION_SCHEMA searches #833
* [BUGFIX] Fix race condition in ReloadConfig #760
* [BUGFIX] Change processlist query to support ONLY_FULL_GROUP_BY sql_mode #684
* [BUGFIX] replication_applier_status_by_worker requires mysql 8.0 #683
* [BUGFIX] Update docker registry link in README.md #813
* [BUGFIX] Fix Docker run command and update documentation for cnf file handling #843
* [BUGFIX] info_schema_tables: do not collect the sys schema #879

Signed-off-by: SuperQ <superq@gmail.com>
@matthewnolf matthewnolf mentioned this pull request Nov 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperQ SuperQ SuperQ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cristiangreco @SuperQ