feat(database_observability.mysql): Add primary and unique index labels and a new row_count metric - #7095
Conversation
Mirrors the same addition on the postgres index_stats PR: fold these onto the existing size_bytes gauge as labels rather than a separate metric, since both are 1:1 with an index the same way schema/table/index already are. is_primary comes free from INDEX_NAME (MySQL always names the primary key index literally "PRIMARY"); is_unique is joined in from information_schema.statistics.NON_UNIQUE. Needed so the unused-index insight doesn't recommend dropping an index that's actually backing a primary key or unique constraint. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sourced from mysql.innodb_table_stats.n_rows, the sibling table to mysql.innodb_index_stats already used by index_stats.go for index size -- same refresh mechanism, same grant requirement already relied on there. Without a row-count signal, the missing-index insight can't tell a table that's genuinely being hammered with full scans from one that's just small enough that a full scan doesn't matter, the same gap this closed on the postgres side. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| } | ||
|
|
||
| func (c *TableStats) collectRowCount(ctx context.Context, ch chan<- prometheus.Metric) { | ||
| query := fmt.Sprintf(selectTableRowCount, buildExcludedSchemasClause(c.excludeSchemas)) |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
collectRowCount interpolates c.excludeSchemas into SQL before QueryContext executes it. Malicious schema configuration could alter the filter and expose data through the metrics queries.
More details about this
collectRowCount builds the SQL text with fmt.Sprintf(selectTableRowCount, buildExcludedSchemasClause(c.excludeSchemas)) and then executes that text through c.dbConnection.QueryContext. The value returned by buildExcludedSchemasClause is inserted into the SQL before the database receives it, rather than being passed as a query parameter.
If c.excludeSchemas can be influenced by deployment configuration or another attacker-controlled input, an attacker could supply a schema value such as tenant_a') OR 1=1 -- . If buildExcludedSchemasClause places that value inside a quoted NOT IN clause, the generated query could change from an exclusion filter into a condition that matches every row. When the metrics collection path calls collectRowCount, QueryContext would then return row counts for schemas that were intended to be excluded; a payload crafted to add a UNION or additional statement could expose other database data if the MySQL driver and account permit it. The same construction pattern is also present in collectNoIdxFetch with selectTableIOWaitsNoIndex and c.excludeSchemas, so the impact may include both table row-count and index-usage metrics.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by string-formatted-query.
We're currently testing semgrep's diff-aware PR comment feature on a subset of our repos-- if you run into issues or find this spammy, please reach out to @danny.cooper in slack and give feedback.
For backwards compatability with gosec, its best to use polyglot suppression comments of the following format for false positives:
// #nosec <gosec rule ID> nosemgrep: <semgrep rule ID>
You can view more details about this finding in the Semgrep AppSec Platform.
There was a problem hiding this comment.
/fp this is running inside an alloy instance and the param is fully under the user control via config file, so the risk is rather low.
Brief description of Pull Request
Adds two signals to the
index_stats/table_statscollectors, mirroring the same additions made to the postgres collectors in #7068:is_primary/is_uniquelabels onmysql_index_stats_size_bytes.is_primaryis derived fromINDEX_NAME = 'PRIMARY'(no extra query needed);is_uniqueis joined in frominformation_schema.statistics.NON_UNIQUE.mysql_table_stats_row_count, a new gauge sourced frommysql.innodb_table_stats.n_rows(the sibling table tomysql.innodb_index_stats, already used for index size).Pull Request Details
Both are needed for the missing/unused-index Knowledge Graph insight:
is_primary/is_unique, the unused-index insight has no signal to avoid recommending the removal of an index that's actually backing a primary key or unique constraint just because its usage counters look low. These are folded onto the existingsize_bytesgauge as labels rather than a separate metric, since both attributes are 1:1 with an index the same wayschema/table/indexalready are — this adds no additional series.row_count, the missing-index insight can't tell a table that's genuinely being hammered with full scans from one that's just small enough that a full scan doesn't matter — the same gap closed on the postgres side bypg_table_stats_row_count.Issue(s) fixed by this Pull Request
Notes to the Reviewer
PR Checklist
🤖 Generated with Claude Code