datalust / seq-forwarder

Local collection and reliable forwarding of log data to Seq
Apache License 2.0
57 stars 15 forks source link

Add PooledConnectionLifetime configuration option and configure HttpCllient with PooledConnectionLifetime value. #67

Closed williamb1024 closed 8 months ago

williamb1024 commented 9 months ago

Addition for #66

nblumhardt commented 9 months ago

Thanks for the PR! I think the use of SocketsHttpHandler will actually be a breaking change (no Windows proxy configuration support) given the forwarder currently targets netcore3.1, which will use WinHttpHandler on Windows by default. (Sockets* only became the default on .NET 5 or 6 IIRC.)

I'm not sure what the path from here should be - perhaps just a UseWinHttpHandler Boolean option that we default to false?

williamb1024 commented 9 months ago

For simplicity, I'll adjust the PooledConnectionLifetime option to be nullable. If the value is assigned, the code for the SocketHttpHandler will kick in; otherwise, the original default HttpClient handler will be used.

I believe requiring opt-in will resolve your concern and also eliminates my issue as well.

williamb1024 commented 9 months ago

I've updated the pull request with the changes that I outlined previously. I have not performed testing on these changes yet. If you are good with this approach, let me know and I'll verify that it operates as expected.

nblumhardt commented 9 months ago

This looks good to me 👍

The only minor thing I'd check out further is the type of the PooledConnectionLifetime property; it's a bit of a pain to construct .NET TimeSpans from JSON/environment configuration - they need to be written as strings like "1.23:45:67.890" etc.

Would it be possible to change this into uint PooledConnectionLifetimeMilliseconds or similar?

williamb1024 commented 9 months ago

Sure. I'll get the PR updated and tested over the next day or two.

williamb1024 commented 8 months ago

Adjusted as requested.

nblumhardt commented 8 months ago

Great, thanks! :+1: