Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a "demuxed" status to proxysql_connection_pool_conn to track connections with multiplexing disabled #4474

Open
wants to merge 1 commit into
base: v2.x
Choose a base branch
from

Conversation

saunderst
Copy link
Contributor

This is a feature we've found highly useful on a daily basis in our dashboards and alerts. I've implemented it as an additional status label on proxysql_connection_pool_conn.

Question for reviewers: Would it be better as an entirely new metric?

Here's a simple illustration of the metric in use. First, we use GET_LOCK() to open a connection with multiplexing disabled:

proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="demuxed"} 1
proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="used"} 1
proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="free"} 0

Note that the demultiplexed connection is counted twice, as both used and demuxed.

Now execute a long-running query in one of the other client sessions. Now we have two used connections, but still just one demuxed.

proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="demuxed"} 1
proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="used"} 2
proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="free"} 0

Release the lock and free the de-multiplexed connection:

proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="demuxed"} 0
proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="used"} 1
proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="free"} 1

Finally, the other query completes and we have two free connections in the pool:

proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="demuxed"} 0
proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="used"} 0
proxysql_connpool_conns{endpoint="mysql-m1:3306",hostgroup="0",status="free"} 2

@renecannao
Copy link
Contributor

Hi @saunderst .
I think this PR introduces a potentially issue, and I have doubts it can be merged as-is.

mysrvc->ConnectionsUsed is an object that basically stores an array of MySQL_Connection * ( to be specific , PtrArray *conns).

When MySQL_HostGroups_Manager::p_update_connection_pool() is called, mysrvc->ConnectionsUsed cannot change (no connection can be added or removed) , therefore it is safe to just count them calling mysrvc->ConnectionsUsed->conns_length() .
But the new function in this PR (conns_with_multiplexing_disabled()) inspects every single connection in this array, calling MultiplexDisabled() .
MultiplexDisabled() checks internal values of the MySQL_Connection object: the problem is that dirty reads are possible, because the connection is currently actively used by one of the MySQL_Thread that owns it.

One could argue that the dirty read is not a serious problem, because in the worse case it can return a slightly incorrect result.
Furthermore, MultiplexDisabled() doesn't check for transactions or potential transactions.
You may look at the new internal status MultiplexDisabled_ext for further details : https://github.com/sysown/proxysql/blob/v2.6.0/lib/MySQL_Session.cpp#L1271-L1282

Finally, I am concerned about the performance cost of inspecting all used connections.

So my question is: maybe it is worth investigating a completely different approach?
For example...
Class MySrvC has queries_sent that count the number of queries sent to that specific server.
If a new counter (let's call it "counterX" for now) is added to count how many times a connection is returned to ConnectionsFree , we can easily compute how efficient is the connection pool for that specific server as a ratio counterX/queries_sent .
counterX can be exposed as a Prometheus metric, and you can use the ratio counterX/queries_sent for alerting.

It is worth to note that counterX would be a counter and not a gauge.

Finally, using this new counter doesn't provide the same information you are trying to achieve with this PR (how many connections have multiplexing disabled) , but provides a metric to determine how efficient is multiplexing.

Thoughts?

@saunderst
Copy link
Contributor Author

Hi @renecannao.

Thanks for the feedback. I had considered the dirty read problem, and decided as you said that a slightly incorrect value is acceptable, since we're just using the metric to spot major problems. I was also concerned about the performance impact, but decided it was probably okay for our numbers of backend connections per ProxySQL instance and how often we're hitting the Prometheus endpoint. However, maybe those assumptions can't be generalized across all ProxySQL users, and I am interested in exploring your alternative suggestion a bit more.

Would it be accurate to say that the counterX/queries_sent ratio as a signal of multiplexing efficiency depends on clients that disable multiplexing and then continue to send a lot of queries on the connection? In contrast, I'm thinking of clients that disable multiplexing then leave the connection idle for some period of time -- it seems that this type of multiplexing disabler might not skew the counterX/queries_sent ratio very much, even though they're preventing other clients from accessing the connection pool. I'd probably have to test it to determine exactly how this plays out in our case and whether we could still get a useful signal from the X/q ratio, but does my question make sense? If so, what might be a good way to address this possibility?

@renecannao
Copy link
Contributor

This PR was closed by mistake. Reopening

@renecannao renecannao reopened this Mar 31, 2024
@mirostauder
Copy link
Collaborator

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants