Mitigate CPU spikes when sending lots of events with lots of data

[antonpirker]

Increasing the HTTP pool size to better handle the requests. Also not reading the response saves some CPU cycles because TLS stuff uses CPU.

This does not fix all CPU spikes, but instead of spikes happening every 1 in 3-4 times it only happens 1 in 7-8 times with my test script.

FIxes #2384

[sl0thentr0py]

these are changes that are hard to reason about in terms of impact but they impact the entire SDK.

Can we instead

• expose num_pools as a config with default 2
• expose preload_content also as a new option and pass it through
• try these 2 in production for a while and then change the defaults if it works better

[sl0thentr0py]

cc @untitaker for a quick sanity check if you see problems with changing this stuff

[untitaker]

I think you want drain_conn instead of release_conn. If I understood the documentation correctly, you are putting a TCP connection back into the pool while the response body still has to be read.

I would guess that if you start sending new requests into that connection, the server may choose to terminate the connection because from their POV you're misbehaving. Even if that happens, urllib3 may transparently recover from that scenario by creating a new connection, but I am not sure. You'd have to test what actually happens. I suggest spinning up a http server locally and looking at wireshark (open it, look at loopback device lo0 and apply "http" filter)

I don't exactly understand what preload_conn does, but from what I can tell the original True value would've prevented this issue because it just behaves like "draining".

This assumes http1, i have no idea what happens with http2. I also just barely skimmed over the urllib3 docs for this, I have never heard of those specific parameters before, so take it with a grain of salt.

[antonpirker]

Hey! Thanks for the input. Both good ideas. Will make it configurable and also look at the traffic to see what is happening.
(the admin of the urllib3 discord server said I should just release_conn without reading the response, but better save than sorry)

[antonpirker]

Looking at wireshark was a good idea @untitaker. When I set preload_content/release_conn for every HTTP post a new TCP connection is established. This not what we want.

I will just revert the preload_content stuff and make the pool size configurable. Let's see if this helps.