Adds new statistics for tracking connections created and closed #202

pfreixes · 2024-06-05T21:12:09Z

We add several new statistics fields for tracking the following:

connections_created: How many connections have been created
connections_broken_closed: How many connections were closed due to a broken state
connections_invalid_closed: How many connections were closed due to be considered invalid.
connections_max_lifetime_closed: How many connections were closed due to reaching the max lifetime.
connections_max_idle_timeout_closed: How many connections were closed due to reaching the idle timeout.

We have added also a new statistic called get_waited_time_micro which provides the accumulated time in micros that all calls to the get method had to wait for a new connection.

We add several fiels for tracking the following: - connections_created: How many connections have been created - connections_broken_closed: How many connections were closed due to a broken state - connections_invalid_closed: How many connections were closed due to be considered invalid. - connections_max_lifetime_closed: How many connections were closed due to reaching the max lifetime. - connections_max_idle_timeout_closed: How many connections were closed due to reaching the idle timeout. We have added also a new statistic called `get_waited_time_micro` which provides the accumulated time in micros that all calls to the `get` method had to wait for a new connection.

codecov · 2024-06-05T21:14:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.15%. Comparing base (276235a) to head (58d5a7a).

❗ Current head 58d5a7a differs from pull request most recent head b321557

Please upload reports for the commit b321557 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
+ Coverage   76.05%   79.15%   +3.10%     
==========================================
  Files           6        6              
  Lines         543      595      +52     
==========================================
+ Hits          413      471      +58     
+ Misses        130      124       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

djc · 2024-06-05T21:14:37Z

bb8/src/inner.rs

+                        self.inner
+                            .statistics
+                            .connections_invalid_closed
+                            .fetch_add(1, Ordering::SeqCst);


We can just make this another StatsKind, right?

jaja, good point. Ill try to do that!

djc · 2024-06-05T21:15:01Z

bb8/src/inner.rs

+            self.inner
+                .statistics
+                .get_waited_time_micro
+                .fetch_add(wait_time.as_micros() as u64, Ordering::SeqCst);


Why micros vs millis or nanos?

I guess that micros would provide enough detail (nanos might be just noise) while we would keep the also the u64 large enough for storing many many many micros.

In any case since we move this to a Duration we will be storing behind the scenes IIRC micros using u128.

Oks now I remembered, everything I wanted to use an Atomic Type. I guess that we can try to use an u128 internally and return a Duration (which IIRC is just a u128). If this works we could just forget about the discussion between nanos and micros.

Finally I went for using micros with u64 for converting later to Duration. If you want to use nanos, i can do that. But got the feeling that nanosecond resolution for this specific use case would not provide a lot of value (indeed micro most likely will not neither)

Ive just kept the type to u64 just for having all atomics using the same type and helping me to keep the record interface simple.

If you want to to go for nanos and/or u128 let me know.

djc · 2024-06-05T21:15:44Z

bb8/src/internals.rs

@@ -139,22 +147,34 @@ where
        ApprovalIter { num: num as usize }
    }

