Const strings where appropriate and improved test for statistics

Changed the get_variable_id_for_name, and knows_variable_name to use const string &. Changed immutable strings to to be const as well. Added details comment to test_admin_stats-t.cpp test. In the test code, improved documentation, set a valid interval for metrics insertion, and added the commands to set the interval. Changed the sleep time to be the inteval plus a second should be long enough to let new metrics data be added. Put additional sleep before test 2. Fixed incorrect column index usage in test 4 result set. Reformulated test 5, to check the number of records inserted after new interval is correct.

include/ProxySQL_Statistics.hpp

@@ -147,13 +147,13 @@ class ProxySQL_Statistics {
 	 * @param variable_name The string variable name identifier
 	 * @return Integer variable id for the given variable name
 	 */
-	int64_t get_variable_id_for_name(std::string variable_name);
+	int64_t get_variable_id_for_name(const std::string & variable_name);
 
 	/** @brief If the variable_name_id_map is empty, then load its contents from all of the history_variables_lookup table records */
 	void load_variable_name_id_map_if_empty();
 
 	/** @return True if the given variable_name is registered in the variable_name_id_map. */
-	bool knows_variable_name(std::string variable_name) const;
+	bool knows_variable_name(const std::string & variable_name) const;
 
 	private:
 	/** @brief Map with the key being the variable_name and the value being the variable_id, used for history_mysql_variables data. Matches the history_mysql_variables_lookup. */

lib/ProxySQL_Statistics.cpp

@@ -1080,12 +1080,12 @@ void ProxySQL_Statistics::MySQL_Query_Cache_sets(SQLite3_result *resultset) {
 	}
 }
 
-bool ProxySQL_Statistics::knows_variable_name(std::string variable_name) const
+bool ProxySQL_Statistics::knows_variable_name(const std::string & variable_name) const
 {
 	return (variable_name_id_map.find(variable_name) != variable_name_id_map.end());
 }
 
