fix: count connect timeouts, not statement timeouts, in connection_timeouts_total

docs/agents.md

@@ -213,8 +213,12 @@ engine.
 
 `instrument_pool_class(pool_class, metrics)` lives beside them and is the other half of the
 metrics wiring: it returns a subclass of `pool_class` that times `Pool.connect()`, which is
-the only place the wait for a connection and the pool checkout timeout are visible — no
-pool event fires for either. The manager applies it to whatever `resolve_pool_class`
+the only place the wait for a connection, the pool checkout timeout and a failed connect
+are visible — no pool event fires for any of them, and with asyncpg none reaches
+`handle_error`. A builtin `TimeoutError` there (asyncpg's connect `timeout`) counts as a
+timeout like the pool's own; any other exception is recorded as an error when `metrics`
+implements `ErrorRecorder`, unless it is the driver's DBAPI error, which `handle_error`
+reports. The manager applies it to whatever `resolve_pool_class`
 returned whenever `metrics` implements `CheckoutWaitRecorder`, so callers get it without
 asking. On an engine you build yourself, pass the result as `poolclass=` to
 `create_async_engine`; `attach_metrics` logs a warning if you did not, rather than leaving
@@ -350,12 +354,12 @@ variables only through the parent `BaseSettings` that holds it. Under
 | `postgres_db_connection_checkout_wait_seconds` | histogram | how long a caller **waited** for a connection — the queue wait, plus the pre-ping and the connect handshake when the pool had to grow. Buckets run to 30 s, the default `pool_timeout` |
 | `postgres_db_connection_held_duration_seconds` | histogram | how long a caller **held** one, checkout to checkin — the query time seen from the pool |
 | `postgres_db_connection_checkout_duration_seconds` | histogram | **deprecated**, and never measured what its name says: it is the held duration under its old name. Kept for one more minor release, then dropped. Move dashboards to `…_held_duration_seconds`, or to `…_checkout_wait_seconds` if what you wanted was the wait |
-| `postgres_db_connection_timeouts_total` | counter | pool checkout timeouts, plus `TimeoutError`s seen by `handle_error` |
-| `postgres_db_connection_errors_total{error_type}` | counter | database errors by exception class name |
+| `postgres_db_connection_timeouts_total` | counter | timeouts acquiring a connection: pool checkout timeouts and connect timeouts. A statement timeout (asyncpg `command_timeout`) is not one; it is `connection_errors_total{error_type="TimeoutError"}` |
+| `postgres_db_connection_errors_total{error_type}` | counter | database errors by exception class name: everything `handle_error` sees, plus a connection the pool could not open |
 
 A `prefix` is prepended with an underscore and must match `^[a-zA-Z_][a-zA-Z0-9_]*$`.
 
-The wait histogram and the pool-timeout half of the counter come from the instrumented pool
+The wait histogram, the timeout counter and the connect errors come from the instrumented pool
 class, not from a listener, so they move only on an engine built by `AsyncSessionManager`
 or one whose pool went through `instrument_pool_class`.
 
@@ -468,6 +472,7 @@ is what to do on the version the row names.
 | `create_async_session_manager(config)` through PgBouncer in transaction mode (0.2.1 and earlier) | `jit="off"` was the default and `db_schema` went as `search_path`, both as startup parameters: a default PgBouncer refuses every connection with `ProtocolViolationError: unsupported startup parameter: jit`; with `ignore_startup_parameters=jit,search_path` it connects and silently drops both, so every query lands in `public` | `jit=None`, `db_schema=None`, and `ALTER ROLE … SET search_path` on the server |
 | `postgres_db_connection_timeouts_total` (0.3.0 and earlier) | stayed at zero through every pool checkout timeout. The counter was fed only from the engine's `handle_error` listener, and a pool `TimeoutError` is raised by `pool.connect()` before any DBAPI call, so it never reaches that listener — the one timeout a pool actually produces under load was the one the counter did not count | count `sqlalchemy.exc.TimeoutError` around your own session calls |
 | `postgres_db_pool_overflow`, `postgres_db_pool_checked_out` (0.4.0 and earlier) | sampled from the `checkout` / `checkin` events: `overflow` exported `QueuePool.overflow()`, which starts at `-pool_size` (`-9` with one connection open of 10), and since `checkin` fires before the connection is back, an idle pool kept reporting its last burst as checked out | read `pool.checkedout()` and `max(pool.overflow(), 0)` yourself |
+| `postgres_db_connection_timeouts_total` (0.4.0) | counted asyncpg statement timeouts — a slow query, on a healthy connection — and missed connect timeouts, which never reach `handle_error`. A database refusing connections moved no series at all | alert on `connection_errors_total{error_type="TimeoutError"}` for slow statements, and on the checkout wait for connections |
 | `postgres_db_connection_checkout_duration_seconds` (0.3.0 and earlier) | the only checkout histogram there was, and it measures the time between the `checkout` and `checkin` events — how long a connection was *held*, which is query duration seen from the pool. The time a caller waited for a connection, which is what the name suggests and what predicts a pool outage, was not exposed at all | none; the wait was not measurable from outside the pool |
 
 `IsolationLevel` itself was always fine — `READ_UNCOMMITTED`, `READ_COMMITTED`,

docs/guide/advanced.md

@@ -275,7 +275,7 @@ session_manager = create_async_session_manager(
 | `myapp_postgres_db_connection_held_duration_seconds` | Histogram | Time a caller **held** a connection, checkout to checkin |
 | `myapp_postgres_db_connection_checkout_duration_seconds` | Histogram | Deprecated alias of the held duration — see below |
 | `myapp_postgres_db_connection_errors_total` | Counter | Database errors by type (`error_type` label) |
-| `myapp_postgres_db_connection_timeouts_total` | Counter | Pool checkout timeouts, and `TimeoutError`s during execution |
+| `myapp_postgres_db_connection_timeouts_total` | Counter | Timeouts acquiring a connection: pool checkout timeouts and connect timeouts. A statement timeout is an error, not one of these |
 
 Without a `prefix` the names are `postgres_db_pool_size` and so on.
 
@@ -300,9 +300,12 @@ keep working, and it will be removed in a future release. Point dashboards at
 `connection_held_duration_seconds` for the same numbers, or at
 `connection_checkout_wait_seconds` if what you actually wanted was the wait.
 
-The wait histogram and the pool-timeout half of the counter are not event listeners —
-SQLAlchemy fires no event when a checkout is *requested* — so they come from the pool class
-the session manager builds. They are populated on any engine created by
+The wait histogram and the timeout counter are not event listeners —
+SQLAlchemy fires no event when a checkout is *requested*, and neither a pool timeout nor a
+failed connect reaches `handle_error` — so they come from the pool class the session
+manager builds. The same pool class counts a connection it could not open (refused, bad
+password) in `connection_errors_total`. A statement that runs into asyncpg's
+`command_timeout` is counted as `connection_errors_total{error_type="TimeoutError"}`. They are populated on any engine created by
 `AsyncSessionManager`, `AsyncSessionManagerBuilder` or `create_async_session_manager`. On
 an engine you build yourself, wrap the pool class:
 

sqlalchemy_foundation_kit/contrib/metrics/postgres.py

@@ -67,7 +67,8 @@ class PostgresMetrics:
         - postgres_db_connection_held_duration_seconds: Time a connection was held by its caller.
         - postgres_db_connection_checkout_duration_seconds: Deprecated alias of the held duration,
           published unchanged for one more minor release.
-        - postgres_db_connection_timeouts_total: Number of connection checkout timeouts.
+        - postgres_db_connection_timeouts_total: Number of times acquiring a connection timed out:
+          pool checkout timeouts and connect timeouts. Statement timeouts are not counted here.
         - postgres_db_connection_errors_total: Number of connection errors.
 
     Labels:
@@ -122,7 +123,7 @@ def __init__(self, prefix: str | None = None) -> None:
         )
         self.connection_timeouts_total = Counter(
             _make_metric_name("postgres_db_connection_timeouts_total", prefix),
-            "Number of connection checkout timeouts",
+            "Number of times acquiring a connection timed out: pool checkout timeouts and connect timeouts",
         )
         self.connection_errors_total = Counter(
             _make_metric_name("postgres_db_connection_errors_total", prefix),
@@ -170,10 +171,10 @@ def record_checkout_wait(
         duration: float,
         timed_out: bool = False,
     ) -> None:
-        """Record the wait for a connection from the pool, and count a pool timeout.
+        """Record the wait for a connection from the pool, and count a timeout acquiring it.
 
-        A pool checkout timeout is raised before any DBAPI call, so it never reaches the
-        engine's ``handle_error`` listener; this is the only place it is counted.
+        A pool checkout timeout and a connect timeout never reach the engine's
+        ``handle_error`` listener; this is the only place either is counted.
         """
         self.connection_checkout_wait.observe(duration)
         if timed_out:
@@ -184,7 +185,11 @@ def record_error(
         error_type: str,
         is_timeout: bool = False,
     ) -> None:
-        """Record a database connection or execution error."""
+        """Record a database connection or execution error.
+
+        ``is_timeout`` is kept for compatibility and still counts a timeout when a custom
+        caller sets it; the library itself no longer does.
+        """
         self.connection_errors_total.labels(error_type=error_type).inc()
         if is_timeout:
             self.connection_timeouts_total.inc()

sqlalchemy_foundation_kit/protocols/metrics.py

@@ -102,22 +102,32 @@ def record_checkout_wait(self, duration: float, timed_out: bool = False) -> None
         Args:
             duration: Seconds spent inside ``pool.connect()`` — the queue wait, and the
                 pre-ping round trip and connect handshake when the pool had to grow.
-            timed_out: True if the wait ended in ``sqlalchemy.exc.TimeoutError`` rather
-                than in a connection. This is the pool checkout timeout; it is raised
-                before any DBAPI call and so never reaches the engine's ``handle_error``.
+            timed_out: True if the wait ended in a timeout rather than in a connection:
+                the pool checkout timeout (``sqlalchemy.exc.TimeoutError``) or the
+                driver's connect timeout (a builtin ``TimeoutError`` while the pool opens
+                a connection). Neither reaches the engine's ``handle_error``.
         """
         ...
 
 
+@runtime_checkable
 class ErrorRecorder(Protocol):
-    """Capability protocol for recording database errors."""
+    """Capability protocol for recording database errors.
+
+    Called for every error the engine's ``handle_error`` sees -- errors on an established
+    connection, a statement timeout among them -- and, from the instrumented pool, for a
+    connection the pool could not open. ``runtime_checkable`` so the instrumented pool can
+    tell whether its recorder takes errors too.
+    """
 
     def record_error(self, error_type: str, is_timeout: bool = False) -> None:
         """Record a database connection or execution error.
 
         Args:
             error_type: The type of error that occurred (e.g., "OperationalError").
-            is_timeout: True if this error was specifically a connection checkout timeout.
+            is_timeout: Kept for compatibility; the library no longer sets it. Timeouts
+                acquiring a connection are reported through
+                :meth:`CheckoutWaitRecorder.record_checkout_wait`.
         """
         ...
 

sqlalchemy_foundation_kit/session/manager.py

@@ -21,7 +21,7 @@
 
 from .._typing import SessionT
 from ..base import build_engine_kwargs, resolve_pool_class
-from ..protocols import CheckoutWaitRecorder, PoolStatsFunctionRecorder
+from ..protocols import CheckoutWaitRecorder, ErrorRecorder, PoolStatsFunctionRecorder
 
 if TYPE_CHECKING:
     from sqlalchemy.engine import Connection
@@ -75,7 +75,7 @@ def _pool_overflow(pool: Pool) -> int:
 
 
 def instrument_pool_class(pool_class: type, metrics: CheckoutWaitRecorder) -> type:
-    """Build a pool class that times the wait for a connection and counts pool timeouts.
+    """Build a pool class that times the wait for a connection and counts timeouts acquiring one.
 
     SQLAlchemy has no event for "a checkout was requested", only for one that succeeded, so
     the time a caller spends queued for a connection cannot be observed from a listener.
@@ -86,6 +86,15 @@ def instrument_pool_class(pool_class: type, metrics: CheckoutWaitRecorder) -> ty
     ``await_only`` inside that call, so the timer spans the whole suspension and measures
     the time the caller really waited.
 
+    Every checkout records its wait, however it ends. Two endings count as a timeout: the
+    pool's own ``sqlalchemy.exc.TimeoutError`` (the queue outlasted ``pool_timeout``) and
+    a builtin ``TimeoutError`` from the driver while the pool opens a new connection
+    (asyncpg's connect ``timeout``). Any other exception is a connection the pool could
+    not open, and is also recorded as an error when ``metrics`` implements
+    ``ErrorRecorder`` -- unless it is the driver's DBAPI error, which the engine's
+    ``handle_error`` reports itself. asyncpg's connect errors are not: none of them reaches
+    ``handle_error``, so this is the only place a database refusing connections shows up.
+
     ``_do_get`` is the wrong hook: ``QueuePool`` recurses into it when the non-blocking get
     comes back empty and the race for an overflow slot is lost, which is exactly the
     contended case worth measuring, and the wait would be observed twice.
@@ -106,6 +115,7 @@ def instrument_pool_class(pool_class: type, metrics: CheckoutWaitRecorder) -> ty
         >>> poolclass = instrument_pool_class(resolve_pool_class("async_adapted_queue"), metrics)
         >>> engine = create_async_engine(url, poolclass=poolclass)
     """
+    error_recorder = metrics if isinstance(metrics, ErrorRecorder) else None
 
     class InstrumentedPool(pool_class):  # type: ignore[misc, valid-type]
         """Pool that reports how long each checkout waited."""
@@ -114,7 +124,7 @@ def connect(self) -> Any:
             started = time.perf_counter()
             try:
                 connection = super().connect()
-            except SATimeoutError:
+            except (SATimeoutError, TimeoutError):
                 _safe_metric_call(
                     lambda: metrics.record_checkout_wait(
                         duration=time.perf_counter() - started,
@@ -123,12 +133,29 @@ def connect(self) -> Any:
                     "Failed to record database checkout timeout",
                 )
                 raise
+            except Exception as exc:
+                _safe_metric_call(
+                    lambda: metrics.record_checkout_wait(duration=time.perf_counter() - started),
+                    "Failed to record database checkout wait",
+                )
+                if error_recorder is not None and not self._reported_by_handle_error(exc):
+                    error_type = type(exc).__name__
+                    _safe_metric_call(
+                        lambda: error_recorder.record_error(error_type=error_type),
+                        "Failed to record database connect error",
+                    )
+                raise
             _safe_metric_call(
                 lambda: metrics.record_checkout_wait(duration=time.perf_counter() - started),
                 "Failed to record database checkout wait",
             )
             return connection
 
+        def _reported_by_handle_error(self, exc: Exception) -> bool:
+            """Whether the engine turns ``exc`` into a ``handle_error`` event itself."""
+            dbapi = getattr(self._dialect, "loaded_dbapi", None)
+            return dbapi is not None and isinstance(exc, dbapi.Error)
+
     setattr(InstrumentedPool, _WAIT_RECORDER_ATTRIBUTE, metrics)
     InstrumentedPool.__name__ = f"Instrumented{pool_class.__name__}"
     InstrumentedPool.__qualname__ = InstrumentedPool.__name__
@@ -208,11 +235,11 @@ def on_checkin(dbapi_connection: Any, connection_record: Any) -> None:
 
     def on_error(exception_context: Any) -> None:
         """Handle database error event."""
-        exc = exception_context.original_exception
-        error_type = type(exc).__name__
-        is_timeout = isinstance(exc, (TimeoutError, SATimeoutError))
+        # Only errors on an established connection get here -- a statement timeout among
+        # them. Timeouts acquiring a connection are counted by the instrumented pool.
+        error_type = type(exception_context.original_exception).__name__
         _safe_metric_call(
-            lambda: metrics.record_error(error_type=error_type, is_timeout=is_timeout),
+            lambda: metrics.record_error(error_type=error_type),
             "Failed to record database error metric",
         )