New observation for connection at creation time

[steve-chavez]

Problem

There are some problems with using ReadyForUseConnectionStatus for when a pool connection is created and for when is returned to the pool.

hasql-pool/src/library/exposed/Hasql/Pool.hs

Lines 167 to 173 in 8a9389f

	Connection.acquire settings >>= \case
	Left connErr -> do
	poolObserver (ConnectionObservation id (TerminatedConnectionStatus (NetworkErrorConnectionTerminationReason (fmap (Text.decodeUtf8With Text.lenientDecode) connErr))))
	atomically $ modifyTVar' poolCapacity succ
	return $ Left $ ConnectionUsageError connErr
	Right connection -> do
	poolObserver (ConnectionObservation id ReadyForUseConnectionStatus)

hasql-pool/src/library/exposed/Hasql/Pool.hs

Lines 207 to 212 in 8a9389f

	returnConn
	poolObserver (ConnectionObservation (entryId entry) ReadyForUseConnectionStatus)
	return $ Left $ SessionUsageError err
	Right (Right res) -> do
	returnConn
	poolObserver (ConnectionObservation (entryId entry) ReadyForUseConnectionStatus)

• It's not possible to create a counter for the total number of created connections. Since ReadyForUseConnectionStatus will happen many times for a pool connection.
• Not straightforward to create a gauge for the number of used connections. Ideally it would just increase on InUseConnectionStatus and decrease on ReadyForUseConnectionStatus, but since ReadyForUseConnectionStatus happens at creation time the gauge will always start at -1.

Proposal

How about a new observation when the pool connection is created, named as CreatedAndReadyForUseConnectionStatus?

[nikita-volkov]

Agreed. You've quickly found a spot in the design which I was not sure about.

The current design is based on a status model. The observation informs the user about the new status that a connection has entered. Such approach is not the lowest possible level of abstraction and does involve some interpretation on the library end. The use of ReadyForUseConnectionStatus for different state transitions is exactly a case of such interpretation. What I was trying to achieve with that approach was to provide for a more stable API. Evidently recognising the validity of your request here I've failed :)

A lower level of abstraction would be to emit events describing what happened without any interpretation on how it affected the status a connection. There would be a separate constructor for every observed event. In such case instead of a status model we would have had an event model. An we actually did have that at some point during the development of this feature, see fc48a10.

The way that I see it we now have two options on what to do next:

1. Extend the status model with the status that you recommend.

2. Consider the status model a failed experiment and switch to event-model.

Unfortunately both options would require a major version bump according to PVP. I'm inclining towards the second option, but please do provide your take on the matter. No need to restore the old code, just change the naming and claims of some current types.

[nikita-volkov]

I've just had an idea about a backward-compatible option. We can introduce a separate module with events and a separate configuration option for them. The current status mechanism can stay with the same API, but under the hood it will be reimplemented as an interpretation layer above the event-model.

The price for going this route will be more technical debt to maintain.

[steve-chavez]

Unfortunately both options would require a major version bump according to PVP. I'm inclining towards the second option, but please do provide your take on the matter.
The price for going this route will be more technical debt to maintain.

From my side I would be fine with a breaking change, I still haven't released PostgREST/postgrest#3420. Wouldn't want to cause maintenance burden.

[nikita-volkov]

I've decided to keep the status model for now to avoid jumping between designs and extended the ReadyForUse status. It's now released as 1.1.

Pleas do post back on whether this suffices. If not we can quickly do another release addressing that.

[steve-chavez]

Those seem enough to solve this issue. Many thanks!

Will close this for now and report back if there are any issues.