-int64_t ProxySQL_Statistics::get_variable_id_for_name(std::string variable_name) {
+int64_t ProxySQL_Statistics::get_variable_id_for_name(const std::string & variable_name) {
 	lock_guard<mutex> lock(mu);
 
 	int64_t variable_id = -1; // Negative value indicates not yet found.
@@ -1100,7 +1100,7 @@ int64_t ProxySQL_Statistics::get_variable_id_for_name(std::string variable_name)
 		char *error = NULL;
 
 		auto select_var_id = [&]() -> int64_t {
-			string var_id_query = "SELECT variable_id FROM history_mysql_status_variables_lookup WHERE variable_name=\"" + variable_name + "\"";
+			const string var_id_query = "SELECT variable_id FROM history_mysql_status_variables_lookup WHERE variable_name=\"" + variable_name + "\"";
 			statsdb_disk->execute_statement(var_id_query.c_str(), &error , &cols , &affected_rows , &result);
 
 			if (error) {

test/tap/tests/test_admin_stats-t.cpp

@@ -1,6 +1,14 @@
 /**
  * @file test_admin_stats-t.cpp
  * @brief This tests the Statistics module and its lookup code and tables.
+ * 
+ * @details The following tests are performed :
+ *  1. The lookup table has at least 50 rows generated after first metrics inserted
+ *  2. There should be multiple distinct timestaps present in the history table
+ *  3. The number of distinct variable_id's in the lookup and history tables should match
+ *  4. Each variable_id has the same number of rows in history table
+ *  5. The number of rows in the history table increases appropriately after insert interval changes
+ *
  * @date 2021-10-28
  */
 
@@ -18,6 +26,7 @@
 #include "utils.h"
 
 using std::string;
+using std::to_string;
 
 int main(int argc, char** argv) {
 	CommandLine cl;
@@ -47,14 +56,16 @@ int main(int argc, char** argv) {
 	}
 
 	// Setup the interval of how often new status entries are created
-	uint16_t new_stats_interval_sec = 2;
+	uint16_t new_stats_interval_sec = 5; // @note: valid values 5, 10, 30, 60, 120, 300
 
-	// @TODO: run command to set the interval
+	// Run command to set the interval
+	MYSQL_QUERY(proxysql_admin, ("SET admin-stats_mysql_connections=" + to_string(new_stats_interval_sec)).c_str());
+	MYSQL_QUERY(proxysql_admin, "LOAD ADMIN VARIABLES TO RUNTIME");	
 
 	// If on a fresh install, wait long enough for the first run of stats to be created
 	// The lookup table will be empty until the first run!
-	sleep(new_stats_interval_sec * 2);
-
+	sleep(new_stats_interval_sec + 1);
+	
     // Test 1: Lookup table has at least 50 rows
 	int64_t lookup_row_count = 0;
     MYSQL_QUERY(proxysql_admin, "SELECT COUNT(*) FROM history_mysql_status_variables_lookup");
@@ -72,7 +83,9 @@ int main(int argc, char** argv) {
 		lookup_row_count
 	);
 
-	// Test 2: Distinct timestamps in 
+	sleep(new_stats_interval_sec + 1);
+
+	// Test 2: There are multiple distinct timestaps present in history table
 	int64_t distinct_timestamp_count = 0; 
 	MYSQL_QUERY(proxysql_admin, "SELECT COUNT(DISTINCT(timestamp)) FROM history_mysql_status_variables");
 	result = mysql_store_result(proxysql_admin);
@@ -118,20 +131,20 @@ int main(int argc, char** argv) {
 		distinct_var_ids_in_lookup
 	);
 
-	// Test 4: Check that for each variable_id has same number of rows in history table
+	// Test 4: Each variable_id has same number of rows in history table
 
-	// @note: As the CI tests are done on a fresh install, these should match in this instance
+	// As the CI tests are done on a fresh install, these should match in this instance
 	// In practice, they could differ if new metrics variables are added.
 
 	std::vector<int64_t> rows_per_var_id;
 	time_t two_mins_ago = time(nullptr) - 60*2;
-	string query = "SELECT variable_id, COUNT(*) FROM history_mysql_status_variables WHERE timestamp < " + std::to_string(two_mins_ago) + " GROUP BY variable_id";
+	const string query = "SELECT variable_id, COUNT(*) FROM history_mysql_status_variables WHERE timestamp < " + to_string(two_mins_ago) + " GROUP BY variable_id";
 	MYSQL_QUERY(proxysql_admin, query.c_str());
 	result = mysql_store_result(proxysql_admin);
 
 	for (int i = 0; i < result->row_count; i++) {
 		row = mysql_fetch_row(result);
-		rows_per_var_id.push_back(strtoll(row[0], nullptr, 10));
+		rows_per_var_id.push_back(strtoll(row[1], nullptr, 10));
 	}
 
 	mysql_free_result(result);
@@ -143,7 +156,7 @@ int main(int argc, char** argv) {
 		"Each variable_id in the history table has the same number of rows."
 	);
 
-	// Test 5: Number of rows in history table increases after connections stat changes
+	// Test 5: Number of rows in history table increases appropriately after insert interval changes
 	int64_t history_rows_before = 0;
 	int64_t history_rows_after = 0;
 
@@ -156,8 +169,13 @@ int main(int argc, char** argv) {
 
 	mysql_free_result(result);
 
-	MYSQL_QUERY(proxysql_admin, "SET admin-stats_mysql_connections=10");
-
+	// Increase interval and wait for next round of inserts.
+	// distinct_var_ids_in_history should equal the # of records inserted.
+	// If the interval isn't updated, then there'd be double what's expected.
+	new_stats_interval_sec = 10;
+	MYSQL_QUERY(proxysql_admin, ("SET admin-stats_mysql_connections=" + to_string(new_stats_interval_sec)).c_str());
+	MYSQL_QUERY(proxysql_admin, "LOAD ADMIN VARIABLES TO RUNTIME");
+
 	sleep(new_stats_interval_sec + 1); // give it time to insert next round of stats
 
 	MYSQL_QUERY(proxysql_admin, "SELECT COUNT(*) FROM history_mysql_status_variables");
@@ -170,10 +188,11 @@ int main(int argc, char** argv) {
 	mysql_free_result(result);
 
 	ok(
-		history_rows_before < history_rows_after,
-		"Number of rows in history table increases after connections stat changes. Before: %lu After: %lu",
+		(history_rows_before + distinct_var_ids_in_history) == history_rows_after,
+		"Number of rows in history table increases correctly after insert interval change. Before: %lu After: %lu Difference should be: %lu",
 		history_rows_before,
-		history_rows_after
+		history_rows_after,
+		distinct_var_ids_in_history
 	);
 
 	return exit_status();