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

[receiver/mysql]: add scraping io_waits metrics #14328

Conversation

aboguszewski-sumo
Copy link
Member

Description:
This PR adds scraping metrics related to I/O waits for tables and indices. There are 4 new metrics. List of used attributes was taken from here: https://github.com/influxdata/telegraf/tree/master/plugins/inputs/mysql#tags

Link to tracking Issue:
#14138

Testing:
Unit tests for scraper and integration tests have been updated. The integration tests required putting some example data (empty schema, table and index), which is done in the setup.sh script.

Documentation:
Documentation automatically generated by mdatagen.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

Looks pretty good. A couple minor points to address though.

Comment on lines 102 to 107
query := `
SELECT OBJECT_SCHEMA, OBJECT_NAME, COUNT_DELETE, COUNT_FETCH, COUNT_INSERT, COUNT_UPDATE,
SUM_TIMER_DELETE, SUM_TIMER_FETCH, SUM_TIMER_INSERT, SUM_TIMER_UPDATE
FROM performance_schema.table_io_waits_summary_by_table
WHERE OBJECT_SCHEMA NOT IN ('mysql', 'performance_schema');
`
Copy link
Member

Choose a reason for hiding this comment

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

Presumably the extra whitespace is ignored by the database, but is it worth building the string in such a way where it is nicely formatted. I imagine users who see the query in the database's query log may find this format unusual.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it a bit:

SELECT OBJECT_SCHEMA, OBJECT_NAME, COUNT_DELETE, COUNT_FETCH, COUNT_INSERT, COUNT_UPDATE,
	SUM_TIMER_DELETE, SUM_TIMER_FETCH, SUM_TIMER_INSERT, SUM_TIMER_UPDATE
FROM performance_schema.table_io_waits_summary_by_table
WHERE OBJECT_SCHEMA NOT IN ('mysql', 'performance_schema');

Copy link
Member

Choose a reason for hiding this comment

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

This still seems odd to me. It reads nicely in code, but why should it contain return chars and tabs that are useful only for the golang code? How about something like this:

query := "SELECT OBJECT_SCHEMA, OBJECT_NAME, COUNT_DELETE, COUNT_FETCH, COUNT_INSERT, " +
	"COUNT_UPDATE, SUM_TIMER_DELETE, SUM_TIMER_FETCH, SUM_TIMER_INSERT, SUM_TIMER_UPDATE " +
	"FROM performance_schema.table_io_waits_summary_by_table " +
	"WHERE OBJECT_SCHEMA NOT IN ('mysql', 'performance_schema');"

It's simple and reads clearly in both go and sql domains.

Comment on lines 130 to 136
query := `
SELECT OBJECT_SCHEMA, OBJECT_NAME, ifnull(INDEX_NAME, 'NONE') as INDEX_NAME,
COUNT_FETCH, COUNT_INSERT, COUNT_UPDATE, COUNT_DELETE,
SUM_TIMER_FETCH, SUM_TIMER_INSERT, SUM_TIMER_UPDATE, SUM_TIMER_DELETE
FROM performance_schema.table_io_waits_summary_by_index_usage
WHERE OBJECT_SCHEMA NOT IN ('mysql', 'performance_schema');
`
Copy link
Member

Choose a reason for hiding this comment

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

Same here

receiver/mysqlreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/mysqlreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/mysqlreceiver/scraper.go Outdated Show resolved Hide resolved
receiver/mysqlreceiver/scraper.go Outdated Show resolved Hide resolved
@aboguszewski-sumo aboguszewski-sumo force-pushed the mysql-receiver-add-perf-io-waits branch 2 times, most recently from d6b4488 to 2c04a3e Compare September 21, 2022 07:32
@aboguszewski-sumo
Copy link
Member Author

Changes applied.

@aboguszewski-sumo aboguszewski-sumo force-pushed the mysql-receiver-add-perf-io-waits branch from 2c04a3e to bcb6c07 Compare September 21, 2022 07:40
@aboguszewski-sumo aboguszewski-sumo force-pushed the mysql-receiver-add-perf-io-waits branch from bcb6c07 to 62d963c Compare September 21, 2022 13:43
receiver/mysqlreceiver/scraper.go Outdated Show resolved Hide resolved
receiver/mysqlreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/mysqlreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/mysqlreceiver/metadata.yaml Outdated Show resolved Hide resolved
receiver/mysqlreceiver/metadata.yaml Outdated Show resolved Hide resolved
@aboguszewski-sumo aboguszewski-sumo force-pushed the mysql-receiver-add-perf-io-waits branch 2 times, most recently from 908badb to f4ce4e9 Compare September 22, 2022 11:36
@aboguszewski-sumo
Copy link
Member Author

A small update regarding the time metrics:
I tried to verify the unit of this metric. It turns out that it's stored in the database in picoseconds (10^-12 of a second, it was hard to find because the documentation was not clear, but I finally found this in MySQL source code). Hence, we need to convert this result to some other unit.
In my opinion, milliseconds sound reasonable, so I'm converting this into milliseconds. Let me know if some other unit would be better.

@djaglowski
Copy link
Member

It turns out that it's stored in the database in picoseconds...
Hence, we need to convert this result to some other unit. In my opinion, milliseconds sound reasonable, so I'm converting this into milliseconds. Let me know if some other unit would be better.

I agree with not using picoseconds. My suggestion would be to use nanoseconds and to represent it as an int (var ns int64 = picos/1000), rather than fractional unit. Technically, we are dropping a tiny amount of precision, but I am skeptical that it is meaningful in any context, or even accurate to that level.

@aboguszewski-sumo aboguszewski-sumo force-pushed the mysql-receiver-add-perf-io-waits branch 3 times, most recently from c7cbf7a to e2d743e Compare September 22, 2022 14:27
@djaglowski djaglowski merged commit e6083b5 into open-telemetry:main Sep 23, 2022
@djaglowski
Copy link
Member

Thanks for iterating @aboguszewski-sumo

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.

3 participants