Skip to content

Commit fa4b1ee

Browse files
pkudikyalapkudikyala
committed
Updated the review comments.
1 parent 2c85ce3 commit fa4b1ee

3 files changed

Lines changed: 49 additions & 30 deletions

File tree

‎src/backup_metrics.go‎

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ var backupMetrics = map[string][]interface{}{
1212
"db.backupActive.logical": {"logical_backup_active", metric.GAUGE},
1313
"db.backupActive.physical": {"physical_backup_active", metric.GAUGE},
1414
"db.backupActive.tableLock": {"table_lock_active", metric.GAUGE},
15-
"db.backupActive.other": {"other_backup_active", metric.GAUGE},
1615
}
1716

1817
// backupHistoryMetrics defines metrics for historical backup operations from performance_schema
@@ -59,14 +58,12 @@ var backupHistoryMetrics = map[string][]interface{}{
5958
//
6059
// - Exclude specific users by filtering on trx_mysql_thread_id
6160
//
62-
// Note: This query is optimized for MariaDB 10.5+. For MySQL 8.0+, use PROCESSLIST_ID instead of OWNER_THREAD_ID
6361
const backupMetricsQuery = `
6462
SELECT
6563
COALESCE(b.physical_backup_active, 0) + COALESCE(b.table_lock_active, 0) + COALESCE(l.logical_backup_active, 0) AS total_backup_active,
6664
COALESCE(l.logical_backup_active, 0) AS logical_backup_active,
6765
COALESCE(b.physical_backup_active, 0) AS physical_backup_active,
68-
COALESCE(b.table_lock_active, 0) AS table_lock_active,
69-
0 AS other_backup_active
66+
COALESCE(b.table_lock_active, 0) AS table_lock_active
7067
FROM (
7168
SELECT
7269
-- Physical backups: FLUSH TABLES WITH READ LOCK (MariaBackup, Percona XtraBackup)
@@ -77,14 +74,18 @@ FROM (
7774
AND LOCK_STATUS = 'GRANTED'
7875
THEN OWNER_THREAD_ID
7976
END) AS physical_backup_active,
80-
-- Table locks: LOCK TABLES ... READ/WRITE
81-
-- Creates SHARED_READ or SHARED_WRITE locks on user tables
82-
-- Includes locks on mysql.* schema as these are common in backup operations
77+
-- Table locks: LOCK TABLES ... READ/WRITE (mysqldump --lock-tables, write-lock backups)
78+
-- SHARED_READ: read locks (LOCK TABLES t READ)
79+
-- SHARED_NO_READ_WRITE/EXCLUSIVE: write locks (LOCK TABLES t WRITE)
80+
-- LOCK_DURATION IN ('EXPLICIT', 'TRANSACTION') covers both engines:
81+
-- MySQL: LOCK TABLES creates EXPLICIT duration locks
82+
-- MariaDB: LOCK TABLES creates TRANSACTION duration locks
8383
COUNT(DISTINCT CASE
8484
WHEN OBJECT_TYPE = 'TABLE'
85-
AND LOCK_TYPE IN ('SHARED_READ', 'SHARED_WRITE', 'SHARED_NO_READ_WRITE', 'EXCLUSIVE')
85+
AND LOCK_TYPE IN ('SHARED_READ', 'SHARED_NO_READ_WRITE', 'EXCLUSIVE')
8686
AND LOCK_STATUS = 'GRANTED'
87-
AND OBJECT_SCHEMA NOT IN ('performance_schema', 'information_schema')
87+
AND LOCK_DURATION IN ('EXPLICIT', 'TRANSACTION')
88+
AND OBJECT_SCHEMA NOT IN ('performance_schema', 'information_schema', 'sys')
8889
THEN OWNER_THREAD_ID
8990
END) AS table_lock_active
9091
FROM performance_schema.metadata_locks
@@ -116,18 +117,18 @@ const backupHistoryMetricsQuery = `
116117
SELECT
117118
COALESCE(SUM(CASE WHEN SQL_TEXT LIKE '%START TRANSACTION WITH CONSISTENT SNAPSHOT%' THEN 1 ELSE 0 END), 0)
118119
+ COALESCE(SUM(CASE WHEN SQL_TEXT LIKE '%FLUSH TABLES%WITH READ LOCK%' THEN 1 ELSE 0 END), 0)
119-
+ COALESCE(SUM(CASE WHEN SQL_TEXT LIKE '%LOCK TABLES%' THEN 1 ELSE 0 END), 0) AS total_backup_history,
120-
SUM(CASE WHEN SQL_TEXT LIKE '%START TRANSACTION WITH CONSISTENT SNAPSHOT%' THEN 1 ELSE 0 END) AS logical_backup_count,
121-
AVG(CASE WHEN SQL_TEXT LIKE '%START TRANSACTION WITH CONSISTENT SNAPSHOT%' THEN TIMER_WAIT/1000000000000 ELSE NULL END) AS logical_backup_avg_duration,
122-
MAX(CASE WHEN SQL_TEXT LIKE '%START TRANSACTION WITH CONSISTENT SNAPSHOT%' THEN TIMER_WAIT/1000000000000 ELSE NULL END) AS logical_backup_max_duration,
123-
SUM(CASE WHEN SQL_TEXT LIKE '%FLUSH TABLES%WITH READ LOCK%' THEN 1 ELSE 0 END) AS physical_backup_count,
124-
AVG(CASE WHEN SQL_TEXT LIKE '%FLUSH TABLES%WITH READ LOCK%' THEN TIMER_WAIT/1000000000000 ELSE NULL END) AS physical_backup_avg_duration,
125-
MAX(CASE WHEN SQL_TEXT LIKE '%FLUSH TABLES%WITH READ LOCK%' THEN TIMER_WAIT/1000000000000 ELSE NULL END) AS physical_backup_max_duration,
126-
SUM(CASE WHEN SQL_TEXT LIKE '%LOCK TABLES%' THEN 1 ELSE 0 END) AS table_lock_count,
127-
AVG(CASE WHEN SQL_TEXT LIKE '%LOCK TABLES%' THEN TIMER_WAIT/1000000000000 ELSE NULL END) AS table_lock_avg_duration,
128-
MAX(CASE WHEN SQL_TEXT LIKE '%LOCK TABLES%' THEN TIMER_WAIT/1000000000000 ELSE NULL END) AS table_lock_max_duration
120+
+ COALESCE(SUM(CASE WHEN SQL_TEXT LIKE 'LOCK TABLES %' THEN 1 ELSE 0 END), 0) AS total_backup_history,
121+
COALESCE(SUM(CASE WHEN SQL_TEXT LIKE '%START TRANSACTION WITH CONSISTENT SNAPSHOT%' THEN 1 ELSE 0 END), 0) AS logical_backup_count,
122+
COALESCE(AVG(CASE WHEN SQL_TEXT LIKE '%START TRANSACTION WITH CONSISTENT SNAPSHOT%' THEN TIMER_WAIT/1000000000000 ELSE NULL END), 0) AS logical_backup_avg_duration,
123+
COALESCE(MAX(CASE WHEN SQL_TEXT LIKE '%START TRANSACTION WITH CONSISTENT SNAPSHOT%' THEN TIMER_WAIT/1000000000000 ELSE NULL END), 0) AS logical_backup_max_duration,
124+
COALESCE(SUM(CASE WHEN SQL_TEXT LIKE '%FLUSH TABLES%WITH READ LOCK%' THEN 1 ELSE 0 END), 0) AS physical_backup_count,
125+
COALESCE(AVG(CASE WHEN SQL_TEXT LIKE '%FLUSH TABLES%WITH READ LOCK%' THEN TIMER_WAIT/1000000000000 ELSE NULL END), 0) AS physical_backup_avg_duration,
126+
COALESCE(MAX(CASE WHEN SQL_TEXT LIKE '%FLUSH TABLES%WITH READ LOCK%' THEN TIMER_WAIT/1000000000000 ELSE NULL END), 0) AS physical_backup_max_duration,
127+
COALESCE(SUM(CASE WHEN SQL_TEXT LIKE 'LOCK TABLES %' THEN 1 ELSE 0 END), 0) AS table_lock_count,
128+
COALESCE(AVG(CASE WHEN SQL_TEXT LIKE 'LOCK TABLES %' THEN TIMER_WAIT/1000000000000 ELSE NULL END), 0) AS table_lock_avg_duration,
129+
COALESCE(MAX(CASE WHEN SQL_TEXT LIKE 'LOCK TABLES %' THEN TIMER_WAIT/1000000000000 ELSE NULL END), 0) AS table_lock_max_duration
129130
FROM performance_schema.events_statements_history
130-
WHERE (SQL_TEXT LIKE '%LOCK TABLES%'
131+
WHERE (SQL_TEXT LIKE 'LOCK TABLES %'
131132
OR SQL_TEXT LIKE '%FLUSH TABLES%WITH READ LOCK%'
132133
OR SQL_TEXT LIKE '%START TRANSACTION WITH CONSISTENT SNAPSHOT%')
133134
`
@@ -145,18 +146,16 @@ SELECT
145146
COALESCE(p.table_lock_active, 0) + COALESCE(t.logical_backup_active, 0) AS total_backup_active,
146147
COALESCE(t.logical_backup_active, 0) AS logical_backup_active,
147148
0 AS physical_backup_active,
148-
COALESCE(p.table_lock_active, 0) AS table_lock_active,
149-
0 AS other_backup_active
149+
COALESCE(p.table_lock_active, 0) AS table_lock_active
150150
FROM (
151151
SELECT
152152
-- Table locks: Sessions executing LOCK TABLES statements
153+
-- Using 'LOCK TABLES %' (starts-with) to avoid matching UNLOCK TABLES statements
153154
COUNT(DISTINCT CASE
154-
WHEN INFO LIKE '%LOCK TABLES%'
155-
AND COMMAND != 'Sleep'
155+
WHEN INFO LIKE 'LOCK TABLES %'
156156
THEN ID
157157
END) AS table_lock_active
158158
FROM information_schema.processlist
159-
WHERE ID IS NOT NULL
160159
) p,
161160
(
162161
SELECT

‎src/backup_metrics_test.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ func TestBackupMetricsMap(t *testing.T) {
1414
"db.backupActive.logical",
1515
"db.backupActive.physical",
1616
"db.backupActive.tableLock",
17-
"db.backupActive.other",
1817
}
1918

2019
for _, metricName := range expectedMetrics {
@@ -38,11 +37,11 @@ func TestBackupMetricsQuery(t *testing.T) {
3837
assert.Contains(t, backupMetricsQuery, "logical_backup_active", "Query should count logical backups")
3938
assert.Contains(t, backupMetricsQuery, "physical_backup_active", "Query should count physical backups")
4039
assert.Contains(t, backupMetricsQuery, "table_lock_active", "Query should count table locks")
41-
assert.Contains(t, backupMetricsQuery, "other_backup_active", "Query should count other backups")
4240
assert.Contains(t, backupMetricsQuery, "performance_schema.metadata_locks", "Query should use metadata_locks for detection")
4341
assert.Contains(t, backupMetricsQuery, "information_schema.innodb_trx", "Query should detect logical backups")
4442
assert.Contains(t, backupMetricsQuery, "BACKUP_FTWRL%", "Query should detect physical backups via FTWRL backup lock")
4543
assert.Contains(t, backupMetricsQuery, "LOCK_TYPE", "Query should detect table locks")
44+
assert.Contains(t, backupMetricsQuery, "LOCK_DURATION IN ('EXPLICIT', 'TRANSACTION')", "Query should cover both MySQL (EXPLICIT) and MariaDB (TRANSACTION) LOCK TABLES behaviour")
4645
assert.Contains(t, backupMetricsQuery, "OWNER_THREAD_ID", "Query should identify backup sessions by thread ID")
4746
}
4847

@@ -56,7 +55,6 @@ func TestBackupMetricsColumnMapping(t *testing.T) {
5655
{"db.backupActive.logical", "logical_backup_active"},
5756
{"db.backupActive.physical", "physical_backup_active"},
5857
{"db.backupActive.tableLock", "table_lock_active"},
59-
{"db.backupActive.other", "other_backup_active"},
6058
}
6159

6260
for _, test := range tests {
@@ -115,7 +113,7 @@ func TestBackupHistoryMetricsQuery(t *testing.T) {
115113
assert.Contains(t, backupHistoryMetricsQuery, "performance_schema.events_statements_history", "Query should use events_statements_history")
116114
assert.Contains(t, backupHistoryMetricsQuery, "START TRANSACTION WITH CONSISTENT SNAPSHOT", "Query should detect logical backups")
117115
assert.Contains(t, backupHistoryMetricsQuery, "FLUSH TABLES%WITH READ LOCK", "Query should detect physical backups")
118-
assert.Contains(t, backupHistoryMetricsQuery, "LOCK TABLES", "Query should detect table locks")
116+
assert.Contains(t, backupHistoryMetricsQuery, "LOCK TABLES %", "Query should detect table locks without matching UNLOCK TABLES")
119117
assert.Contains(t, backupHistoryMetricsQuery, "TIMER_WAIT/1000000000000", "Query should convert timer to seconds")
120118
}
121119

@@ -148,6 +146,6 @@ func TestBackupHistoryMetricsColumnMapping(t *testing.T) {
148146

149147
func TestBackupMetricsCount(t *testing.T) {
150148
// Verify the expected number of metrics
151-
assert.Equal(t, 5, len(backupMetrics), "Should have 5 active backup metrics")
149+
assert.Equal(t, 4, len(backupMetrics), "Should have 4 active backup metrics")
152150
assert.Equal(t, 10, len(backupHistoryMetrics), "Should have 10 historical backup metrics")
153151
}

‎src/mysql_test.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,28 @@ func (d testdb) getBackupQuery() string {
121121
return backupMetricsQuery
122122
}
123123

124+
type testdbFallback struct {
125+
testdb
126+
}
127+
128+
func (d testdbFallback) getBackupQuery() string {
129+
return backupMetricsQueryFallback
130+
}
131+
132+
func TestGetBackupQueryFallback(t *testing.T) {
133+
db := testdbFallback{}
134+
query := db.getBackupQuery()
135+
136+
assert.Equal(t, backupMetricsQueryFallback, query, "Fallback db should return backupMetricsQueryFallback")
137+
assert.Contains(t, query, "information_schema.processlist", "Fallback query should use processlist")
138+
assert.Contains(t, query, "information_schema.innodb_trx", "Fallback query should detect logical backups via innodb_trx")
139+
assert.Contains(t, query, "total_backup_active", "Fallback query should return total_backup_active")
140+
assert.Contains(t, query, "logical_backup_active", "Fallback query should return logical_backup_active")
141+
assert.Contains(t, query, "physical_backup_active", "Fallback query should return physical_backup_active")
142+
assert.Contains(t, query, "table_lock_active", "Fallback query should return table_lock_active")
143+
assert.Contains(t, query, "LOCK TABLES %", "Fallback query should use starts-with pattern to avoid matching UNLOCK TABLES")
144+
}
145+
124146
func TestGetRawData(t *testing.T) {
125147
database := testdb{
126148
inventory: map[string]interface{}{

0 commit comments

Comments
 (0)