-    pub(crate) fn reap(&mut self, config: &Builder<M>) -> ApprovalIter {
+    pub(crate) fn reap(&mut self, config: &Builder<M>) -> (ApprovalIter, u64, u64) {
+        let mut max_lifetime_closed: u64 = 0;


Please avoid type annotations where possible.

djc · 2024-06-05T21:16:59Z

bb8/src/internals.rs

+    pub(crate) connections_created: AtomicU64,
+    pub(crate) connections_broken_closed: AtomicU64,
+    pub(crate) connections_invalid_closed: AtomicU64,
+    pub(crate) connections_max_lifetime_closed: AtomicU64,
+    pub(crate) connections_max_idle_timeout_closed: AtomicU64,


Let's skip the connections_ prefixes.

For all of them? at least at first sight the connections_created, connections_broken_closed, connections_broken_closed and connections_invalid_closed without the connections_ prefix will seem a bit off.

Also it follows the same practice for the get_ Kind, where we prefixed it and IMO helps to the user to understand the scope of them, same would happen for connections_

Okay, let's keep connections_ but I'd like closed moved directly after connections_ and rename max_idle_timeout to idle_timeout.

djc · 2024-06-05T21:17:41Z

bb8/src/internals.rs

 }

 impl AtomicStatistics {
-    pub(crate) fn record(&self, kind: StatsKind) {
+    pub(crate) fn record_get(&self, kind: StatsKind) {


Given that we can use it for incrementing other (non-get) counters, prefer to revert this.

djc · 2024-06-05T21:18:20Z

bb8/src/api.rs

@@ -99,6 +99,20 @@ pub struct Statistics {
    pub get_waited: u64,
    /// Total gets performed that timed out while waiting for a connection.
    pub get_timed_out: u64,
+    /// Total time accumulated waiting for a connection.
+    pub get_waited_time_micro: u64,


Let's make this a std::time::Duration, and lose the _micro suffix.

pfreixes · 2024-06-07T14:36:22Z

@djc this one is ready for a second review.

djc

Okay, I want a clean commit history with separate logical changes, so this should end up being in a different commit. I usually handle this by using interactive rebases but feel free to use separate PRs if that's simpler for you. I think we need the following changes:

Rename StatsKind to StatsGet
Rename record() to record_get()
Track wait_time: add wait_time variable in get() and change record_get() to accept the Option<Duration>, should end up in get_wait_time_micros in AtomicStatistics and in get_wait_time in Statistics
Track closed_broken and closed_created: recreate AtomicStatistics::record() and StatsKind for these
Track closed_max_lifetime and closed_idle_timeout: create a separate method AtomicStatistics::reaped() for these

djc · 2024-06-10T08:06:30Z

bb8/src/inner.rs

@@ -85,7 +85,8 @@ where
    }

    pub(crate) async fn get(&self) -> Result<PooledConnection<'_, M>, RunError<M::Error>> {
-        let mut kind = StatsKind::Direct;
+        let mut get_kind = StatsKind::GetDirect;


Don't rename this get_kind, please.

djc · 2024-06-10T08:07:56Z

bb8/src/inner.rs

+        self.inner.statistics.record(get_kind, 1);
+
+        if let Some(wait_time_start) = wait_time_start {
+            let wait_time = Instant::now() - wait_time_start;
+            self.inner
+                .statistics
+                .record(StatsKind::GetWaitedTime, wait_time.as_micros() as u64);
+        }


Let's wrap this up in a method on AtomicStatistics called record_get().

djc · 2024-06-10T08:12:20Z

bb8/src/internals.rs

+        self.statistics
+            .record(StatsKind::ConnectionsMaxLifeTimeClosed, max_lifetime_closed);
+        self.statistics.record(
+            StatsKind::ConnectionsMaxIdleTimeoutClosed,
+            max_idle_timeout_closed,
+        );


Let's wrap this in a separate method on AtomicStatistics, too, and keep all of the reaping-related changes in a separate commit or PR.

djc · 2024-06-10T08:12:55Z

bb8/src/internals.rs

+        let mut max_lifetime_closed = 0;
+        let mut max_idle_timeout_closed = 0;


Let's call these closed_max_lifetime and closed_idle_timeout.

djc · 2024-06-10T08:13:54Z

bb8/src/internals.rs

+    pub(crate) connections_created: AtomicU64,
+    pub(crate) connections_broken_closed: AtomicU64,
+    pub(crate) connections_invalid_closed: AtomicU64,
+    pub(crate) connections_max_lifetime_closed: AtomicU64,
+    pub(crate) connections_max_idle_timeout_closed: AtomicU64,


Okay, let's keep connections_ but I'd like closed moved directly after connections_ and rename max_idle_timeout to idle_timeout.

djc · 2024-06-10T08:16:15Z

bb8/src/internals.rs

-    Direct,
-    Waited,
-    TimedOut,
+    GetDirect,


I think we'll want to split this into StatsGet { Waited, Direct, TimedOut }.

@djc just for having an agreement on the new kinds and record operations that we would have within AtomicStatistics, we can go for this

pub(crate) enum StatsGet { Waited, Direct, TimedOut, } pub(crate) enum StatsConnectionClosed { Broken, Invalid, }

Note: we should not need more stats kind since the used record_* methods for stats like the ones coming from the reap function will just call an ad-hoc function like record_connections_reaped

And then we would have the following record_* methods within the AtomicStatistics:

record_get (takes the StatsGet parameter)

record_connection_closed (takes the StatsConnectionClosed parameter)

record_connections_reaped

record_get_wait_time

record_connection_created

Let me know if this works for you and i can create a new PR with separated commits for each group, starting with the rename of the record to record_get

No, as suggested in my previous message we should reuse a simple record() with StatsKind for both connection_closed and connection_created, and record_get() should also handle get_wait_time.

Oks, let me provide a new draft - sorry for the back and forth here :

pub(crate) enum StatsGet { Waited, Direct, TimedOut, WaitTime } pub(crate) enum StatsConnectionClosed { Broken, Invalid, }

And following record methods

record which would take a StatsConnectionClosed kind type

record_get for recording any stats related to get including wait time

record_connections_reaped for recording the reaped connections either idle timeout or max lifetime

How does it sound? is this aligned with your expectations?

No. Let's do one PR at a time, please implement the first chunk and I'll give feedback. Please closely read my feedback from this PR because you've still missed some of it.

pfreixes changed the title ~~Adds new statistics for tracking connectoins created and closed~~ Adds new statistics for tracking connections created and closed Jun 5, 2024

djc reviewed Jun 5, 2024

View reviewed changes

Apply feedback

b321557

djc reviewed Jun 10, 2024

View reviewed changes

djc closed this Jun 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds new statistics for tracking connections created and closed #202

Adds new statistics for tracking connections created and closed #202

pfreixes commented Jun 5, 2024

codecov bot commented Jun 5, 2024 •

edited

Loading

djc Jun 5, 2024

pfreixes Jun 7, 2024

djc Jun 5, 2024

pfreixes Jun 7, 2024

pfreixes Jun 7, 2024

pfreixes Jun 7, 2024

djc Jun 5, 2024

djc Jun 5, 2024

pfreixes Jun 7, 2024

djc Jun 10, 2024

djc Jun 5, 2024

djc Jun 5, 2024

pfreixes commented Jun 7, 2024

djc left a comment

djc Jun 10, 2024

djc Jun 10, 2024

djc Jun 10, 2024

djc Jun 10, 2024

djc Jun 10, 2024

djc Jun 10, 2024

pfreixes Jun 10, 2024

djc Jun 10, 2024

pfreixes Jun 10, 2024

djc Jun 10, 2024

		let mut max_lifetime_closed = 0;
		let mut max_idle_timeout_closed = 0;

Adds new statistics for tracking connections created and closed #202

Adds new statistics for tracking connections created and closed #202

Conversation

pfreixes commented Jun 5, 2024

codecov bot commented Jun 5, 2024 • edited Loading

Codecov Report

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

pfreixes commented Jun 7, 2024

djc left a 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

codecov bot commented Jun 5, 2024 •

edited

Loading