Issues with MySQL version tag

[fxedel]

Relevant telegraf.conf

[[inputs.mysql]]
  # ...
  gather_global_variables = true

Logs from Telegraf

N/A

System info

Telegraf 1.22.3

Docker

No response

Steps to reproduce

Case 1:

• Make sure that SHOW GLOBAL VARIABLES returns more than 20 variables and that the version variable is not among the first 20

Case 2:

• Make sure that SHOW GLOBAL VARIABLES returns variables that include the string version in their name, like innodb_version, tls_version, replica_type_conversions

Expected behavior

Case 1:

• All metrics should have the version tag set correctly

Case 2:

• Only the version variable is added as tag (worth discussing?)

Actual behavior

Case 1:

• The first metric with the first 20 fields has no version tag

Case 2:

• All variables with the string version in their name are added as tag

Additional info

The gatherGlobalVariables function in the mysql plugin tries adding the MySQL as version instead of adding it as a field. This logic is flawed, however, for two reasons:

1. Metrics are sent in packages of 20 fields and tags are copied when using acc.AddFields("..", fields, tags), so if the version variable isn't among the first 20 variables, these are sent without the tag. As soon as the version variable has been found, all following fields in this gather have the tag.
2. Every variable containing the string version is considered as a version tag. While this might be intended for variables like version_comment, I suspect adding variables like tls_version was not intended, and variables like replica_type_conversions are clearly false positives.

When implementing #10987, I have also written a test that demonstrates these issues:

telegraf/plugins/inputs/mysql/mysql_test.go

Lines 274 to 321 in 2bf2322

	// TODO: enable following failing tests by fixing the plugin:
	// {
	// "version tag with many fields", // metrics are sent in packages of 20 fields, so the tags can vary (but should not)
	// fields{
	// {"__test__var01", "text", "text", false},
	// {"__test__var02", "text", "text", false},
	// {"__test__var03", "text", "text", false},
	// {"__test__var04", "text", "text", false},
	// {"__test__var05", "text", "text", false},
	// {"__test__var06", "text", "text", false},
	// {"__test__var07", "text", "text", false},
	// {"__test__var08", "text", "text", false},
	// {"__test__var09", "text", "text", false},
	// {"__test__var10", "text", "text", false},
	// {"__test__var11", "text", "text", false},
	// {"__test__var12", "text", "text", false},
	// {"__test__var13", "text", "text", false},
	// {"__test__var14", "text", "text", false},
	// {"__test__var15", "text", "text", false},
	// {"__test__var16", "text", "text", false},
	// {"__test__var17", "text", "text", false},
	// {"__test__var18", "text", "text", false},
	// {"__test__var19", "text", "text", false},
	// {"__test__var20", "text", "text", false},
	// {"__test__var21", "text", "text", false},
	// {"version", "8.0.27-0ubuntu0.20.04.1", "8.0.27-0ubuntu0.20.04.1", false},
	// },
	// tags{"server": "127.0.0.1:3306", "version": "8.0.27-0ubuntu0.20.04.1"},
	// },
	// {
	// "misinterpreted version tags", // e.g. contains("version") also matches "conversion"
	// fields{
	// {"admin_tls_version", "TLSv1,TLSv1.1,TLSv1.2,TLSv1.3", "TLSv1,TLSv1.1,TLSv1.2,TLSv1.3", false},
	// {"innodb_version", "8.0.27", "8.0.27", false},
	// {"protocol_version", "10", "10", false},
	// {"replica_type_conversions", "", "", false},
	// {"server", "127.0.0.1:3306", "127.0.0.1:3306", false},
	// {"slave_type_conversions", "", "", false},
	// {"tls_version", "TLSv1,TLSv1.1,TLSv1.2,TLSv1.3", "TLSv1,TLSv1.1,TLSv1.2,TLSv1.3", false},
	// {"version", "8.0.27-0ubuntu0.20.04.1", "8.0.27-0ubuntu0.20.04.1", false},
	// {"version_comment", "(Ubuntu)", "(Ubuntu)", false},
	// {"version_compile_machine", "x86_64", "x86_64", false},
	// {"version_compile_os", "Linux", "Linux", false},
	// {"version_compile_zlib", "1.2.11", "1.2.11", false},
	// },
	// tags{"server": "127.0.0.1:3306", "version": "8.0.27-0ubuntu0.20.04.1"}, // TODO: Discuss which version tags should be used as tags
	// },

Fixing the first issue requires reading all rows, storing them in a map and remembering the version tag. Then iterate through it again and send the metrics.

Fixing the second issue requires a discussion about which variables to keep as tag. I'd suggest for version and variables starting with version_ only. This is technically a breaking change, though, if someone actually uses the tls_version tag for example.

One could discuss whether this tag is necessary at all. The mysql instance is already uniquely identified with the server tag, what's the use of a version tag? And why do we only have it for the mysql_variables metric?

[reimda]

Thanks for opening an issue about this.

Every variable containing the string version is considered as a version tag

When checking for version tags, would it be enough to split the name on underscore and then look for the complete word 'version'? That would work for innodb_version and tls_version but replica_type_conversions wouldn't be a false positive.

One could discuss whether this tag is necessary at all

Since we don't know all the ways people use this, we need to preserve backward compatibility. I think it's a good idea to remove the false positives like replica_type_conversions matching 'version' which are obvious coding errors, but I don't think we should remove version tags.

Would you like to submit a PR to implement the changes you outlined? Thanks!