Add connection pool get_connection_stats method

[yeraydiazdiaz]

Closes#18

[lovelydinosaur]

Okay, great!

So I'd been thinking about this one for a while actually. Something that we do want from this is to provide a useful level of structured debugging info. Something that we don't want is to introduce API that starts exposing internal implementation details.

I was wondering if perhaps we should shift to something like this...

{origin: [str, str, str, ...], origin: [...], ...}

Where the str is an under-defined string representation of the state of a connection.

Perhaps also get_connection_info() rather than get_connection_stats()?

Eg.

{
(b'https', b'example.org', 443): ["HTTP/2, 3 active streams"],
(b'https', b'tomchristie.com', 443): ["HTTP/1.1, idle keep-alive", "HTTP/1.1, active", "HTTP/1.1, active"],
(b'https', google.com', 443): ["Connecting"],
}

It's more verbose, but it doesn't start leaking implementation details, since we can document the strings as "opaque, don't rely on the format of this" but descriptive strings.

[yeraydiazdiaz]

I agree with leaking implementation details, which is why I added not the actual enum class but the name. Formalizing your suggestion strings a bit:

<HTTP_VERSION>, <STATE>, <ACTIVE_STREAMS_FOR_HTTP2>

I don't think keep_alive is an attribute of the connection though?

Also, do we want to keep the origin as a 3-tuple?

[yeraydiazdiaz]

Oh, and 👍 on get_connection_info.

[lovelydinosaur]

Also, do we want to keep the origin as a 3-tuple?

Erm, good point. Nope, let's have it as a plain string.

<HTTP_VERSION>, <STATE>, <ACTIVE_STREAMS_FOR_HTTP2>

Yup sounds fantastic. While we're connecting we don't yet have an HTTP version, so we've got a "Connecting" case too.

We probably want a .info() -> str method on HTTPConnection and HTTP2Connection, HTTP11Connection.

[lovelydinosaur]

So, something like this, then?...

{
"https://example.org:443": ["HTTP/2, ACTIVE, 3 streams"],
"https://tomchristie.com:443": ["HTTP/1.1, IDLE", "HTTP/1.1, ACTIVE", "HTTP/1.1, ACTIVE"],
"https://google.com:443": ["Connecting"],
}

[yeraydiazdiaz]

Yup, sounds good.

[cansarigol]

Nice improvement 👍

[florimondmanca]

LGTM

Are there any tests we can update to use this instead of private API?

Refs #18 (comment) :

Noticable in particular in the test cases which happen to reach inside intended-private API in order to deteremine the connection state

[yeraydiazdiaz]

Are there any tests we can update to use this instead of private API?

It did in the first iteration of this PR, but since we're using now a string summary I decided to not to assert on the result of the new method to make things clearer.

[yeraydiazdiaz]

Thanks for the reviews @florimondmanca and @cansarigol 🙏

[lovelydinosaur]

Fantastic stuff! Love it. 👍

It did in the first iteration of this PR, but since we're using now a string summary I decided to not to assert on the result of the new method to make things clearer.

Actually I do still think we should update the test cases. It's okay for us to document that the specific format shouldn't be relied upon, and could be subject to change, while still asserting against it ourselves in test cases.

I'd probably suggest...

• Tweak the docstring, to mention that the result should be treated as an opaque string, as the exact formatting and internal behaviours could be subject to change.
• Update our own test cases to assert against it, since we really do want to make sure we're tracking if and when those changes occur.