Closes #3310: Improve error reporting for 'MySQL_Monitor' and 'MySQL_Session' backend connections via counters #3311

JavierJF · 2021-02-17T15:17:37Z

Closes #3310.

…Session' backend connections via counters

…llectable'

renecannao · 2021-02-17T15:32:45Z

lib/MySQL_Monitor.cpp

@@ -872,10 +874,12 @@ void * monitor_connect_thread(void *arg) {
 			(strncmp(mmsd->mysql_error_msg,"ProxySQL Error: Access denied for user",strlen("ProxySQL Error: Access denied for user"))==0)
 		) {
 			proxy_error("Server %s:%d is returning \"Access denied\" for monitoring user\n", mmsd->hostname, mmsd->port);
+			MyHGM->p_update_mysql_error_counter(p_mysql_error_type::proxysql, mmsd->hostgroup_id, mmsd->hostname, mmsd->port, ER_ACCESS_DENIED_ERROR);


Why are we hardcoding ER_ACCESS_DENIED_ERROR instead of using mysql_errno(mmsd->mysql) ?

Check for logging error is being performed on mmsd->mysql_error_msg, I assumed that if mysql_errno(mmsd->mysql) was guaranteed to be ER_ACCESS_DENIED_ERROR this check would much more simpler.

Because those are the only messages we want to write in the error log.
I don't see a valid reason to hardcode an error code, even if we know already what is.

renecannao · 2021-02-17T15:33:29Z

lib/MySQL_Monitor.cpp

 		}
 		else if (strncmp(mmsd->mysql_error_msg,"Your password has expired.",strlen("Your password has expired."))==0)
 		{
 			proxy_error("Server %s:%d is returning \"Your password has expired.\" for monitoring user\n", mmsd->hostname, mmsd->port);
+			MyHGM->p_update_mysql_error_counter(p_mysql_error_type::proxysql, mmsd->hostgroup_id, mmsd->hostname, mmsd->port, ER_MUST_CHANGE_PASSWORD_LOGIN);


Why are we hardcoding ER_MUST_CHANGE_PASSWORD_LOGIN instead of using mysql_errno(mmsd->mysql) ?

Same reason as in comment #3311 (comment)

renecannao · 2021-02-17T15:37:21Z

lib/MySQL_Monitor.cpp

@@ -1138,6 +1145,7 @@ void * monitor_read_only_thread(void *arg) {
 			free(mmsd->mysql_error_msg);
 			mmsd->mysql_error_msg = new_error;
 			proxy_error("Timeout on read_only check for %s:%d after %lldms. Unable to create a connection. If the server is overload, increase mysql-monitor_connect_timeout. Error: %s.\n", mmsd->hostname, mmsd->port, (now-mmsd->t1)/1000, new_error);
+			MyHGM->p_update_mysql_error_counter(p_mysql_error_type::proxysql, mmsd->hostgroup_id, mmsd->hostname, mmsd->port, ER_PROXYSQL_READ_ONLY_CHECK_CONN_TIMEOUT);


If a connection is not created (for example, because of a timeout) the counter is already increased in create_new_connection().
This line can go away

Counter in create_new_connection is incrementing p_mysql_error_type::mysql counter, this is incrementing p_mysql_error_type::proxysql counter, the first one reflects the error in the backend connection, this one the internal ProxySQL error generated because it was a READ_ONLY_CHECK_CONN_TIMEOUT. This second one offers information missing in the first one.

Why not using ER_PROXYSQL_READ_ONLY_CHECK_TIMEOUT ?
Too many variables seems an over-engineering, especially because it means creating a double number of time series. A timeout happened, this is all we need to know.
Same comment applies to all the others.

renecannao · 2021-02-17T15:37:55Z

lib/MySQL_Monitor.cpp

@@ -1192,6 +1201,7 @@ void * monitor_read_only_thread(void *arg) {
 		if (now > mmsd->t1 + mysql_thread___monitor_read_only_timeout * 1000) {
 			mmsd->mysql_error_msg=strdup("timeout check");
 			proxy_error("Timeout on read_only check for %s:%d after %lldms. If the server is overload, increase mysql-monitor_read_only_timeout.\n", mmsd->hostname, mmsd->port, (now-mmsd->t1)/1000);
+			MyHGM->p_update_mysql_error_counter(p_mysql_error_type::proxysql, mmsd->hostgroup_id, mmsd->hostname, mmsd->port, ER_PROXYSQL_READ_ONLY_CHECK_TIMEOUT);


If a connection is not created (for example, because of a timeout) the counter is already increased in create_new_connection().
This line can go away

Same reasoning as for #3311 (comment)

renecannao · 2021-02-17T15:40:16Z

lib/MySQL_Monitor.cpp

@@ -1297,20 +1307,23 @@ VALGRIND_ENABLE_ERROR_REPORTING;
 					if (resultset->rows_count) {
 						// disable host
 						proxy_error("Server %s:%d missed %d read_only checks. Assuming read_only=1\n", mmsd->hostname, mmsd->port, max_failures);
+						MyHGM->p_update_mysql_error_counter(p_mysql_error_type::proxysql, mmsd->hostgroup_id, mmsd->hostname, mmsd->port, ER_PROXYSQL_ASSUME_SRV_READ_ONLY);


This itself it is not an error.
We can have a counter for when ProxySQL assumes that read_only=1 , but this is not an error.
The error is when the check fails, not when as action is performed.
This line must go away.

This isn't tracked as a MySQL error, but as an ProxySQL generated error p_mysql_error_type::proxysql, the line is outdated, new code is ER_PROXYSQL_READ_ONLY_CHECKS_MISSED

renecannao · 2021-02-17T16:44:38Z

lib/MySQL_Session.cpp

@@ -2851,7 +2848,6 @@ bool MySQL_Session::handler_again___status_CHANGING_AUTOCOMMIT(int *_rc) {
 		myds->DSS = STATE_MARIADB_GENERIC;
 		NEXT_IMMEDIATE_NEW(st);
 	} else {
-		MyHGM->p_update_mysql_error_counter(p_mysql_error_type::mysql, myconn->parent->myhgc->hid, myconn->parent->address, myconn->parent->port, mysql_errno(myconn->mysql));


Instead of replacing this with 3 calls, we can use this:

MyHGM->p_update_mysql_error_counter(p_mysql_error_type::mysql, myconn->parent->myhgc->hid, myconn->parent->address, myconn->parent->port, ( mysql_errno(myconn->mysql) ? mysql_errno(myconn->mysql) : ER_PROXYSQL_OFFLINE_SRV ) );

renecannao · 2021-02-17T16:44:51Z

lib/MySQL_Session.cpp

 				} else {
 					proxy_error(
 						"Detected a broken connection during SET AUTOCOMMIT on %s , %d : %d, %s\n",
 						myconn->parent->address,
 						myconn->parent->port,
-						myerr,
+						ER_PROXYSQL_OFFLINE_SRV,


This can stay myerr .
For the same replace didn't happen in othe functions, like handler_again___status_SETTING_INIT_CONNECT

renecannao · 2021-02-17T16:54:17Z

include/MySQL_Monitor.hpp

+ * Enum holding the different connection errors that could be reached for
+ * the backend connections oppened by 'MySQL_Monitor' module.
+ */
+enum PROXYSQL_MONITOR_ERR {


Any reason why we have PROXYSQL_MONITOR_ERR and not just extend PROXYSQL_MYSQL_ERR ?

I thought it could be handy to have a clear separation between general errors and monitoring errors, easier to remember error code ranges that particular values.

I think it makes the maintenance more difficult.
Especially in cases in which they overlap.
For example, a ping can timeout no matter if in MySQL Session or on Monitor.
On the same topic, I think I don't see a counter for ping timeout in Monitor

renecannao · 2021-02-17T16:55:35Z

include/MySQL_Monitor.hpp

+ * the backend connections oppened by 'MySQL_Monitor' module.
+ */
+enum PROXYSQL_MONITOR_ERR {
+	ER_PROXYSQL_ASSUME_SRV_READ_ONLY = 10000,


From my other comments, I think 7 enums should go away, and 5 should stay

renecannao · 2021-02-17T16:57:00Z

include/MySQL_Protocol.h

+ * Enum holding the different MySQL connection errors that are used to report
+ * invalid states in the backend connections.
+ */
+enum PROXYSQL_MYSQL_ERR {


While here, we could also have added error codes 9001 to 9003

renecannao · 2021-02-17T18:59:16Z

include/MySQL_Monitor.hpp

+ * Enum holding the different connection errors that could be reached for
+ * the backend connections oppened by 'MySQL_Monitor' module.
+ */
+enum PROXYSQL_MONITOR_ERR {


I think it makes the maintenance more difficult.
Especially in cases in which they overlap.
For example, a ping can timeout no matter if in MySQL Session or on Monitor.
On the same topic, I think I don't see a counter for ping timeout in Monitor

renecannao · 2021-02-17T19:03:23Z

lib/MySQL_Monitor.cpp

@@ -872,10 +874,12 @@ void * monitor_connect_thread(void *arg) {
 			(strncmp(mmsd->mysql_error_msg,"ProxySQL Error: Access denied for user",strlen("ProxySQL Error: Access denied for user"))==0)
 		) {
 			proxy_error("Server %s:%d is returning \"Access denied\" for monitoring user\n", mmsd->hostname, mmsd->port);
+			MyHGM->p_update_mysql_error_counter(p_mysql_error_type::proxysql, mmsd->hostgroup_id, mmsd->hostname, mmsd->port, ER_ACCESS_DENIED_ERROR);


Because those are the only messages we want to write in the error log.
I don't see a valid reason to hardcode an error code, even if we know already what is.

renecannao · 2021-02-17T19:03:32Z

lib/MySQL_Monitor.cpp

 		}
 		else if (strncmp(mmsd->mysql_error_msg,"Your password has expired.",strlen("Your password has expired."))==0)
 		{
 			proxy_error("Server %s:%d is returning \"Your password has expired.\" for monitoring user\n", mmsd->hostname, mmsd->port);
+			MyHGM->p_update_mysql_error_counter(p_mysql_error_type::proxysql, mmsd->hostgroup_id, mmsd->hostname, mmsd->port, ER_MUST_CHANGE_PASSWORD_LOGIN);


renecannao · 2021-02-17T19:10:27Z

lib/MySQL_Monitor.cpp

@@ -1138,6 +1145,7 @@ void * monitor_read_only_thread(void *arg) {
 			free(mmsd->mysql_error_msg);
 			mmsd->mysql_error_msg = new_error;
 			proxy_error("Timeout on read_only check for %s:%d after %lldms. Unable to create a connection. If the server is overload, increase mysql-monitor_connect_timeout. Error: %s.\n", mmsd->hostname, mmsd->port, (now-mmsd->t1)/1000, new_error);
+			MyHGM->p_update_mysql_error_counter(p_mysql_error_type::proxysql, mmsd->hostgroup_id, mmsd->hostname, mmsd->port, ER_PROXYSQL_READ_ONLY_CHECK_CONN_TIMEOUT);


Why not using ER_PROXYSQL_READ_ONLY_CHECK_TIMEOUT ?
Too many variables seems an over-engineering, especially because it means creating a double number of time series. A timeout happened, this is all we need to know.
Same comment applies to all the others.

…_MYSQL_ERR' holding all ProxySQL errors for backend connections

…tor and replaced zero error codes with ER_PROXYSQL_OFFLINE_SRV

…ect_thread' and no longer required include

…ssion_status_flags-t' to avoid replication lag issues in the CI

JavierJF added 2 commits February 17, 2021 16:12

Closes #3310: Improve error reporting for 'MySQL_Monitor' and 'MySQL_…

851339b

…Session' backend connections via counters

Added missed default initialization of flag 'registered_prometheus_co…

9bffef4

…llectable'

renecannao requested changes Feb 17, 2021

View reviewed changes

Improved naming on new 'PROXYSQL_ERR' enums and explicit values

9713abd

renecannao requested changes Feb 17, 2021

View reviewed changes

JavierJF added 4 commits February 18, 2021 14:48

Removed 'PROXYSQL_MONITOR_ERR' enum in favor of single enum 'PROXYSQL…

84e205d

…_MYSQL_ERR' holding all ProxySQL errors for backend connections

Compressed multiple 'p_update_mysql_error_counter' with ternary opera…

7c1c829

…tor and replaced zero error codes with ER_PROXYSQL_OFFLINE_SRV

Removed hardcoded values for error codes in counters in 'monitor_conn…

6587049

…ect_thread' and no longer required include

Added extra wait after setting 'max_replication_lag' in test 'test_se…

1deee92

…ssion_status_flags-t' to avoid replication lag issues in the CI

renecannao merged commit 1dd24cc into v2.1.1 Mar 2, 2021

renecannao deleted the v2.1.1-3310 branch June 4, 2021 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #3310: Improve error reporting for 'MySQL_Monitor' and 'MySQL_Session' backend connections via counters #3311

Closes #3310: Improve error reporting for 'MySQL_Monitor' and 'MySQL_Session' backend connections via counters #3311

JavierJF commented Feb 17, 2021

renecannao Feb 17, 2021

JavierJF Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

JavierJF Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

JavierJF Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

JavierJF Feb 17, 2021

renecannao Feb 17, 2021

JavierJF Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

JavierJF Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

renecannao Feb 17, 2021

Closes #3310: Improve error reporting for 'MySQL_Monitor' and 'MySQL_Session' backend connections via counters #3311

Closes #3310: Improve error reporting for 'MySQL_Monitor' and 'MySQL_Session' backend connections via counters #3311

Conversation

JavierJF commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment