fix: implement connection tracking in metrics - #4672
Conversation
Fix should have an entry on the CHANGELOG |
0f475e3 to
592e5a3
Compare
1f3b4bc to
bce1f6e
Compare
7383b59 to
9b3c004
Compare
9b3c004 to
6af7b30
Compare
|
@mkleczek Since this is a |
27d9a1d to
e438eed
Compare
| SpecState{specAppState = appState, specMetrics = metrics, specObsChan} <- getState | ||
| let waitFor = waitForObs specObsChan | ||
|
|
||
| liftIO $ checkState' metrics [ | ||
| -- we expect in use connections to be the same once finished | ||
| inUseConnections (+ 0) | ||
| ] $ do | ||
| signal <- newEmptyMVar | ||
| -- make sure waiting thread is signaled | ||
| bracket_ (pure ()) (putMVar signal ()) $ | ||
| -- expecting one more connection in use | ||
| checkState' metrics [ | ||
| inUseConnections (+ 1) | ||
| ] $ do | ||
| -- start a thread hanging on a single connection until signaled | ||
| void $ forkIO $ void $ AppState.usePool appState $ liftIO (readMVar signal) | ||
| -- main thread waits for ConnectionObservation with InUseConnectionStatus | ||
| -- after which used connections count should be incremented | ||
| waitFor (1 * sec) "InUseConnectionStatus" $ \x -> [ o | o@(HasqlPoolObs (SQL.ConnectionObservation _ SQL.InUseConnectionStatus)) <- pure x] | ||
|
|
||
| -- hanging thread was signaled and should return the connection | ||
| waitFor (1 * sec) "ReadyForUseConnectionStatus" $ \x -> [ o | o@(HasqlPoolObs (SQL.ConnectionObservation _ (SQL.ReadyForUseConnectionStatus _))) <- pure x] |
There was a problem hiding this comment.
I'm looking at this test-case.. and it just feels like an imperative language like python is much better to represent such a step-by-step test.
I can't even make full sense of the test, but I believe pytest + toxiproxy + metrics endpoint should allow the same test, right?
There was a problem hiding this comment.
I'm looking at this test-case.. and it just feels like an imperative language like python is much better to represent such a step-by-step test.
I personally prefer statically typed language and many people find Haskell The Best Imperative Language
Especially in terms of concurrency support (and it is important in this test) Python is very far behind Haskell.
I can't even make full sense of the test, but I believe pytest + toxiproxy + metrics endpoint should allow the same test, right?
Not really (well... you could but it is not very convenient).
Toxiproxy is not used in this test so does not matter.
The idea of this test is to start a worker thread that will borrow a connection from the pool and keep it used until signaled by main thread.
After spawning the worker thread, main thread first waits for InUseConnectionStatus so that it is sure connection has been indeed borrowed from the pool. Then it checks it is properly accounted for in the metrics. Then it signals the worker thread, which in turn releases the connection and exits. Main thread then makes sure ReadyForUseConnectionStatus was published and then verifies connection release was accounted for in the metrics.
This is a "white box" test - we don't really test PostgREST as a whole here but only AppState.usePool function behavior.
There is no easy way to do the same in Python. We would need to somehow use the database advisory locks to be able to achieve "borrow connection from the pool and wait for a signal before releasing it". We also need to be able to wait for specific observations in the test. This was enabled for HSpec tests in #4671 and discussed with @steve-chavez and @taimoorzaeem
It is probably doable in Python (everything is doable in Turing complete language) but would require a lot of preparation and specific fixtures. It would also require accessing database directly from Python which we don't do in io tests.
There was a problem hiding this comment.
Toxiproxy is not used in this test so does not matter.
But we do need it to prove the fix for #4622 as done in #4855 right?
I can't even make full sense of the test
I also don't find this readable, if the test in #4855 is enough maybe the test here is not really necessary?
(Not arguing for python or haskell, but seeing if is possible to save us some lines of code)
There was a problem hiding this comment.
If the test in #4855 is enough maybe the test here is not really necessary?
IMHO this test is necessary regardless. The test in #4855 tests if poolAvailable metric does not become negative during adverse network conditions.
This test is about inUse count during AppState.usePool execution.
It is something different and I would say this test is needed to simply make sure we properly calculate connection counts.
There was a problem hiding this comment.
I personally prefer statically typed language and many people find Haskell The Best Imperative Language
From that link:
However, the false half is the syntax. I find Haskell pretty verbose and awkward to use in an imperative style.
Right on point for me.
Toxiproxyis not used in this test so does not matter.
One of the problems the io tests have is the big dependence on timing. Wait a second here, two there, whatever. Of course, we could write it with this approach in pytest as well - but I don't think that's a good idea.
So I thought of using toxiproxy to allow us to do this properly, i.e. by pausing the underlying connections, checking the state, then resuming / finishing the connection, checking the state again. But after reading the remainder of your comment about advisory locks, I see that we probably need these to even get to the right spot to pause the connection - and once we're there, we don't need toxiproxy for that specific case anymore.
There was a problem hiding this comment.
However, the false half is the syntax. I find Haskell pretty verbose and awkward to use in an imperative style.
Right on point for me.
It's the same for me but there's also another angle here. @mkleczek On #4672 (comment), you mention "support". I've had the experience of support + customers (i.e. users of PostgREST) challenging if a bug is fixed or not. Pointing them to a clear black box python test quickly settled the debate. It won't be the same in Haskell.
But after reading the remainder of your comment about advisory locks, I see that we probably need these to even get to the right spot to pause the connection - and once we're there, we don't need toxiproxy for that specific case anymore.
Can we use the advisory locks to achieve a negative connection metric?
There was a problem hiding this comment.
Not opposed to keep the current Haskell test. But perhaps we can add another one python io test that proves the Prometheus metrics are correct?
It would also require accessing database directly from Python which we don't do in io tests.
We do btw, a couple of choices:
There was a problem hiding this comment.
It's the same for me but there's also another angle here. @mkleczek On #4672 (comment), you mention "support". I've had the experience of support + customers (i.e. users of PostgREST) challenging if a bug is fixed or not. Pointing them to a clear black box python test quickly settled the debate. It won't be the same in Haskell.
From my experience they don't really care about automated tests you have in your codebase, what language they are written in and whether they instantiate a program as a "black box" or test the module containing the bug - the whole source code and its organization is a black box to them (and they are not programmers most of the time).
They do care that you declare the bug is fixed though, which seems needs to wait.
But after reading the remainder of your comment about advisory locks, I see that we probably need these to even get to the right spot to pause the connection - and once we're there, we don't need toxiproxy for that specific case anymore.
Can we use the advisory locks to achieve a negative connection metric?
No, we can't. This is about connection errors - ie. failing to connect to the database server.
I'm sorry guys - I've raised #4855 to have a reproducer - I don't have any other idea on how to reproduce it, so if it is not mergeable then it looks like we have to live with #4622 until someone implements a different reproducer (possibly using different testing infrastructure and language). I am happy to wait and rebase this PR once such a reproducer is merged into main.
@steve-chavez @wolfgangwalther - you are maintainers of this project and you decide on the overall direction and whether PRs are aligned with your vision. I personally find it discouraging, though, that these grand vision discussions as in #4868 are really just blocking improvements and bug fixing.
I personally prefer continuous improvement and implementing sometimes imperfect solutions to current problems instead of blocking them just because they are imperfect - but that's my personal view.
There was a problem hiding this comment.
I personally find it discouraging, though, that these grand vision discussions as in #4868 are really just blocking improvements and bug fixing.
Just to be clear: I specifically said this discussion is not blocking for me. What is blocking for Steve is essentially the backlog of #1766.
Of course, this won't help you in any way and I fully understand that this sucks. :/
90134d4 to
c196a70
Compare
|
@steve-chavez @wolfgangwalther - what do you think is required to push this forward? #4622 is being now reported by our support and it is becoming urgent to fix it. |
c389f35 to
202f84e
Compare
This does not introduce any new test infrastructure that I'd be opposed to, so I won't block on the long term vision of how our tests should be structured. It uses the existing infrastructure. I might not like the way the test is written, but I don't see a need to block on that either. To be clear, the question in #4672 (comment) was asked to get a feeling of how things could be done differently, if we had better test infrastructure elsewhere - and not to block this PR's progress. Imho the only previously blocking comment is #4672 (comment). Now, since I wrote that comment, I started a major discussion on how we should test in general, which is blocking the other, test-infra related, issues/PRs. I don't think we should hold this PR hostage to that either. TLDR: No blockers for me. |
It's gonna be really confusing when we look back in history and we say we fix #4622 and there isn't a precise test proving it (the current test does not). Let's not set a precedent here that can later hurt us with tech debt, so we should first clear the above thread. |
The thread you linked is about the currently existing test, so that doesn't quite match the first sentence about a missing test. If you're concerned about a missing test, then we need to clear #4672 (comment). I'd still say the situation now is different compared to when #1766 happened - we are actively working on improving the test situation and we have an open PR to track the addition of the herein-missing test. Thus, I'd say the risk of this getting forgotten is much smaller than earlier. I'd say we should go ahead with this. |
202f84e to
8e12a9a
Compare
|
@wolfgangwalther Let's not merge because I have a much simpler test almost ready for PR, let's merge this after that. |
|
No worries, I don't intend to merge. I just wanted to make my implicit approval explicit. I am well aware that you still have a thread open (this is now actually blocking the merge as well) - and I'm not just going to override you and resolve that thread. That's for you to decide :) |
8e12a9a to
bf18e9a
Compare
Right now metrics observation handler does not track database connections but updates a single Gauge based on HasqlPoolObs events. This is problematic because Hasql pool reports various connection events in multiple phases. The connection state machine is not simple and to precisely report the number of connections in various states, it is necessary to track their lifecycles. This change adds a ConnTrack data structure and logic to track database connections lifecycles. At the moment it supports "connected" and "inUse" connection counts precisely. The "pgrst_db_pool_available" metric is implemented on top of ConnTrack instead of a simple Gauge.
bf18e9a to
7785329
Compare
| ### Fixed | ||
|
|
||
| - Fix unnecessary connection pool flushes during schema cache reloading by @mkleczek in #4645 | ||
| - Fix race condition in pool_available metric causing negative values during network instability by @mkleczek in #4622 |
There was a problem hiding this comment.
@mkleczek This was added in an old version Fixed section https://github.com/PostgREST/postgrest/blob/main/CHANGELOG.md#fixed-2 😕
There was a problem hiding this comment.
Ehh... rebasing changelog is inherently tricky. My bad.
Raised #4942
Yup, sounds good. |
4 participants
DISCLAIMER:
This commit was authored entirely by a human without the assistance of LLMs.Right now metrics observation handler does not track database connections but updates a single Gauge based on HasqlPoolObs events.
This is problematic because Hasql pool reports various connection events in various states that make it impossible to predict the state change from the received event. The connection state machine is not simple and to precisely report the number of connections in various states, it is necessary to track their lifecycles.
Fixes #4622