Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use SocketsHttpHandler to configure HttpClient in .NET 6+ #2931

Merged
merged 31 commits into from
Jan 7, 2025

Conversation

tippmar-nr
Copy link
Member

@tippmar-nr tippmar-nr commented Dec 18, 2024

  • Updated NRHttpClient to dynamically create an instance of SocketsHttpHandler when the app is running in .NET 6 or later, and configured PooledConnectionTimeout and PooledConnectionIdleTimeout to help the agent respond better to DNS changes. The connection timeout value is hard-coded to 5 minutes, which overrides the default of "never" and is probably sufficient for our needs. I didn't see a compelling reason to add a configuration setting for this value. Resolves Update HttpClient usage to better support DNS changes on .NET/.NET Core #1845.

Additional refactoring:

  • Update calls to HttpClientWrapper.SendAsync() to add a .ConfigureAwait(false) which seems to help prevent deadlocks.
  • Replace lock() usage with SemaphoreSlim in several places, also aimed at helping to prevent deadlocks.
  • Added .ConfigureAwait(false) in nearly all locations where we're currently doing a .GetAwaiter().GetResult() pattern. More deadlock prevention.
  • Updated all event aggregators to log the ... harvest finished message even if no events were sent. This will help in troubleshooting recently reported deadlocks when sending data.
  • Renamed IHttpClientFactory.CreateClient() to GetOrCreateClient() to better reflect what the method does.
  • Removed an unused property from IHttpClient and the classes that implement it
  • Refactored ConnectionHandler.SendDataOverWire<T>() to make exception handling a bit cleaner and more clear. Resolves Refactor nested try/catch in ConnectionHandler #2778
  • Cleaned up several log messages in exception handlers so that they only log the exception message instead of the full stack trace if the handler is just going to throw the exception again anyway.
  • Added logging to allow better tracing when sending data.
  • Added a couple usings in NRHttpClient for objects that are IDisposable (HttpRequestMessage, ByteArrayContent)

@tippmar-nr tippmar-nr marked this pull request as ready for review December 19, 2024 17:36
@tippmar-nr tippmar-nr requested a review from a team as a code owner December 19, 2024 17:36
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 86.84211% with 25 lines in your changes missing coverage. Please review.

Project coverage is 81.49%. Comparing base (63788ff) to head (2f67e2f).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...ic/Agent/Core/DataTransport/Client/NRHttpClient.cs 71.87% 7 Missing and 2 partials ⚠️
...elic/Agent/Core/DataTransport/ConnectionManager.cs 80.00% 7 Missing ⚠️
src/Agent/NewRelic/Agent/Core/AgentShim.cs 16.66% 5 Missing ⚠️
...elic/Agent/Core/DataTransport/ConnectionHandler.cs 88.23% 1 Missing and 1 partial ⚠️
...Relic/Agent/Core/Aggregators/LogEventAggregator.cs 93.33% 0 Missing and 1 partial ⚠️
...nt/Core/DataTransport/Client/HttpContentWrapper.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2931      +/-   ##
==========================================
+ Coverage   81.42%   81.49%   +0.06%     
==========================================
  Files         465      465              
  Lines       29563    29639      +76     
  Branches     3278     3285       +7     
==========================================
+ Hits        24073    24155      +82     
+ Misses       4697     4689       -8     
- Partials      793      795       +2     
Flag Coverage Δ
Agent 82.42% <86.84%> (+0.07%) ⬆️
Profiler 73.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ic/Agent/Core/Aggregators/CustomEventAggregator.cs 98.71% <100.00%> (+0.03%) ⬆️
...lic/Agent/Core/Aggregators/ErrorEventAggregator.cs 94.89% <100.00%> (+0.10%) ⬆️
...lic/Agent/Core/Aggregators/ErrorTraceAggregator.cs 90.12% <100.00%> (+0.25%) ⬆️
...ewRelic/Agent/Core/Aggregators/MetricAggregator.cs 97.18% <100.00%> (+0.16%) ⬆️
...elic/Agent/Core/Aggregators/SpanEventAggregator.cs 82.41% <100.00%> (+0.39%) ⬆️
...Relic/Agent/Core/Aggregators/SqlTraceAggregator.cs 100.00% <100.00%> (ø)
...ent/Core/Aggregators/TransactionEventAggregator.cs 99.00% <100.00%> (+0.02%) ⬆️
...ent/Core/Aggregators/TransactionTraceAggregator.cs 94.20% <100.00%> (+0.35%) ⬆️
.../Agent/Core/DataTransport/Client/HttpClientBase.cs 84.09% <100.00%> (ø)
...ent/Core/DataTransport/Client/HttpClientWrapper.cs 100.00% <ø> (+64.28%) ⬆️
... and 14 more

... and 2 files with indirect coverage changes

@tippmar-nr tippmar-nr merged commit eb3afda into main Jan 7, 2025
95 checks passed
@tippmar-nr tippmar-nr deleted the feature/sockets-http-handler branch January 7, 2025 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants