From 6baadd4e7ee84c3f3639e1536d4bc0438ccd1a48 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Wed, 18 Dec 2024 11:51:03 -0600 Subject: [PATCH 01/27] feat: Use `SocketsHttpHandler` to configure `HttpClient` in .NET 6+ --- .../Core/Aggregators/CustomEventAggregator.cs | 12 +++-- .../Core/Aggregators/ErrorEventAggregator.cs | 13 ++--- .../Core/Aggregators/ErrorTraceAggregator.cs | 14 +++--- .../Core/Aggregators/LogEventAggregator.cs | 33 ++++++------ .../Core/Aggregators/MetricAggregator.cs | 13 +++-- .../Core/Aggregators/SpanEventAggregator.cs | 16 +++--- .../Core/Aggregators/SqlTraceAggregator.cs | 15 +++--- .../Aggregators/TransactionEventAggregator.cs | 12 +++-- .../Aggregators/TransactionTraceAggregator.cs | 14 ++++-- .../DataTransport/Client/HttpClientWrapper.cs | 17 +------ .../Client/Interfaces/IHttpClientFactory.cs | 4 +- .../Client/Interfaces/IHttpClientWrapper.cs | 2 - .../Core/DataTransport/Client/NRHttpClient.cs | 50 +++++++++++++++++-- .../Client/NRHttpClientFactory.cs | 5 +- .../Client/WebRequestHttpClientFactory.cs | 4 +- .../Core/DataTransport/ConnectionHandler.cs | 42 ++++++++-------- .../Core/DataTransport/ConnectionManager.cs | 4 +- .../DataTransport/DataTransportService.cs | 13 ++++- .../Core/DataTransport/HttpCollectorWire.cs | 2 +- .../Client/NRHttpClientFactoryTests.cs | 20 ++++---- .../DataTransport/HttpCollectorWireTests.cs | 16 +++--- 21 files changed, 186 insertions(+), 135 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/CustomEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/CustomEventAggregator.cs index b974b6a017..1044d3c295 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/CustomEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/CustomEventAggregator.cs @@ -89,14 +89,16 @@ protected void InternalHarvest(string transactionId = null) var customEvents = originalCustomEvents.Where(node => node != null).Select(node => node.Data).ToList(); // if we don't have any events to publish then don't - if (customEvents.Count <= 0) - return; + var eventCount = customEvents.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(customEvents, transactionId); - var responseStatus = DataTransportService.Send(customEvents, transactionId); + HandleResponse(responseStatus, customEvents); + } - HandleResponse(responseStatus, customEvents); + Log.Finest($"Custom Event harvest finished. {eventCount} event(s) sent."); - Log.Finest("Custom Event harvest finished."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorEventAggregator.cs index 3a5d6de467..e4500a98d6 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorEventAggregator.cs @@ -99,14 +99,15 @@ protected void InternalHarvest(string transactionId = null) var eventHarvestData = new EventHarvestData(originalErrorEvents.Size, originalErrorEvents.GetAddAttemptsCount()); // if we don't have any events to publish then don't - if (aggregatedEvents.Count <= 0) - return; - - var responseStatus = DataTransportService.Send(eventHarvestData, aggregatedEvents, transactionId); + var eventCount = aggregatedEvents.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(eventHarvestData, aggregatedEvents, transactionId); - HandleResponse(responseStatus, aggregatedEvents); + HandleResponse(responseStatus, aggregatedEvents); + } - Log.Finest("Error Event harvest finished."); + Log.Finest($"Error Event harvest finished. {eventCount} event(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs index ef7c49ae9b..7448a9e9e6 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs @@ -79,14 +79,16 @@ protected void InternalHarvest(string transactionId = null) _readerWriterLock.ExitWriteLock(); } - if (errorTraceWireModels.Count <= 0) - return; - - var responseStatus = DataTransportService.Send(errorTraceWireModels, transactionId); + // if we don't have any events to publish then don't + var eventCount = errorTraceWireModels.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(errorTraceWireModels, transactionId); - HandleResponse(responseStatus, errorTraceWireModels); + HandleResponse(responseStatus, errorTraceWireModels); + } - Log.Finest("Error Trace harvest finished."); + Log.Finest($"Error Trace harvest finished. {eventCount} event(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/LogEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/LogEventAggregator.cs index 6ec943ebc6..01520d6cea 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/LogEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/LogEventAggregator.cs @@ -88,28 +88,27 @@ protected void InternalHarvest(string transactionId = null) Interlocked.Add(ref _logsDroppedCount, originalLogEvents.GetAndResetDroppedItemCount()); // if we don't have any events to publish then don't - if (aggregatedEvents.Count <= 0) + var eventCount = aggregatedEvents.Count; + if (eventCount > 0) { - return; - } + // matches metadata so that utilization and this match + var hostname = !string.IsNullOrEmpty(_configuration.UtilizationFullHostName) + ? _configuration.UtilizationFullHostName + : _configuration.UtilizationHostName; - // matches metadata so that utilization and this match - var hostname = !string.IsNullOrEmpty(_configuration.UtilizationFullHostName) - ? _configuration.UtilizationFullHostName - : _configuration.UtilizationHostName; + var modelsCollection = new LogEventWireModelCollection( + _configuration.ApplicationNames.ElementAt(0), + _configuration.EntityGuid, + hostname, + _configuration.LabelsEnabled ? _labelsService.GetFilteredLabels(_configuration.LabelsExclude) : [], + aggregatedEvents); - var modelsCollection = new LogEventWireModelCollection( - _configuration.ApplicationNames.ElementAt(0), - _configuration.EntityGuid, - hostname, - _configuration.LabelsEnabled ? _labelsService.GetFilteredLabels(_configuration.LabelsExclude) : [], - aggregatedEvents); + var responseStatus = DataTransportService.Send(modelsCollection, transactionId); - var responseStatus = DataTransportService.Send(modelsCollection, transactionId); - - HandleResponse(responseStatus, aggregatedEvents); + HandleResponse(responseStatus, aggregatedEvents); + } - Log.Finest("Log Event harvest finished."); + Log.Finest($"Log Event harvest finished. {eventCount} event(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs index fd4c7d9581..819e2dde44 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using NewRelic.Agent.Core.DataTransport; using NewRelic.Agent.Core.Events; using NewRelic.Agent.Core.Metrics; @@ -75,12 +76,16 @@ protected void InternalHarvest(string transactionId = null) var oldMetrics = GetStatsCollectionForHarvest(); oldMetrics.MergeUnscopedStats(MetricNames.SupportabilityMetricHarvestTransmit, MetricDataWireModel.BuildCountData()); - var metricsToSend = oldMetrics.ConvertToJsonForSending(_metricNameService); + var metricsToSend = oldMetrics.ConvertToJsonForSending(_metricNameService).ToList(); - var responseStatus = DataTransportService.Send(metricsToSend, transactionId); - HandleResponse(responseStatus, metricsToSend); + var eventCount = metricsToSend.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(metricsToSend, transactionId); + HandleResponse(responseStatus, metricsToSend); + } - Log.Debug("Metric harvest finished."); + Log.Finest($"Metric harvest finished. {eventCount} metric(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/SpanEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/SpanEventAggregator.cs index 45813b29bc..6f050dcec6 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/SpanEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/SpanEventAggregator.cs @@ -123,16 +123,16 @@ protected void InternalHarvest(string transactionId = null) var eventHarvestData = new EventHarvestData(spanEventsPriorityQueue.Size, spanEventsPriorityQueue.GetAddAttemptsCount()); var wireModels = spanEventsPriorityQueue.Where(node => null != node).Select(node => node.Data).ToList(); - - // if we don't have any events to publish then don't - if (wireModels.Count <= 0) - return; - - var responseStatus = DataTransportService.Send(eventHarvestData, wireModels, transactionId); - HandleResponse(responseStatus, wireModels); + // if we don't have any events to publish then don't + var eventCount = wireModels.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(eventHarvestData, wireModels, transactionId); + HandleResponse(responseStatus, wireModels); + } - Log.Finest("Span Event harvest finished."); + Log.Finest($"Span Event harvest finished. {eventCount} event(s) sent."); } private void ReduceReservoirSize(int newSize) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/SqlTraceAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/SqlTraceAggregator.cs index f090294a65..df77a6f567 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/SqlTraceAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/SqlTraceAggregator.cs @@ -66,14 +66,15 @@ protected void InternalHarvest(string transactionId = null) .Take(_configuration.SqlTracesPerPeriod) .ToList(); - if (!slowestTraces.Any()) - return; - - var responseStatus = DataTransportService.Send(slowestTraces, transactionId); - - HandleResponse(responseStatus, slowestTraces); + // if we don't have any traces to publish then don't + var traceCount = slowestTraces.Count; + if (traceCount > 0) + { + var responseStatus = DataTransportService.Send(slowestTraces, transactionId); + HandleResponse(responseStatus, slowestTraces); + } - Log.Finest("SQL Trace harvest finished."); + Log.Finest($"SQL Trace harvest finished. {traceCount} trace(s) sent."); } private void HandleResponse(DataTransportResponseStatus responseStatus, ICollection traces) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionEventAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionEventAggregator.cs index b6af92a475..3b1e65af7d 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionEventAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionEventAggregator.cs @@ -96,14 +96,16 @@ protected void InternalHarvest(string transactionId = null) var eventHarvestData = new EventHarvestData(originalTransactionEvents.Size, originalTransactionEvents.GetAddAttemptsCount()); // if we don't have any events to publish then don't - if (aggregatedEvents.Count <= 0) - return; + var eventCount = aggregatedEvents.Count; + if (eventCount > 0) + { + var responseStatus = DataTransportService.Send(eventHarvestData, aggregatedEvents, transactionId); - var responseStatus = DataTransportService.Send(eventHarvestData, aggregatedEvents, transactionId); + HandleResponse(responseStatus, aggregatedEvents); + } - HandleResponse(responseStatus, aggregatedEvents); + Log.Finest($"Transaction Event harvest finished. {eventCount} event(s) sent."); - Log.Finest("Transaction Event harvest finished."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs index 7846c017c2..156dc0d5e9 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs @@ -59,13 +59,17 @@ protected void InternalHarvest(string transactionId = null) .Select(t => t.CreateWireModel()) .ToList(); - if (!traceWireModels.Any()) - return; + // if we don't have any traces to publish then don't + var traceCount = traceWireModels.Count; + if (traceCount > 0) + { + LogUnencodedTraceData(traceWireModels); - LogUnencodedTraceData(traceWireModels); + var responseStatus = DataTransportService.Send(traceWireModels, transactionId); + HandleResponse(responseStatus, traceSamples); + } - var responseStatus = DataTransportService.Send(traceWireModels, transactionId); - HandleResponse(responseStatus, traceSamples); + Log.Finest($"Transaction Trace harvest finished. {traceCount} trace(s) sent."); } private void HandleResponse(DataTransportResponseStatus responseStatus, ICollection traceSamples) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs index 1779a985a5..9b48734a19 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs @@ -1,13 +1,10 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #if !NETFRAMEWORK -using System; using System.Net.Http; using System.Threading; using System.Threading.Tasks; -using NewRelic.Agent.Configuration; -using NewRelic.Agent.Core.Config; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; namespace NewRelic.Agent.Core.DataTransport.Client @@ -34,18 +31,6 @@ public async Task SendAsync(HttpRequestMessage mess var cts = new CancellationTokenSource(_timeoutMilliseconds); return new HttpResponseMessageWrapper(await _httpClient.SendAsync(message, cts.Token)); } - - public TimeSpan Timeout - { - get - { - return _httpClient.Timeout; - } - set - { - _httpClient.Timeout = value; - } - } } } #endif diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientFactory.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientFactory.cs index 921446e134..f8509f840c 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientFactory.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientFactory.cs @@ -1,4 +1,4 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 using System.Net; @@ -8,6 +8,6 @@ namespace NewRelic.Agent.Core.DataTransport.Client.Interfaces { public interface IHttpClientFactory { - public IHttpClient CreateClient(IWebProxy proxy, IConfiguration configuration); + public IHttpClient GetOrCreateClient(IWebProxy proxy, IConfiguration configuration); } } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientWrapper.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientWrapper.cs index 708180dd9e..bbc234b7cd 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientWrapper.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/Interfaces/IHttpClientWrapper.cs @@ -11,8 +11,6 @@ namespace NewRelic.Agent.Core.DataTransport.Client.Interfaces public interface IHttpClientWrapper : IDisposable { Task SendAsync(HttpRequestMessage message); - - TimeSpan Timeout { get; set; } } } #endif diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs index ad0b445ae3..54b3e91d0d 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs @@ -6,7 +6,7 @@ using System.Net; using System.Net.Http; using System.Net.Http.Headers; -using System.Threading.Tasks; +using System.Reflection; using NewRelic.Agent.Configuration; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; using NewRelic.Agent.Extensions.Logging; @@ -26,12 +26,54 @@ public NRHttpClient(IWebProxy proxy, IConfiguration configuration) : base(proxy) _configuration = configuration; // set the default timeout to "infinite", but specify the configured collector timeout as the actual timeout for SendAsync() calls - var httpHandler = new HttpClientHandler { Proxy = proxy }; - Log.Info("Current HttpClientHandler TLS Configuration (HttpClientHandler.SslProtocols): {0}", httpHandler.SslProtocols.ToString()); - var httpClient = new HttpClient(httpHandler, true) {Timeout = System.Threading.Timeout.InfiniteTimeSpan}; + var httpHandler = GetHttpHandler(proxy); + + var httpClient = new HttpClient(httpHandler, true) { Timeout = System.Threading.Timeout.InfiniteTimeSpan }; _httpClientWrapper = new HttpClientWrapper(httpClient, (int)configuration.CollectorTimeout); } + private dynamic GetHttpHandler(IWebProxy proxy) + { + // check whether the application is running .NET 6 or later + // if so, set the pooled connection lifetime to 1 minute + // https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclienthandler.pooledconnectionlifetime?view=net-6.0 + + if (System.Environment.Version.Major >= 6) + { + try + { + var pooledConnectionLifetime = TimeSpan.FromMinutes(5); // an in-use connection will be closed and recycled after 5 minutes + var pooledConnectionIdleTimeout = TimeSpan.FromMinutes(1); // a connection that is idle for 1 minute will be closed and recycled + + Log.Info($"Creating a SocketsHttpHandler with PooledConnectionLifetime set to {pooledConnectionLifetime} and PooledConnectionIdleTimeout set to {pooledConnectionIdleTimeout}"); + + // use reflection to create a SocketsHttpHandler instance and set the PooledConnectionLifetime to 1 minute + var assembly = Assembly.Load("System.Net.Http"); + var handlerType = assembly.GetType("System.Net.Http.SocketsHttpHandler"); + dynamic handler = Activator.CreateInstance(handlerType); + + handler.PooledConnectionLifetime = pooledConnectionLifetime; + handler.PooledConnectionIdleTimeout = pooledConnectionIdleTimeout; + + handler.Proxy = proxy; + + Log.Info("Current SocketsHttpHandler TLS Configuration (SocketsHttpHandler.SslOptions): {0}", handler.SslOptions.EnabledSslProtocols); + return handler; + } + catch (Exception e) + { + Log.Info(e, "Application is running .NET 6+ but an exception occurred trying to create SocketsHttpHandler. Falling back to HttpHandler."); + } + } + + // if the application is not running .NET 6 or later, use the default HttpClientHandler + var httpClientHandler = new HttpClientHandler { Proxy = proxy }; + Log.Info("Current HttpClientHandler TLS Configuration (HttpClientHandler.SslProtocols): {0}", httpClientHandler.SslProtocols.ToString()); + + return httpClientHandler; + } + + public override IHttpResponse Send(IHttpRequest request) { try diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClientFactory.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClientFactory.cs index a44c48ba6f..59f5035f0f 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClientFactory.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClientFactory.cs @@ -1,9 +1,8 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #if !NETFRAMEWORK using System.Net; -using System.Threading; using NewRelic.Agent.Configuration; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; @@ -19,7 +18,7 @@ public class NRHttpClientFactory : IHttpClientFactory private bool? _hasProxy; - public IHttpClient CreateClient(IWebProxy proxy, IConfiguration configuration) + public IHttpClient GetOrCreateClient(IWebProxy proxy, IConfiguration configuration) { var proxyRequired = (proxy != null); if (_httpClient != null && (_hasProxy == proxyRequired)) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/WebRequestHttpClientFactory.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/WebRequestHttpClientFactory.cs index 803ac67b5c..146b3d5fb2 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/WebRequestHttpClientFactory.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/WebRequestHttpClientFactory.cs @@ -1,4 +1,4 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #if NETFRAMEWORK using System.Net; @@ -18,7 +18,7 @@ public WebRequestHttpClientFactory() { } - public IHttpClient CreateClient(IWebProxy proxy, IConfiguration configuration) + public IHttpClient GetOrCreateClient(IWebProxy proxy, IConfiguration configuration) { return new NRWebRequestClient(proxy, configuration); } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs index eb67f8c641..bbc0c4c957 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs @@ -323,7 +323,7 @@ private string GetIdentifier() var appNames = string.Join(":", _configuration.ApplicationNames.ToArray()); #if NETSTANDARD2_0 - return $"{Path.GetFileName(_processStatic.GetCurrentProcess().MainModuleFileName)}{appNames}"; + return $"{Path.GetFileName(_processStatic.GetCurrentProcess().MainModuleFileName)}{appNames}"; #else return HttpRuntime.AppDomainAppId != null @@ -420,34 +420,36 @@ private T SendNonDataRequest(string method, params object[] data) private T SendDataOverWire(ICollectorWire wire, string method, params object[] data) { var requestGuid = Guid.NewGuid(); + string serializedData; try { - var serializedData = _serializer.Serialize(data); - try - { - var responseBody = wire.SendData(method, _connectionInfo, serializedData, requestGuid); - return ParseResponse(responseBody); - } - catch (DataTransport.HttpException ex) - { - Log.Debug(ex, "Request({0}): Received a {1} {2} response invoking method \"{3}\" with payload \"{4}\"", requestGuid, (int)ex.StatusCode, ex.StatusCode, method, serializedData); + serializedData = _serializer.Serialize(data); + } + catch (Exception ex) + { + Log.Debug("Request({0}): Exception occurred serializing request data: {1}", requestGuid, ex); // log message only since exception is rethrown + throw; + } - if (ex.StatusCode == HttpStatusCode.Gone) - { - Log.Info(ex, "Request({0}): The server has requested that the agent disconnect. The agent is shutting down.", requestGuid); - } + try + { + var responseBody = wire.SendData(method, _connectionInfo, serializedData, requestGuid); + return ParseResponse(responseBody); + } + catch (DataTransport.HttpException ex) + { + Log.Debug(ex, "Request({0}): Received a {1} {2} response invoking method \"{3}\" with payload \"{4}\"", requestGuid, (int)ex.StatusCode, ex.StatusCode, method, serializedData); - throw; - } - catch (Exception ex) + if (ex.StatusCode == HttpStatusCode.Gone) { - Log.Debug(ex, "Request({0}): An error occurred invoking method \"{1}\" with payload \"{2}\": {3}", requestGuid, method, serializedData, ex); - throw; + Log.Info("Request({0}): The server has requested that the agent disconnect. The agent is shutting down.", requestGuid); } + + throw; } catch (Exception ex) { - Log.Debug(ex, "Request({0}): Exception occurred serializing request data: {1}", requestGuid, ex); + Log.Debug("Request({0}): An error occurred invoking method \"{1}\" with payload \"{2}\": {3}", requestGuid, method, serializedData, ex); // log message only since exception is rethrown throw; } } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs index e31d2c5a34..5813b69e63 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs @@ -2,16 +2,15 @@ // SPDX-License-Identifier: Apache-2.0 using NewRelic.Agent.Core.Events; -using NewRelic.Agent.Core.DataTransport; using NewRelic.Agent.Core.Time; using NewRelic.Agent.Core.Utilities; using NewRelic.Agent.Extensions.Logging; using System; using System.IO; -using System.Linq; using System.Net; #if !NETFRAMEWORK using System.Net.Http; +using System.Threading.Tasks; #endif using System.Net.Sockets; @@ -119,6 +118,7 @@ private void HandleConnectionException(Exception ex) // Occurs when the agent is unable to connect to APM. The request failed due to an underlying // issue such as network connectivity, DNS failure, server certificate validation or timeout. case HttpRequestException: + case TaskCanceledException: #endif // Occurs when the agent connects to APM but the connection gets aborted by the collector case SocketException: diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs index 62da5474d7..ece99d68cf 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs @@ -17,7 +17,9 @@ using System.Linq; using System.Net; using System.Net.Sockets; - +#if !NETFRAMEWORK +using System.Threading.Tasks; +#endif namespace NewRelic.Agent.Core.DataTransport { public interface IDataTransportService @@ -176,11 +178,18 @@ private DataTransportResponse TrySendDataRequest(string method, params obj var errorStatus = GetDataTransportResponseStatusByHttpStatusCode(ex.StatusCode); return new DataTransportResponse(errorStatus); } - catch (Exception ex) when (ex is SocketException || ex is WebException || ex is OperationCanceledException) + catch (Exception ex) when (ex is SocketException or WebException or OperationCanceledException) + { + LogErrorResponse(ex, method, startTime, null); + return new DataTransportResponse(DataTransportResponseStatus.Retain); + } +#if !NETFRAMEWORK + catch (Exception ex) when (ex is TaskCanceledException) // This exception is specific to .NET Core { LogErrorResponse(ex, method, startTime, null); return new DataTransportResponse(DataTransportResponseStatus.Retain); } +#endif catch (Exception ex) { LogErrorResponse(ex, method, startTime, null); diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs index 0c34f280f2..abc8f9876d 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs @@ -45,7 +45,7 @@ public string SendData(string method, ConnectionInfo connectionInfo, string seri try { - var httpClient = _httpClientFactory.CreateClient(connectionInfo.Proxy, _configuration); + var httpClient = _httpClientFactory.GetOrCreateClient(connectionInfo.Proxy, _configuration); request = new HttpRequest(_configuration) { diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientFactoryTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientFactoryTests.cs index d5776005b9..64666a86f9 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientFactoryTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientFactoryTests.cs @@ -1,4 +1,4 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. +// Copyright 2020 New Relic, Inc. All rights reserved. // SPDX-License-Identifier: Apache-2.0 #if !NETFRAMEWORK @@ -36,7 +36,7 @@ public void SetUp() [Test] public void CreateClient_NotNull() { - var client = _httpClientFactory.CreateClient(null, _mockConfiguration); + var client = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); Assert.That(client, Is.Not.Null); } @@ -44,8 +44,8 @@ public void CreateClient_NotNull() [Test] public void CreateClient_NoProxy_ReturnsSameClient() { - var clientA = _httpClientFactory.CreateClient(null, _mockConfiguration); - var clientB = _httpClientFactory.CreateClient(null, _mockConfiguration); + var clientA = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); + var clientB = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); Assert.That(clientA, Is.EqualTo(clientB)); } @@ -53,8 +53,8 @@ public void CreateClient_NoProxy_ReturnsSameClient() [Test] public void CreateClient_Proxy_ReturnsSameClient() { - var clientA = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration); - var clientB = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration); + var clientA = _httpClientFactory.GetOrCreateClient(_mockProxy, _mockConfiguration); + var clientB = _httpClientFactory.GetOrCreateClient(_mockProxy, _mockConfiguration); Assert.That(clientA, Is.EqualTo(clientB)); } @@ -62,8 +62,8 @@ public void CreateClient_Proxy_ReturnsSameClient() [Test] public void CreateClient_NoProxyToProxy_ReturnsNewClient() { - var clientA = _httpClientFactory.CreateClient(null, _mockConfiguration); - var clientB = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration); + var clientA = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); + var clientB = _httpClientFactory.GetOrCreateClient(_mockProxy, _mockConfiguration); Assert.That(clientA, Is.Not.EqualTo(clientB)); } @@ -71,8 +71,8 @@ public void CreateClient_NoProxyToProxy_ReturnsNewClient() [Test] public void CreateClient_ProxyToNoProxy_ReturnsNewClient() { - var clientA = _httpClientFactory.CreateClient(_mockProxy, _mockConfiguration); - var clientB = _httpClientFactory.CreateClient(null, _mockConfiguration); + var clientA = _httpClientFactory.GetOrCreateClient(_mockProxy, _mockConfiguration); + var clientB = _httpClientFactory.GetOrCreateClient(null, _mockConfiguration); Assert.That(clientA, Is.Not.EqualTo(clientB)); } diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/HttpCollectorWireTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/HttpCollectorWireTests.cs index b7508f69b6..fd88b6aad7 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/HttpCollectorWireTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/HttpCollectorWireTests.cs @@ -64,7 +64,7 @@ public void SendData_ShouldSendRequestWithValidParameters(bool usePutForSend) var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Returns(mockHttpResponse); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); @@ -73,7 +73,7 @@ public void SendData_ShouldSendRequestWithValidParameters(bool usePutForSend) // Assert Assert.That(response, Is.EqualTo(expected)); - Mock.Assert(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); + Mock.Assert(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); } [Test] @@ -96,14 +96,14 @@ public void SendData_ShouldThrowHttpRequestException_WhenRequestThrows() var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Throws(new HttpRequestException()); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); // Act and Assert Assert.Throws(() => collectorWire.SendData("test_method", connectionInfo, serializedData, Guid.NewGuid())); - Mock.Assert(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); + Mock.Assert(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); } [Test] @@ -126,14 +126,14 @@ public void SendData_ShouldThrowHttpException_WhenResponse_IsNotSuccessful() var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Returns(mockHttpResponse); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); // Act and Assert Assert.Throws(() => collectorWire.SendData("test_method", connectionInfo, serializedData, Guid.NewGuid())); - Mock.Assert(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); + Mock.Assert(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny()), Occurs.Once()); } [Test] @@ -158,7 +158,7 @@ public void SendData_ShouldDropPayload_WhenPayloadSizeExceedsMaxSize() var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Returns(mockHttpResponse); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); @@ -192,7 +192,7 @@ public void SendData_ShouldNotCallAuditLog_UnlessAuditLogIsEnabled(bool isEnable var mockHttpClient = Mock.Create(); Mock.Arrange(() => mockHttpClient.Send(Arg.IsAny())).Returns(mockHttpResponse); - Mock.Arrange(() => _httpClientFactory.CreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); + Mock.Arrange(() => _httpClientFactory.GetOrCreateClient(Arg.IsAny(), Arg.IsAny())).Returns(mockHttpClient); var collectorWire = CreateHttpCollectorWire(); From c39c9edd7d550ad40fad62c79e55479bfefa0075 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Wed, 18 Dec 2024 12:03:23 -0600 Subject: [PATCH 02/27] Minor cleanup --- .../NewRelic/Agent/Core/DataTransport/ConnectionManager.cs | 1 + .../NewRelic/Agent/Core/DataTransport/DataTransportService.cs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs index 5813b69e63..f6ddba0c7f 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs @@ -118,6 +118,7 @@ private void HandleConnectionException(Exception ex) // Occurs when the agent is unable to connect to APM. The request failed due to an underlying // issue such as network connectivity, DNS failure, server certificate validation or timeout. case HttpRequestException: + // Occurs when HttpClient.SendAsync() times out on .NET 6+ case TaskCanceledException: #endif // Occurs when the agent connects to APM but the connection gets aborted by the collector diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs index ece99d68cf..799a65c6da 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs @@ -184,7 +184,7 @@ private DataTransportResponse TrySendDataRequest(string method, params obj return new DataTransportResponse(DataTransportResponseStatus.Retain); } #if !NETFRAMEWORK - catch (Exception ex) when (ex is TaskCanceledException) // This exception is specific to .NET Core + catch (Exception ex) when (ex is TaskCanceledException) // This exception is specific to .NET 6+ { LogErrorResponse(ex, method, startTime, null); return new DataTransportResponse(DataTransportResponseStatus.Retain); From 948d21c056d667213f5e984a8d1f0413e56fa6eb Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Wed, 18 Dec 2024 12:08:52 -0600 Subject: [PATCH 03/27] More cleanup --- .../NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs | 6 +++--- .../NewRelic/Agent/Core/Aggregators/MetricAggregator.cs | 6 +++--- .../Agent/Core/Aggregators/TransactionTraceAggregator.cs | 2 ++ 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs index 7448a9e9e6..8384ce2363 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/ErrorTraceAggregator.cs @@ -80,15 +80,15 @@ protected void InternalHarvest(string transactionId = null) } // if we don't have any events to publish then don't - var eventCount = errorTraceWireModels.Count; - if (eventCount > 0) + var traceCount = errorTraceWireModels.Count; + if (traceCount > 0) { var responseStatus = DataTransportService.Send(errorTraceWireModels, transactionId); HandleResponse(responseStatus, errorTraceWireModels); } - Log.Finest($"Error Trace harvest finished. {eventCount} event(s) sent."); + Log.Finest($"Error Trace harvest finished. {traceCount} trace(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs index 819e2dde44..f82d53ba5c 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/MetricAggregator.cs @@ -78,14 +78,14 @@ protected void InternalHarvest(string transactionId = null) oldMetrics.MergeUnscopedStats(MetricNames.SupportabilityMetricHarvestTransmit, MetricDataWireModel.BuildCountData()); var metricsToSend = oldMetrics.ConvertToJsonForSending(_metricNameService).ToList(); - var eventCount = metricsToSend.Count; - if (eventCount > 0) + var metricCount = metricsToSend.Count; + if (metricCount > 0) { var responseStatus = DataTransportService.Send(metricsToSend, transactionId); HandleResponse(responseStatus, metricsToSend); } - Log.Finest($"Metric harvest finished. {eventCount} metric(s) sent."); + Log.Finest($"Metric harvest finished. {metricCount} metric(s) sent."); } protected override void OnConfigurationUpdated(ConfigurationUpdateSource configurationUpdateSource) diff --git a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs index 156dc0d5e9..e338fed3cd 100644 --- a/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs +++ b/src/Agent/NewRelic/Agent/Core/Aggregators/TransactionTraceAggregator.cs @@ -49,6 +49,8 @@ public override void Collect(TransactionTraceWireModelComponents transactionTrac protected void InternalHarvest(string transactionId = null) { + Log.Finest("Transaction Trace harvest starting."); + var traceSamples = _transactionCollectors .Where(t => t != null) .SelectMany(t => t.GetCollectedSamples()) From 5648aac5b9663fa39fc85a45f59f2954b6626f1b Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Wed, 18 Dec 2024 13:44:14 -0600 Subject: [PATCH 04/27] Code cleanup, logging --- .../DataTransport/Client/HttpClientWrapper.cs | 18 ++++++++++++++++-- .../Core/DataTransport/Client/NRHttpClient.cs | 2 ++ .../Core/DataTransport/ConnectionHandler.cs | 2 +- .../Core/DataTransport/DataTransportService.cs | 2 +- .../Core/DataTransport/HttpCollectorWire.cs | 1 + 5 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs index 9b48734a19..3f79ed3dc9 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs @@ -2,10 +2,12 @@ // SPDX-License-Identifier: Apache-2.0 #if !NETFRAMEWORK +using System; using System.Net.Http; using System.Threading; using System.Threading.Tasks; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; +using NewRelic.Agent.Extensions.Logging; namespace NewRelic.Agent.Core.DataTransport.Client { @@ -28,8 +30,20 @@ public void Dispose() public async Task SendAsync(HttpRequestMessage message) { - var cts = new CancellationTokenSource(_timeoutMilliseconds); - return new HttpResponseMessageWrapper(await _httpClient.SendAsync(message, cts.Token)); + using var cts = new CancellationTokenSource(_timeoutMilliseconds); + try + { + var httpResponseMessage = await _httpClient.SendAsync(message, cts.Token); + return new HttpResponseMessageWrapper(httpResponseMessage); + } + catch (Exception e) + { + Log.Debug(cts.IsCancellationRequested + ? $"HttpClient.SendAsync() timed out after {_timeoutMilliseconds}ms." + : $"HttpClient.SendAsync() threw an unexpected exception: {e}"); + + throw; + } } } } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs index 54b3e91d0d..7e90622bc3 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs @@ -107,7 +107,9 @@ public override IHttpResponse Send(IHttpRequest request) req.Content.Headers.Add(contentHeader.Key, contentHeader.Value); } + Log.Finest($"Request({request.RequestGuid}: Sending"); var response = _httpClientWrapper.SendAsync(req).GetAwaiter().GetResult(); + Log.Finest($"Request({request.RequestGuid}: Sent"); var httpResponse = new HttpResponse(request.RequestGuid, response); return httpResponse; diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs index bbc0c4c957..2b821143fe 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs @@ -438,7 +438,7 @@ private T SendDataOverWire(ICollectorWire wire, string method, params object[ } catch (DataTransport.HttpException ex) { - Log.Debug(ex, "Request({0}): Received a {1} {2} response invoking method \"{3}\" with payload \"{4}\"", requestGuid, (int)ex.StatusCode, ex.StatusCode, method, serializedData); + Log.Debug("Request({0}): Received a {1} {2} response invoking method \"{3}\" with payload \"{4}\"", requestGuid, (int)ex.StatusCode, ex.StatusCode, method, serializedData); if (ex.StatusCode == HttpStatusCode.Gone) { diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs index 799a65c6da..d2b69cd853 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs @@ -234,7 +234,7 @@ private void LogErrorResponse(Exception exception, string method, DateTime start { var endTime = DateTime.UtcNow; _agentHealthReporter.ReportSupportabilityCollectorErrorException(method, endTime - startTime, httpStatusCode); - Log.Error(exception, ""); + Log.Error(exception, "_connectionManager.SendDataRequest() failed."); } private DataTransportResponseStatus GetDataTransportResponseStatusByHttpStatusCode(HttpStatusCode httpStatusCode) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs index abc8f9876d..e5b352c519 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/HttpCollectorWire.cs @@ -45,6 +45,7 @@ public string SendData(string method, ConnectionInfo connectionInfo, string seri try { + Log.Finest("Request({0}): Invoking \"{1}\"", requestGuid, method); var httpClient = _httpClientFactory.GetOrCreateClient(connectionInfo.Proxy, _configuration); request = new HttpRequest(_configuration) From b890de5fc6b2ed52ee49ee44a0bee680c219cfab Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Wed, 18 Dec 2024 13:52:35 -0600 Subject: [PATCH 05/27] Removed unnecessary TaskCanceledException handling, add clarifying comments --- .../Agent/Core/DataTransport/ConnectionManager.cs | 3 +-- .../Core/DataTransport/DataTransportService.cs | 14 ++------------ 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs index f6ddba0c7f..36983e6bed 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs @@ -118,8 +118,6 @@ private void HandleConnectionException(Exception ex) // Occurs when the agent is unable to connect to APM. The request failed due to an underlying // issue such as network connectivity, DNS failure, server certificate validation or timeout. case HttpRequestException: - // Occurs when HttpClient.SendAsync() times out on .NET 6+ - case TaskCanceledException: #endif // Occurs when the agent connects to APM but the connection gets aborted by the collector case SocketException: @@ -128,6 +126,7 @@ private void HandleConnectionException(Exception ex) // Occurs when no network connection is available, DNS unavailable, etc. case WebException: // Usually occurs when a request times out but did not get far enough along to trigger a timeout exception + // OperationCanceledException is a base class for TaskCanceledException, which can be thrown by HttpClient.SendAsync in .NET 6+ case OperationCanceledException: Log.Info("Connection failed: {0}", ex.Message); break; diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs index d2b69cd853..5a97891122 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs @@ -5,7 +5,6 @@ using NewRelic.Agent.Core.Aggregators; using NewRelic.Agent.Core.Commands; using NewRelic.Agent.Core.Events; -using NewRelic.Agent.Core.DataTransport; using NewRelic.Agent.Core.Segments; using NewRelic.Agent.Core.ThreadProfiling; using NewRelic.Agent.Core.Utilities; @@ -17,9 +16,6 @@ using System.Linq; using System.Net; using System.Net.Sockets; -#if !NETFRAMEWORK -using System.Threading.Tasks; -#endif namespace NewRelic.Agent.Core.DataTransport { public interface IDataTransportService @@ -178,18 +174,12 @@ private DataTransportResponse TrySendDataRequest(string method, params obj var errorStatus = GetDataTransportResponseStatusByHttpStatusCode(ex.StatusCode); return new DataTransportResponse(errorStatus); } - catch (Exception ex) when (ex is SocketException or WebException or OperationCanceledException) + // OperationCanceledException is a base class for TaskCanceledException, which can be thrown by HttpClient.SendAsync in .NET 6+ + catch (Exception ex) when (ex is SocketException or WebException or OperationCanceledException) { LogErrorResponse(ex, method, startTime, null); return new DataTransportResponse(DataTransportResponseStatus.Retain); } -#if !NETFRAMEWORK - catch (Exception ex) when (ex is TaskCanceledException) // This exception is specific to .NET 6+ - { - LogErrorResponse(ex, method, startTime, null); - return new DataTransportResponse(DataTransportResponseStatus.Retain); - } -#endif catch (Exception ex) { LogErrorResponse(ex, method, startTime, null); From 56a25376527d025e4fe5acdb5c8ac01da1b73f97 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Wed, 18 Dec 2024 13:54:06 -0600 Subject: [PATCH 06/27] More cleanup --- .../NewRelic/Agent/Core/DataTransport/DataTransportService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs index 5a97891122..ae81fda0d8 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/DataTransportService.cs @@ -224,7 +224,7 @@ private void LogErrorResponse(Exception exception, string method, DateTime start { var endTime = DateTime.UtcNow; _agentHealthReporter.ReportSupportabilityCollectorErrorException(method, endTime - startTime, httpStatusCode); - Log.Error(exception, "_connectionManager.SendDataRequest() failed."); + Log.Error(exception, "Exception in TrySendDataRequest():"); } private DataTransportResponseStatus GetDataTransportResponseStatusByHttpStatusCode(HttpStatusCode httpStatusCode) From cad4f8747780b1f9b61a41ce71384ebac4df2b9a Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Thu, 19 Dec 2024 09:25:26 -0600 Subject: [PATCH 07/27] Added a few `using`s --- .../Core/DataTransport/Client/NRHttpClient.cs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs index 7e90622bc3..18f6326202 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs @@ -35,9 +35,6 @@ public NRHttpClient(IWebProxy proxy, IConfiguration configuration) : base(proxy) private dynamic GetHttpHandler(IWebProxy proxy) { // check whether the application is running .NET 6 or later - // if so, set the pooled connection lifetime to 1 minute - // https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclienthandler.pooledconnectionlifetime?view=net-6.0 - if (System.Environment.Version.Major >= 6) { try @@ -78,12 +75,9 @@ public override IHttpResponse Send(IHttpRequest request) { try { - var req = new HttpRequestMessage - { - RequestUri = request.Uri, - Method = _configuration.PutForDataSend ? HttpMethod.Put : HttpMethod.Post - }; - + using var req = new HttpRequestMessage(); + req.RequestUri = request.Uri; + req.Method = _configuration.PutForDataSend ? HttpMethod.Put : HttpMethod.Post; req.Headers.Add("User-Agent", $"NewRelic-DotNetAgent/{AgentInstallConfiguration.AgentVersion}"); req.Headers.Add("Connection", "keep-alive"); req.Headers.Add("Keep-Alive", "true"); @@ -94,7 +88,7 @@ public override IHttpResponse Send(IHttpRequest request) req.Headers.Add(header.Key, header.Value); } - var content = new ByteArrayContent(request.Content.PayloadBytes); + using var content = new ByteArrayContent(request.Content.PayloadBytes); var encoding = request.Content.IsCompressed ? request.Content.CompressionType.ToLower() : "identity"; content.Headers.ContentType = new MediaTypeHeaderValue(request.Content.ContentType); content.Headers.Add("Content-Encoding", encoding); @@ -108,7 +102,7 @@ public override IHttpResponse Send(IHttpRequest request) } Log.Finest($"Request({request.RequestGuid}: Sending"); - var response = _httpClientWrapper.SendAsync(req).GetAwaiter().GetResult(); + using var response = _httpClientWrapper.SendAsync(req).GetAwaiter().GetResult(); Log.Finest($"Request({request.RequestGuid}: Sent"); var httpResponse = new HttpResponse(request.RequestGuid, response); From 9ea72b1a9ad79e31ae75ace916443fcc09e39f5c Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Thu, 19 Dec 2024 09:55:39 -0600 Subject: [PATCH 08/27] Oops. A little too liberal with the `using`s --- .../NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs index 18f6326202..9583fe91a5 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs @@ -102,7 +102,7 @@ public override IHttpResponse Send(IHttpRequest request) } Log.Finest($"Request({request.RequestGuid}: Sending"); - using var response = _httpClientWrapper.SendAsync(req).GetAwaiter().GetResult(); + var response = _httpClientWrapper.SendAsync(req).GetAwaiter().GetResult(); Log.Finest($"Request({request.RequestGuid}: Sent"); var httpResponse = new HttpResponse(request.RequestGuid, response); From f5508913eab28b7f450eb0a5cdf1b3e804966bf1 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:38:27 -0600 Subject: [PATCH 09/27] Unit tests --- .../DataTransport/Client/NRHttpClientTests.cs | 66 +++++++++++++++++-- .../ConvertMetricDataToJsonTests.cs | 20 +++++- 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs index 23ad71b34c..f2c46d94a9 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs @@ -3,6 +3,7 @@ #if !NETFRAMEWORK using System; +using System.Linq; using System.Net; using System.Net.Http; using System.Threading.Tasks; @@ -27,16 +28,17 @@ public class NRHttpClientTests public void SetUp() { _mockConfiguration = Mock.Create(); - Mock.Arrange(() => _mockConfiguration.AgentLicenseKey).Returns( "12345"); + Mock.Arrange(() => _mockConfiguration.AgentLicenseKey).Returns("12345"); Mock.Arrange(() => _mockConfiguration.AgentRunId).Returns("123"); Mock.Arrange(() => _mockConfiguration.CollectorMaxPayloadSizeInBytes).Returns(int.MaxValue); + Mock.Arrange(() => _mockConfiguration.CollectorTimeout).Returns(60000); // 60 seconds + Mock.Arrange(() => _mockConfiguration.PutForDataSend).Returns(false); _mockConnectionInfo = Mock.Create(); Mock.Arrange(() => _mockConnectionInfo.Host).Returns("testhost.com"); Mock.Arrange(() => _mockConnectionInfo.HttpProtocol).Returns("https"); Mock.Arrange(() => _mockConnectionInfo.Port).Returns(123); - _mockProxy = Mock.Create(); _mockHttpClientWrapper = Mock.Create(); @@ -50,6 +52,7 @@ public void TearDown() _client.Dispose(); _mockHttpClientWrapper.Dispose(); } + [Test] public void Send_ReturnsResponse_WhenSendAsyncSucceeds() { @@ -68,10 +71,11 @@ public void Send_ReturnsResponse_WhenSendAsyncSucceeds() // Assert Assert.That(response, Is.Not.Null); + Assert.That(response.StatusCode, Is.EqualTo(HttpStatusCode.OK)); } [Test] - public void SendAsync_ThrowsHttpRequestException_WhenSendAsyncThrows() + public void Send_ThrowsHttpRequestException_WhenSendAsyncThrows() { // Arrange var request = CreateHttpRequest(); @@ -83,6 +87,60 @@ public void SendAsync_ThrowsHttpRequestException_WhenSendAsyncThrows() Assert.Throws(() => _client.Send(request)); } + [Test] + public void Send_AddsCustomHeaders() + { + // Arrange + var request = CreateHttpRequest(); + request.Headers.Add("Custom-Header", "HeaderValue"); + + var mockHttpResponseMessage = Mock.Create(); + Mock.Arrange(() => mockHttpResponseMessage.StatusCode).Returns(HttpStatusCode.OK); + Mock.Arrange(() => mockHttpResponseMessage.IsSuccessStatusCode).Returns(true); + + Mock.Arrange(() => _mockHttpClientWrapper.SendAsync(Arg.IsAny())) + .ReturnsAsync(mockHttpResponseMessage); + + // Act + var response = _client.Send(request); + + // Assert + Mock.Assert(() => _mockHttpClientWrapper.SendAsync(Arg.Matches(req => + req.Headers.Contains("Custom-Header") && req.Headers.GetValues("Custom-Header").First() == "HeaderValue")), Occurs.Once()); + } + + [Test] + public void Send_SetsContentHeaders() + { + // Arrange + var request = CreateHttpRequest(); + + var mockHttpResponseMessage = Mock.Create(); + Mock.Arrange(() => mockHttpResponseMessage.StatusCode).Returns(HttpStatusCode.OK); + Mock.Arrange(() => mockHttpResponseMessage.IsSuccessStatusCode).Returns(true); + + Mock.Arrange(() => _mockHttpClientWrapper.SendAsync(Arg.IsAny())) + .ReturnsAsync(mockHttpResponseMessage); + + // Act + var response = _client.Send(request); + + // Assert + Mock.Assert(() => _mockHttpClientWrapper.SendAsync(Arg.Matches(req => + req.Content.Headers.ContentType.MediaType == "application/json" && + req.Content.Headers.ContentLength == request.Content.PayloadBytes.Length)), Occurs.Once()); + } + + [Test] + public void Dispose_DisposesHttpClientWrapper() + { + // Act + _client.Dispose(); + + // Assert + Mock.Assert(() => _mockHttpClientWrapper.Dispose(), Occurs.Once()); + } + private IHttpRequest CreateHttpRequest() { var request = new HttpRequest(_mockConfiguration) @@ -90,7 +148,7 @@ private IHttpRequest CreateHttpRequest() Endpoint = "Test", ConnectionInfo = _mockConnectionInfo, RequestGuid = Guid.NewGuid(), - Content = { SerializedData = "{\"Test\"}", ContentType = "application/json"} + Content = { SerializedData = "{\"Test\"}", ContentType = "application/json" } }; return request; diff --git a/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/ConvertMetricDataToJsonTests.cs b/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/ConvertMetricDataToJsonTests.cs index 23f7c0b55e..59ba644df9 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/ConvertMetricDataToJsonTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/JsonConverters/ConvertMetricDataToJsonTests.cs @@ -10,8 +10,10 @@ using NewRelic.Agent.Core.Utilities; using NewRelic.Agent.Core.WireModels; using NewRelic.Agent.Core.SharedInterfaces; +using Newtonsoft.Json; using NUnit.Framework; using Telerik.JustMock; +using JsonSerializer = NewRelic.Agent.Core.DataTransport.JsonSerializer; namespace NewRelic.Agent.Core.JsonConverters { @@ -24,7 +26,6 @@ public class ConvertMetricDataToJsonTests private MetricWireModelCollection _wellformedMetricData; private string _wellformedJson = "[\"440491846668652\",1450462672.0,1450462710.0,[[{\"name\":\"DotNet/name\",\"scope\":\"WebTransaction/DotNet/name\"},[1,3.0,2.0,3.0,3.0,9.0]],[{\"name\":\"Custom/name\"},[1,4.0,3.0,4.0,4.0,16.0]]]]"; - [SetUp] public void Setup() { @@ -55,6 +56,16 @@ public void Serialize_NoErrors() Assert.DoesNotThrow(() => _connectionHandler.SendDataRequest("metric_data", _wellformedMetricData)); } + [Test] + public void Serialize_ThrowsOnInvalidObject() + { + var node1 = new Node { Name = "Node1" }; + var node2 = new Node { Name = "Node2", Child = node1 }; + node1.Child = node2; // circular reference, should throw + + Assert.Throws(() => _connectionHandler.SendDataRequest("metric_data", node1)); + } + [Test] public void Serialize_MatchesExpectedOutput() { @@ -63,5 +74,12 @@ public void Serialize_MatchesExpectedOutput() var serializedMetrics = Newtonsoft.Json.JsonConvert.SerializeObject(model); Assert.That(serializedMetrics, Is.EqualTo(_wellformedJson)); } + + public class Node + { + public string Name { get; set; } + public Node Child { get; set; } + } + } } From d77a0a4994d7dc057bfa5fa55a9f48a800ed15a8 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Thu, 19 Dec 2024 13:10:12 -0600 Subject: [PATCH 10/27] Add AsyncHelper, toss in a few `.ConfigureAwait(false)` incantations. --- .../DataTransport/Client/HttpClientWrapper.cs | 2 +- .../Core/DataTransport/Client/NRHttpClient.cs | 3 +- .../Agent/Core/Utilities/AsyncHelper.cs | 34 +++++++++++++++++++ 3 files changed, 37 insertions(+), 2 deletions(-) create mode 100644 src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs index 3f79ed3dc9..a93fd19b38 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs @@ -33,7 +33,7 @@ public async Task SendAsync(HttpRequestMessage mess using var cts = new CancellationTokenSource(_timeoutMilliseconds); try { - var httpResponseMessage = await _httpClient.SendAsync(message, cts.Token); + var httpResponseMessage = await _httpClient.SendAsync(message, cts.Token).ConfigureAwait(false); return new HttpResponseMessageWrapper(httpResponseMessage); } catch (Exception e) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs index 9583fe91a5..9105b6c156 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs @@ -9,6 +9,7 @@ using System.Reflection; using NewRelic.Agent.Configuration; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; +using NewRelic.Agent.Core.Utilities; using NewRelic.Agent.Extensions.Logging; namespace NewRelic.Agent.Core.DataTransport.Client @@ -102,7 +103,7 @@ public override IHttpResponse Send(IHttpRequest request) } Log.Finest($"Request({request.RequestGuid}: Sending"); - var response = _httpClientWrapper.SendAsync(req).GetAwaiter().GetResult(); + var response = AsyncHelper.RunSync(() => _httpClientWrapper.SendAsync(req)); Log.Finest($"Request({request.RequestGuid}: Sent"); var httpResponse = new HttpResponse(request.RequestGuid, response); diff --git a/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs b/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs new file mode 100644 index 0000000000..5c3c72b351 --- /dev/null +++ b/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs @@ -0,0 +1,34 @@ +// Copyright 2020 New Relic, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#if !NETFRAMEWORK +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace NewRelic.Agent.Core.Utilities +{ + public class AsyncHelper + { + private static readonly TaskFactory _taskFactory = new + TaskFactory(CancellationToken.None, + TaskCreationOptions.None, + TaskContinuationOptions.None, + TaskScheduler.Default); + + /// + /// Safely executes an async method synchronously. + /// + /// + /// + /// + public static TReturn RunSync(Func> task) + { + return _taskFactory.StartNew(task) + .Unwrap() + .GetAwaiter() + .GetResult(); + } + } +} +#endif From 7c5534e39e278be11f8fcd905c2b4c69faa8dc6a Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Fri, 20 Dec 2024 16:09:38 -0600 Subject: [PATCH 11/27] Add `.ConfigureAwait(false)` to nearly all async calls --- src/Agent/NewRelic/Agent/Core/Agent.cs | 4 ++-- .../Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs | 6 +++--- .../Agent/Core/DataTransport/Client/HttpClientBase.cs | 2 +- .../Agent/Core/DataTransport/Client/HttpContentWrapper.cs | 2 +- .../Agent/Core/DataTransport/Client/HttpResponse.cs | 2 +- .../NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs | 5 ++++- .../Agent/Core/DataTransport/DataStreamingService.cs | 2 +- .../Agent/Core/Utilization/VendorHttpApiRequestor.cs | 4 ++-- .../AspNetCore6Plus/BrowserInjectingStreamWrapper.cs | 2 +- 9 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/Agent.cs b/src/Agent/NewRelic/Agent/Core/Agent.cs index 920906c93d..5da8235af1 100644 --- a/src/Agent/NewRelic/Agent/Core/Agent.cs +++ b/src/Agent/NewRelic/Agent/Core/Agent.cs @@ -325,10 +325,10 @@ public async Task TryInjectBrowserScriptAsync(string contentType, string request if (rumBytes == null) { transaction.LogFinest("Skipping RUM Injection: No script was available."); - await baseStream.WriteAsync(buffer, 0, buffer.Length); + await baseStream.WriteAsync(buffer, 0, buffer.Length).ConfigureAwait(false); } else - await BrowserScriptInjectionHelper.InjectBrowserScriptAsync(buffer, baseStream, rumBytes, transaction); + await BrowserScriptInjectionHelper.InjectBrowserScriptAsync(buffer, baseStream, rumBytes, transaction).ConfigureAwait(false); } private string TryGetRUMScriptInternal(string contentType, string requestPath) diff --git a/src/Agent/NewRelic/Agent/Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs b/src/Agent/NewRelic/Agent/Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs index 7b76a3c841..6257da2b4b 100644 --- a/src/Agent/NewRelic/Agent/Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs +++ b/src/Agent/NewRelic/Agent/Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs @@ -25,7 +25,7 @@ public static async Task InjectBrowserScriptAsync(byte[] buffer, Stream baseStre { // not found, can't inject anything transaction?.LogFinest("Skipping RUM Injection: No suitable location found to inject script."); - await baseStream.WriteAsync(buffer, 0, buffer.Length); + await baseStream.WriteAsync(buffer, 0, buffer.Length).ConfigureAwait(false); return; } @@ -34,13 +34,13 @@ public static async Task InjectBrowserScriptAsync(byte[] buffer, Stream baseStre if (index < buffer.Length) // validate index is less than buffer length { // Write everything up to the insertion index - await baseStream.WriteAsync(buffer, 0, index); + await baseStream.WriteAsync(buffer, 0, index).ConfigureAwait(false); // Write the RUM script await baseStream.WriteAsync(rumBytes, 0, rumBytes.Length); // Write the rest of the doc, starting after the insertion index - await baseStream.WriteAsync(buffer, index, buffer.Length - index); + await baseStream.WriteAsync(buffer, index, buffer.Length - index).ConfigureAwait(false); } else transaction?.LogFinest($"Skipping RUM Injection: Insertion index was invalid."); diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientBase.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientBase.cs index c619e8f9eb..2f8eef0ed0 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientBase.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientBase.cs @@ -66,7 +66,7 @@ protected void TestConnection() wc.DownloadString(testAddress); } #else - _lazyHttpClient.Value.GetAsync(testAddress).GetAwaiter().GetResult(); + _lazyHttpClient.Value.GetAsync(testAddress).ConfigureAwait(false).GetAwaiter().GetResult(); #endif Log.Info("Connection test to \"{0}\" succeeded", testAddress); } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpContentWrapper.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpContentWrapper.cs index 57f55359dc..80392fbb03 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpContentWrapper.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpContentWrapper.cs @@ -22,7 +22,7 @@ public HttpContentWrapper(HttpContent httpContent) public Stream ReadAsStream() { - return _httpContent.ReadAsStreamAsync().GetAwaiter().GetResult(); + return _httpContent.ReadAsStreamAsync().ConfigureAwait(false).GetAwaiter().GetResult(); } public IHttpContentHeadersWrapper Headers => new HttpContentHeadersWrapper(_httpContent.Headers); diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpResponse.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpResponse.cs index d4632f0c0e..1d4da062a1 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpResponse.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpResponse.cs @@ -48,7 +48,7 @@ public string GetContent() using (responseStream) using (var reader = new StreamReader(responseStream, Encoding.UTF8)) { - var responseBody = reader.ReadLineAsync().GetAwaiter().GetResult(); + var responseBody = reader.ReadLineAsync().ConfigureAwait(false).GetAwaiter().GetResult(); if (responseBody != null) { diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs index 2b821143fe..e92f15b655 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionHandler.cs @@ -17,7 +17,10 @@ using System.IO; using System.Linq; using System.Net; +using Newtonsoft.Json; +#if NETFRAMEWORK using System.Web; +#endif namespace NewRelic.Agent.Core.DataTransport { @@ -425,7 +428,7 @@ private T SendDataOverWire(ICollectorWire wire, string method, params object[ { serializedData = _serializer.Serialize(data); } - catch (Exception ex) + catch (JsonSerializationException ex) { Log.Debug("Request({0}): Exception occurred serializing request data: {1}", requestGuid, ex); // log message only since exception is rethrown throw; diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/DataStreamingService.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/DataStreamingService.cs index 975fb4fb9d..ff1ef5bfe6 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/DataStreamingService.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/DataStreamingService.cs @@ -49,7 +49,7 @@ private async Task WaitForResponse() var success = false; try { - success = await _responseStream.MoveNext(_streamCancellationToken); + success = await _responseStream.MoveNext(_streamCancellationToken).ConfigureAwait(false); } catch (RpcException rpcEx) { diff --git a/src/Agent/NewRelic/Agent/Core/Utilization/VendorHttpApiRequestor.cs b/src/Agent/NewRelic/Agent/Core/Utilization/VendorHttpApiRequestor.cs index d14f492a7c..2bf7a7607c 100644 --- a/src/Agent/NewRelic/Agent/Core/Utilization/VendorHttpApiRequestor.cs +++ b/src/Agent/NewRelic/Agent/Core/Utilization/VendorHttpApiRequestor.cs @@ -113,7 +113,7 @@ private string CallWithHttpClient(Uri uri, string method, string vendorName, IEn } } - var response = httpClient.Value.SendAsync(request).GetAwaiter().GetResult(); + var response = httpClient.Value.SendAsync(request).ConfigureAwait(false).GetAwaiter().GetResult(); if (!response.IsSuccessStatusCode) { var statusCode = response.StatusCode; @@ -122,7 +122,7 @@ private string CallWithHttpClient(Uri uri, string method, string vendorName, IEn Log.Debug("CallVendorApi ({0}) failed with WebException with status: {1}; message: {2}", vendorName, statusCode, statusDescription); } - return response.Content.ReadAsStringAsync().GetAwaiter().GetResult(); + return response.Content.ReadAsStringAsync().ConfigureAwait(false).GetAwaiter().GetResult(); } catch (Exception ex) { diff --git a/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs b/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs index b2b1d2954b..06d74d8bd7 100644 --- a/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs +++ b/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs @@ -77,7 +77,7 @@ public override void Write(byte[] buffer, int offset, int count) // Set a flag on the context to indicate we're in the middle of injecting - prevents multiple recursions when response compression is in use StartInjecting(); _agent.TryInjectBrowserScriptAsync(_context.Response.ContentType, _context.Request.Path.Value, buffer, _baseStream) - .GetAwaiter().GetResult(); + .ConfigureAwait(false).GetAwaiter().GetResult(); } finally { From 5c4429df4f99ce1f7ba2068d79313553e26f4f43 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Fri, 20 Dec 2024 16:09:55 -0600 Subject: [PATCH 12/27] Minor cleanup --- .../NewRelic/Agent/Core/Utilities/AsyncHelper.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs b/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs index 5c3c72b351..d7315a9d99 100644 --- a/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs +++ b/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs @@ -8,16 +8,15 @@ namespace NewRelic.Agent.Core.Utilities { + // Borrowed from https://stackoverflow.com/a/56928748/2078975 + [NrExcludeFromCodeCoverage] public class AsyncHelper { - private static readonly TaskFactory _taskFactory = new - TaskFactory(CancellationToken.None, - TaskCreationOptions.None, - TaskContinuationOptions.None, - TaskScheduler.Default); + private static readonly TaskFactory _taskFactory = + new(CancellationToken.None, TaskCreationOptions.None, TaskContinuationOptions.None, TaskScheduler.Default); /// - /// Safely executes an async method synchronously. + /// Safely executes an async method synchronously. /// /// /// From 9875d54d385340712b9c7f25e47466bb2e425375 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Mon, 6 Jan 2025 10:32:49 -0600 Subject: [PATCH 13/27] Remove AsyncHelper and refactor usage --- .../Core/DataTransport/Client/NRHttpClient.cs | 3 +- .../Agent/Core/Utilities/AsyncHelper.cs | 33 ------------------- 2 files changed, 1 insertion(+), 35 deletions(-) delete mode 100644 src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs index 9105b6c156..b02eeb2eff 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs @@ -9,7 +9,6 @@ using System.Reflection; using NewRelic.Agent.Configuration; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; -using NewRelic.Agent.Core.Utilities; using NewRelic.Agent.Extensions.Logging; namespace NewRelic.Agent.Core.DataTransport.Client @@ -103,7 +102,7 @@ public override IHttpResponse Send(IHttpRequest request) } Log.Finest($"Request({request.RequestGuid}: Sending"); - var response = AsyncHelper.RunSync(() => _httpClientWrapper.SendAsync(req)); + var response = _httpClientWrapper.SendAsync(req).ConfigureAwait(false).GetAwaiter().GetResult(); Log.Finest($"Request({request.RequestGuid}: Sent"); var httpResponse = new HttpResponse(request.RequestGuid, response); diff --git a/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs b/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs deleted file mode 100644 index d7315a9d99..0000000000 --- a/src/Agent/NewRelic/Agent/Core/Utilities/AsyncHelper.cs +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright 2020 New Relic, Inc. All rights reserved. -// SPDX-License-Identifier: Apache-2.0 - -#if !NETFRAMEWORK -using System; -using System.Threading; -using System.Threading.Tasks; - -namespace NewRelic.Agent.Core.Utilities -{ - // Borrowed from https://stackoverflow.com/a/56928748/2078975 - [NrExcludeFromCodeCoverage] - public class AsyncHelper - { - private static readonly TaskFactory _taskFactory = - new(CancellationToken.None, TaskCreationOptions.None, TaskContinuationOptions.None, TaskScheduler.Default); - - /// - /// Safely executes an async method synchronously. - /// - /// - /// - /// - public static TReturn RunSync(Func> task) - { - return _taskFactory.StartNew(task) - .Unwrap() - .GetAwaiter() - .GetResult(); - } - } -} -#endif From 92d70efc5bf01effd8ab9862562e971ccdb8588f Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Mon, 6 Jan 2025 13:07:26 -0600 Subject: [PATCH 14/27] Replace `lock()` with `SemaphoreSlim` in a few select places --- src/Agent/NewRelic/Agent/Core/AgentShim.cs | 11 +++-- .../Core/DataTransport/ConnectionManager.cs | 40 +++++++++++++++---- .../NewRelic/Agent/Core/Time/Scheduler.cs | 30 +++++++++++--- 3 files changed, 66 insertions(+), 15 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/AgentShim.cs b/src/Agent/NewRelic/Agent/Core/AgentShim.cs index 34c269c045..bd64aa3dc7 100644 --- a/src/Agent/NewRelic/Agent/Core/AgentShim.cs +++ b/src/Agent/NewRelic/Agent/Core/AgentShim.cs @@ -21,14 +21,15 @@ static void Initialize() } private static bool _initialized = false; - private static object _initLock = new object(); + private static SemaphoreSlim _lockSemaphore = new SemaphoreSlim(1, 1); static bool TryInitialize(string method) { - if (Monitor.IsEntered(_initLock)) return false; + if (_lockSemaphore.CurrentCount == 0) return false; if (DeferInitialization(method)) return false; - lock (_initLock) + _lockSemaphore.Wait(); + try { if (!_initialized) { @@ -38,6 +39,10 @@ static bool TryInitialize(string method) return true; } + finally + { + _lockSemaphore.Release(); + } } private static HashSet _deferInitializationOnTheseMethods = new HashSet diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs index 36983e6bed..9d58a38bcc 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs @@ -8,9 +8,10 @@ using System; using System.IO; using System.Net; +using System.Threading; + #if !NETFRAMEWORK using System.Net.Http; -using System.Threading.Tasks; #endif using System.Net.Sockets; @@ -37,7 +38,7 @@ public class ConnectionManager : ConfigurationBasedService, IConnectionManager private readonly IScheduler _scheduler; private int _connectionAttempt = 0; private bool _started; - private readonly object _syncObject = new object(); + private readonly SemaphoreSlim _lockSemaphore = new SemaphoreSlim(1, 1); private bool _runtimeConfigurationUpdated = false; public ConnectionManager(IConnectionHandler connectionHandler, IScheduler scheduler) @@ -69,7 +70,8 @@ private void Start() if (_started) return; - lock (_syncObject) + _lockSemaphore.Wait(); + try { // Second, a thread-safe check inside the blocking code block that ensures we'll never start more than once if (_started) @@ -82,17 +84,26 @@ private void Start() _started = true; } + finally + { + _lockSemaphore.Release(); + } } private void Connect() { try { - lock (_syncObject) + _lockSemaphore.Wait(); + try { _runtimeConfigurationUpdated = false; _connectionHandler.Connect(); } + finally + { + _lockSemaphore.Release(); + } // If the runtime configuration has changed, the app names have updated, so we schedule a restart // This uses the existing ScheduleRestart logic so the current Connect can finish and we follow the backoff pattern and don't spam reconnect attempts. @@ -159,29 +170,44 @@ private void HandleConnectionException(Exception ex) private void Disconnect() { - lock (_syncObject) + _lockSemaphore.Wait(); + try { _connectionHandler.Disconnect(); } + finally + { + _lockSemaphore.Release(); + } } private void Reconnect() { EventBus.Publish(new StopHarvestEvent()); - lock (_syncObject) + _lockSemaphore.Wait(); + try { Disconnect(); Connect(); } + finally + { + _lockSemaphore.Release(); + } } public T SendDataRequest(string method, params object[] data) { - lock (_syncObject) + _lockSemaphore.Wait(); + try { return _connectionHandler.SendDataRequest(method, data); } + finally + { + _lockSemaphore.Release(); + } } #endregion Synchronized methods diff --git a/src/Agent/NewRelic/Agent/Core/Time/Scheduler.cs b/src/Agent/NewRelic/Agent/Core/Time/Scheduler.cs index ce03e33044..434929feec 100644 --- a/src/Agent/NewRelic/Agent/Core/Time/Scheduler.cs +++ b/src/Agent/NewRelic/Agent/Core/Time/Scheduler.cs @@ -16,7 +16,7 @@ public class Scheduler : IScheduler, IDisposable // The System.Threading.Timer class uses -1 milliseconds as a magic "off" value private static readonly TimeSpan DisablePeriodicExecution = TimeSpan.FromMilliseconds(-1); - private readonly object _lock = new object(); + private readonly SemaphoreSlim _lockSemaphore = new SemaphoreSlim(1, 1); private readonly DisposableCollection _oneTimeTimers = new DisposableCollection(); @@ -29,7 +29,8 @@ public void ExecuteOnce(Action action, TimeSpan timeUntilExecution) if (timeUntilExecution < TimeSpan.Zero) throw new ArgumentException("Must be non-negative", "timeUntilExecution"); - lock (_lock) + _lockSemaphore.Wait(); + try { // Removes processed oneTimeTimers var timersToRemove = new List(); @@ -58,6 +59,10 @@ public void ExecuteOnce(Action action, TimeSpan timeUntilExecution) _oneTimeTimers.Add(newExecution); } + finally + { + _lockSemaphore.Release(); + } } public void ExecuteEvery(Action action, TimeSpan timeBetweenExecutions, TimeSpan? optionalInitialDelay = null) @@ -65,7 +70,8 @@ public void ExecuteEvery(Action action, TimeSpan timeBetweenExecutions, TimeSpan if (timeBetweenExecutions < TimeSpan.Zero) throw new ArgumentException("Must be non-negative", "timeBetweenExecutions"); - lock (_lock) + _lockSemaphore.Wait(); + try { var existingTimer = _recurringTimers.GetValueOrDefault(action); if (existingTimer != null) @@ -77,6 +83,10 @@ public void ExecuteEvery(Action action, TimeSpan timeBetweenExecutions, TimeSpan var timer = CreateExecuteEveryTimer(action, timeBetweenExecutions, optionalInitialDelay); _recurringTimers[action] = timer; } + finally + { + _lockSemaphore.Release(); + } } public static Timer CreateExecuteOnceTimer(Action action) @@ -169,7 +179,8 @@ public static Timer CreateExecuteEveryTimer(Action action, TimeSpan timeBetweenE public void StopExecuting(Action action, TimeSpan? timeToWaitForInProgressAction = null) { - lock (_lock) + _lockSemaphore.Wait(); + try { var existingTimer = _recurringTimers.GetValueOrDefault(action); if (existingTimer == null) @@ -190,13 +201,18 @@ public void StopExecuting(Action action, TimeSpan? timeToWaitForInProgressAction waitHandle.WaitOne(timeToWaitForInProgressAction.Value); } } + finally + { + _lockSemaphore.Release(); + } } #endregion Public API public void Dispose() { - lock (_lock) + _lockSemaphore.Wait(); + try { _oneTimeTimers.Dispose(); @@ -206,6 +222,10 @@ public void Dispose() } _recurringTimers.Clear(); } + finally + { + _lockSemaphore.Release(); + } } private class TimerStatus : IDisposable From a98119b41fd7e7aca570b51c508e5a32ba913578 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Mon, 6 Jan 2025 15:03:48 -0600 Subject: [PATCH 15/27] Locking logic refactor in ConnectionManager --- .../Core/DataTransport/ConnectionManager.cs | 37 +++++++++---------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs index 9d58a38bcc..9b3cf53ab2 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs @@ -80,7 +80,7 @@ private void Start() if (_configuration.CollectorSyncStartup || _configuration.CollectorSendDataOnExit) Connect(); else - _scheduler.ExecuteOnce(Connect, TimeSpan.Zero); + _scheduler.ExecuteOnce(LockAndConnect, TimeSpan.Zero); _started = true; } @@ -90,20 +90,25 @@ private void Start() } } + private void LockAndConnect() + { + _lockSemaphore.Wait(); + try + { + Connect(); + } + finally + { + _lockSemaphore.Release(); + } + } + private void Connect() { try { - _lockSemaphore.Wait(); - try - { - _runtimeConfigurationUpdated = false; - _connectionHandler.Connect(); - } - finally - { - _lockSemaphore.Release(); - } + _runtimeConfigurationUpdated = false; + _connectionHandler.Connect(); // If the runtime configuration has changed, the app names have updated, so we schedule a restart // This uses the existing ScheduleRestart logic so the current Connect can finish and we follow the backoff pattern and don't spam reconnect attempts. @@ -170,15 +175,7 @@ private void HandleConnectionException(Exception ex) private void Disconnect() { - _lockSemaphore.Wait(); - try - { - _connectionHandler.Disconnect(); - } - finally - { - _lockSemaphore.Release(); - } + _connectionHandler.Disconnect(); } private void Reconnect() From 8e32fd7aa72dd24e0ae5fd5303657dbc6b4aca81 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Mon, 6 Jan 2025 15:04:14 -0600 Subject: [PATCH 16/27] Comments to future self --- .../Agent/Core/DataTransport/Client/HttpClientWrapper.cs | 1 + .../NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs index a93fd19b38..8b17d983e6 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/HttpClientWrapper.cs @@ -33,6 +33,7 @@ public async Task SendAsync(HttpRequestMessage mess using var cts = new CancellationTokenSource(_timeoutMilliseconds); try { + // .ConfigureAwait(false) is used to avoid deadlocks. var httpResponseMessage = await _httpClient.SendAsync(message, cts.Token).ConfigureAwait(false); return new HttpResponseMessageWrapper(httpResponseMessage); } diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs index b02eeb2eff..02e9f7cb20 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/Client/NRHttpClient.cs @@ -102,6 +102,7 @@ public override IHttpResponse Send(IHttpRequest request) } Log.Finest($"Request({request.RequestGuid}: Sending"); + // .ConfigureAwait(false) is used to avoid deadlocks. var response = _httpClientWrapper.SendAsync(req).ConfigureAwait(false).GetAwaiter().GetResult(); Log.Finest($"Request({request.RequestGuid}: Sent"); From 0543eeabaf454fed963fb26ab041b10f0aa6b732 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Mon, 6 Jan 2025 15:04:34 -0600 Subject: [PATCH 17/27] Unit tests to check for deadlock. Also new unit tests for HttpClientWrapper. --- .../CustomAttributesWebApi/Program.cs | 2 + .../Core.UnitTest/Core.UnitTest.csproj | 1 + .../Client/HttpClientWrapperTests.cs | 81 +++++++++++++++++++ .../DataTransport/Client/NRHttpClientTests.cs | 35 +++++++- 4 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/HttpClientWrapperTests.cs diff --git a/tests/Agent/IntegrationTests/Applications/CustomAttributesWebApi/Program.cs b/tests/Agent/IntegrationTests/Applications/CustomAttributesWebApi/Program.cs index fe356913bc..5973374ff4 100644 --- a/tests/Agent/IntegrationTests/Applications/CustomAttributesWebApi/Program.cs +++ b/tests/Agent/IntegrationTests/Applications/CustomAttributesWebApi/Program.cs @@ -19,6 +19,8 @@ public class Program static void Main(string[] args) { + Debugger.Launch(); + if (Parser.Default == null) throw new NullReferenceException("CommandLine.Parser.Default"); diff --git a/tests/Agent/UnitTests/Core.UnitTest/Core.UnitTest.csproj b/tests/Agent/UnitTests/Core.UnitTest/Core.UnitTest.csproj index d25977308c..e4b66a749a 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/Core.UnitTest.csproj +++ b/tests/Agent/UnitTests/Core.UnitTest/Core.UnitTest.csproj @@ -15,6 +15,7 @@ + all diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/HttpClientWrapperTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/HttpClientWrapperTests.cs new file mode 100644 index 0000000000..57d21716b4 --- /dev/null +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/HttpClientWrapperTests.cs @@ -0,0 +1,81 @@ +// Copyright 2020 New Relic, Inc. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +#if !NETFRAMEWORK +using System.Net; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; +using Telerik.JustMock; + + +namespace NewRelic.Agent.Core.DataTransport.Client +{ + [TestFixture] + public class HttpClientWrapperTests + { + private HttpClientWrapper _httpClientWrapper; + private HttpClient _mockHttpClient; + private int _timeoutMilliseconds = 1000; + + [SetUp] + public void SetUp() + { + _mockHttpClient = Mock.Create(); + _httpClientWrapper = new HttpClientWrapper(_mockHttpClient, _timeoutMilliseconds); + } + + [TearDown] + public void TearDown() + { + _httpClientWrapper.Dispose(); + _mockHttpClient.Dispose(); + } + + [Test] + public async Task SendAsync_ShouldReturnHttpResponseMessageWrapper_OnSuccess() + { + // Arrange + var requestMessage = new HttpRequestMessage(HttpMethod.Get, "http://example.com"); + var responseMessage = new HttpResponseMessage(HttpStatusCode.OK); + Mock.Arrange(() => _mockHttpClient.SendAsync(Arg.IsAny(), Arg.IsAny())) + .Returns(Task.FromResult(responseMessage)); + + // Act + var result = await _httpClientWrapper.SendAsync(requestMessage); + + // Assert + Assert.That(result.IsSuccessStatusCode, Is.True); + } + + [Test] + public void SendAsync_ShouldThrowException_OnFailure() + { + // Arrange + var requestMessage = new HttpRequestMessage(HttpMethod.Get, "http://example.com"); + Mock.Arrange(() => _mockHttpClient.SendAsync(Arg.IsAny(), Arg.IsAny())) + .Throws(new HttpRequestException("Request failed")); + + // Act & Assert + Assert.ThrowsAsync(async () => await _httpClientWrapper.SendAsync(requestMessage)); + } + + [Test] + public void SendAsync_ShouldThrowException_OnTimeout() + { + // Arrange + var requestMessage = new HttpRequestMessage(HttpMethod.Get, "http://example.com"); + Mock.Arrange(() => _mockHttpClient.SendAsync(Arg.IsAny(), Arg.IsAny())) + .Returns(async (HttpRequestMessage msg, CancellationToken token) => + { + await Task.Delay(_timeoutMilliseconds + 1000, token); + return new HttpResponseMessage(HttpStatusCode.RequestTimeout); + }); + + // Act & Assert + Assert.ThrowsAsync(async () => await _httpClientWrapper.SendAsync(requestMessage)); + } + } +} +#endif diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs index f2c46d94a9..e5d7ce7455 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs @@ -6,12 +6,12 @@ using System.Linq; using System.Net; using System.Net.Http; -using System.Threading.Tasks; using NewRelic.Agent.Configuration; using NewRelic.Agent.Core.DataTransport.Client.Interfaces; using NUnit.Framework; using Telerik.JustMock; -using Telerik.JustMock.Helpers; +using Nito.AsyncEx; +using System.Threading; namespace NewRelic.Agent.Core.DataTransport.Client { @@ -141,6 +141,37 @@ public void Dispose_DisposesHttpClientWrapper() Mock.Assert(() => _mockHttpClientWrapper.Dispose(), Occurs.Once()); } + [Test] + public void Send_Does_Not_Deadlock() + { + using var client = new NRHttpClient(_mockProxy, _mockConfiguration); + + client.SetHttpClientWrapper(new HttpClientWrapper(new HttpClient(), 500)); + + var request = new HttpRequest(_mockConfiguration) + { + Endpoint = "connect", + ConnectionInfo = _mockConnectionInfo, + RequestGuid = Guid.NewGuid(), + Content = { SerializedData = "{\"Test\"}", ContentType = "application/json" } + }; + + // start a thread that might deadlock - if join times out, we know it deadlocked + var thread = new Thread(() => SendWithAsyncContext(client, request)); + thread.Start(); + if (!thread.Join(5000)) { + Assert.Fail("Deadlock detected. Check logic in NRHttpClient.Send() and HttpClientWrapper.Send() for correct usage of .ConfigureAwait(false)"); + } + } + + private static void SendWithAsyncContext(NRHttpClient client, HttpRequest request) + { + AsyncContext.Run(() => + { + var response = client.Send(request); + }); + } + private IHttpRequest CreateHttpRequest() { var request = new HttpRequest(_mockConfiguration) From 549543e7f4397181749ec7722f033855c1db2901 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Mon, 6 Jan 2025 15:41:04 -0600 Subject: [PATCH 18/27] Cleanup, comments, fixed a couple of spots where I didn't handle the locking refactor correctly. --- .../Core/DataTransport/ConnectionManager.cs | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs index 9b3cf53ab2..11f0b1e2e8 100644 --- a/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs +++ b/src/Agent/NewRelic/Agent/Core/DataTransport/ConnectionManager.cs @@ -103,6 +103,7 @@ private void LockAndConnect() } } + // This method does not acquire the semaphore. Be certain it is only called from within a semaphore block. private void Connect() { try @@ -173,11 +174,26 @@ private void HandleConnectionException(Exception ex) ImmediateShutdown(); } + // This method does not acquire the semaphore. Be certain it is only called from within a semaphore block. private void Disconnect() { _connectionHandler.Disconnect(); } + private void LockAndDisconnect() + { + _lockSemaphore.Wait(); + try + { + Disconnect(); + } + finally + { + _lockSemaphore.Release(); + } + } + + private void Reconnect() { EventBus.Publish(new StopHarvestEvent()); @@ -221,7 +237,7 @@ private void ScheduleRestart() { var retryTime = ConnectionRetryBackoffSequence[_connectionAttempt]; Log.Info("Will attempt to reconnect in {0} seconds", retryTime.TotalSeconds); - _scheduler.ExecuteOnce(Connect, retryTime); + _scheduler.ExecuteOnce(LockAndConnect, retryTime); _connectionAttempt = Math.Min(_connectionAttempt + 1, ConnectionRetryBackoffSequence.Length - 1); } @@ -271,9 +287,8 @@ private void OnRestartAgent(RestartAgentEvent eventData) private void OnCleanShutdown(CleanShutdownEvent eventData) { - Disconnect(); + LockAndDisconnect(); } - #endregion Event handlers } } From d9328f7f248ee55fb16c9b7a53e37bcac072ce74 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Mon, 6 Jan 2025 15:41:12 -0600 Subject: [PATCH 19/27] Oops --- .../Applications/CustomAttributesWebApi/Program.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/Agent/IntegrationTests/Applications/CustomAttributesWebApi/Program.cs b/tests/Agent/IntegrationTests/Applications/CustomAttributesWebApi/Program.cs index 5973374ff4..fe356913bc 100644 --- a/tests/Agent/IntegrationTests/Applications/CustomAttributesWebApi/Program.cs +++ b/tests/Agent/IntegrationTests/Applications/CustomAttributesWebApi/Program.cs @@ -19,8 +19,6 @@ public class Program static void Main(string[] args) { - Debugger.Launch(); - if (Parser.Default == null) throw new NullReferenceException("CommandLine.Parser.Default"); From a07edab393cf6eb43e41108a3f4125229680133b Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Tue, 7 Jan 2025 10:16:11 -0600 Subject: [PATCH 20/27] test: Fix unit tests broken after NUnit update --- .../Core.UnitTest/DistributedTracing/TracingStateTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Agent/UnitTests/Core.UnitTest/DistributedTracing/TracingStateTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DistributedTracing/TracingStateTests.cs index 8ff4072723..04fde3c5c8 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DistributedTracing/TracingStateTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DistributedTracing/TracingStateTests.cs @@ -64,7 +64,7 @@ public void AcceptDistributedTraceHeadersHydratesValidNewRelicPayload(string hea Assert.That(tracingState.Priority, Is.EqualTo(Priority)); Assert.That(tracingState.Sampled, Is.EqualTo(Sampled)); Assert.That(tracingState.TransactionId, Is.EqualTo(TransactionId)); - Assert.That(tracingState.Timestamp, Is.Not.EqualTo(default), $"Timestamp should not be {(DateTime)default}"); + Assert.That(tracingState.Timestamp, Is.Not.EqualTo(default(DateTime)), $"Timestamp should not be {(DateTime)default}"); Assert.That(tracingState.TransportDuration, Is.GreaterThan(TimeSpan.Zero), $"TransportDuration should not be Zero"); }); } @@ -222,7 +222,7 @@ public void AcceptDistributedTraceHeadersHydratesValidW3CTraceContext() Assert.That(tracingState.Sampled, Is.EqualTo(Sampled)); Assert.That(tracingState.TransactionId, Is.EqualTo(TransactionId)); Assert.That(tracingState.ParentId, Is.EqualTo(ParentId)); - Assert.That(tracingState.Timestamp, Is.Not.EqualTo(default), $"Timestamp should not be {(DateTime)default}"); + Assert.That(tracingState.Timestamp, Is.Not.EqualTo(default(DateTime)), $"Timestamp should not be {(DateTime)default}"); Assert.That(tracingState.TransportDuration, Is.GreaterThan(TimeSpan.Zero), $"TransportDuration should not be Zero"); }); } From 8a245725f31fb0f09bb6513f223fe7c406cc1d77 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Tue, 7 Jan 2025 10:22:53 -0600 Subject: [PATCH 21/27] Consistency --- .../Agent/Core/DistributedTracing/TracingState.cs | 2 +- .../DistributedTracing/TracingStateTests.cs | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/DistributedTracing/TracingState.cs b/src/Agent/NewRelic/Agent/Core/DistributedTracing/TracingState.cs index a851a55bc7..c101474a70 100644 --- a/src/Agent/NewRelic/Agent/Core/DistributedTracing/TracingState.cs +++ b/src/Agent/NewRelic/Agent/Core/DistributedTracing/TracingState.cs @@ -109,7 +109,7 @@ public DateTime Timestamp return _timestamp = _newRelicPayload.Timestamp; } - return _timestamp = (DateTime)default; // default is same as new. + return _timestamp = default(DateTime); // default is same as new. } } diff --git a/tests/Agent/UnitTests/Core.UnitTest/DistributedTracing/TracingStateTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DistributedTracing/TracingStateTests.cs index 04fde3c5c8..bd9b45c31d 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DistributedTracing/TracingStateTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DistributedTracing/TracingStateTests.cs @@ -64,7 +64,7 @@ public void AcceptDistributedTraceHeadersHydratesValidNewRelicPayload(string hea Assert.That(tracingState.Priority, Is.EqualTo(Priority)); Assert.That(tracingState.Sampled, Is.EqualTo(Sampled)); Assert.That(tracingState.TransactionId, Is.EqualTo(TransactionId)); - Assert.That(tracingState.Timestamp, Is.Not.EqualTo(default(DateTime)), $"Timestamp should not be {(DateTime)default}"); + Assert.That(tracingState.Timestamp, Is.Not.EqualTo(default(DateTime)), $"Timestamp should not be {default(DateTime)}"); Assert.That(tracingState.TransportDuration, Is.GreaterThan(TimeSpan.Zero), $"TransportDuration should not be Zero"); }); } @@ -92,7 +92,7 @@ public void AcceptDistributedTraceHeadersPopulatesErrorsIfNull(string headerName Assert.That(tracingState.TransactionId, Is.Null); Assert.That(tracingState.Sampled, Is.Null); Assert.That(tracingState.Priority, Is.Null); - Assert.That(tracingState.Timestamp, Is.EqualTo((DateTime)default), $"Timestamp: expected {(DateTime)default}, actual: {tracingState.Timestamp}"); + Assert.That(tracingState.Timestamp, Is.EqualTo(default(DateTime)), $"Timestamp: expected {default(DateTime)}, actual: {tracingState.Timestamp}"); Assert.That(tracingState.TransportDuration, Is.EqualTo(TimeSpan.Zero), $"TransportDuration: expected {TimeSpan.Zero}, actual: {tracingState.TransportDuration}"); Assert.That(tracingState.IngestErrors, Does.Contain(IngestErrorType.NullPayload), "TracingState IngestErrors should contain NullPayload"); @@ -124,7 +124,7 @@ public void AcceptDistributedTraceHeadersPopulatesErrorsIfUnsupportedVersion(str Assert.That(tracingState.TransactionId, Is.Null); Assert.That(tracingState.Sampled, Is.Null); Assert.That(tracingState.Priority, Is.Null); - Assert.That(tracingState.Timestamp, Is.EqualTo((DateTime)default), $"Timestamp: expected {(DateTime)default}, actual: {tracingState.Timestamp}"); + Assert.That(tracingState.Timestamp, Is.EqualTo(default(DateTime)), $"Timestamp: expected {default(DateTime)}, actual: {tracingState.Timestamp}"); Assert.That(tracingState.TransportDuration, Is.EqualTo(TimeSpan.Zero), $"TransportDuration: expected {TimeSpan.Zero}, actual: {tracingState.TransportDuration}"); Assert.That(tracingState.IngestErrors, Does.Contain(IngestErrorType.Version), "TracingState IngestErrors should contain Version error."); @@ -156,7 +156,7 @@ public void AcceptDistributedTraceHeadersPopulatesErrorsIfInvalidTimestamp(strin Assert.That(tracingState.TransactionId, Is.Null); Assert.That(tracingState.Sampled, Is.Null); Assert.That(tracingState.Priority, Is.Null); - Assert.That(tracingState.Timestamp, Is.EqualTo((DateTime)default), $"Timestamp: expected {(DateTime)default}, actual: {tracingState.Timestamp}"); + Assert.That(tracingState.Timestamp, Is.EqualTo(default(DateTime)), $"Timestamp: expected {default(DateTime)}, actual: {tracingState.Timestamp}"); Assert.That(tracingState.TransportDuration, Is.EqualTo(TimeSpan.Zero), $"TransportDuration: expected {TimeSpan.Zero}, actual: {tracingState.TransportDuration}"); Assert.That(tracingState.IngestErrors, Does.Contain(IngestErrorType.ParseException), "TracingState IngestErrors should contain ParseException."); @@ -188,7 +188,7 @@ public void AcceptDistributedTraceHeadersPopulatesErrorsIfNotTraceable(string he Assert.That(tracingState.TransactionId, Is.Null); Assert.That(tracingState.Sampled, Is.Null); Assert.That(tracingState.Priority, Is.Null); - Assert.That(tracingState.Timestamp, Is.EqualTo((DateTime)default), $"Timestamp: expected {(DateTime)default}, actual: {tracingState.Timestamp}"); + Assert.That(tracingState.Timestamp, Is.EqualTo(default(DateTime)), $"Timestamp: expected {default(DateTime)}, actual: {tracingState.Timestamp}"); Assert.That(tracingState.TransportDuration, Is.EqualTo(TimeSpan.Zero), $"TransportDuration: expected {TimeSpan.Zero}, actual: {tracingState.TransportDuration}"); Assert.That(tracingState.IngestErrors, Does.Contain(IngestErrorType.ParseException), "TracingState IngestErrors should contain ParseException."); @@ -222,7 +222,7 @@ public void AcceptDistributedTraceHeadersHydratesValidW3CTraceContext() Assert.That(tracingState.Sampled, Is.EqualTo(Sampled)); Assert.That(tracingState.TransactionId, Is.EqualTo(TransactionId)); Assert.That(tracingState.ParentId, Is.EqualTo(ParentId)); - Assert.That(tracingState.Timestamp, Is.Not.EqualTo(default(DateTime)), $"Timestamp should not be {(DateTime)default}"); + Assert.That(tracingState.Timestamp, Is.Not.EqualTo(default(DateTime)), $"Timestamp should not be {default(DateTime)}"); Assert.That(tracingState.TransportDuration, Is.GreaterThan(TimeSpan.Zero), $"TransportDuration should not be Zero"); }); } @@ -251,7 +251,7 @@ public void AcceptDistributedTraceHeadersHydratesValidW3CTraceContext_WithoutAVa Assert.That(tracingState.TraceId, Is.EqualTo(TraceId)); Assert.That(tracingState.ParentId, Is.EqualTo(ParentId)); Assert.That(tracingState.Type, Is.EqualTo(DistributedTracingParentType.Unknown)); - Assert.That(tracingState.Timestamp, Is.EqualTo((DateTime)default), $"Timestamp: expected {(DateTime)default}, actual: {tracingState.Timestamp}"); + Assert.That(tracingState.Timestamp, Is.EqualTo(default(DateTime)), $"Timestamp: expected {default(DateTime)}, actual: {tracingState.Timestamp}"); Assert.That(tracingState.TransportDuration, Is.EqualTo(TimeSpan.Zero), $"TransportDuration: expected {TimeSpan.Zero}, actual: {tracingState.TransportDuration}"); }); } From a8b6876da6b72e5b3ff7803bfe3513cf5dffdee9 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Tue, 7 Jan 2025 10:54:20 -0600 Subject: [PATCH 22/27] fix deadlock test --- .../DataTransport/Client/NRHttpClientTests.cs | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs index e5d7ce7455..c662ac0abd 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs @@ -12,6 +12,7 @@ using Telerik.JustMock; using Nito.AsyncEx; using System.Threading; +using System.Threading.Tasks; namespace NewRelic.Agent.Core.DataTransport.Client { @@ -156,19 +157,28 @@ public void Send_Does_Not_Deadlock() Content = { SerializedData = "{\"Test\"}", ContentType = "application/json" } }; - // start a thread that might deadlock - if join times out, we know it deadlocked - var thread = new Thread(() => SendWithAsyncContext(client, request)); - thread.Start(); - if (!thread.Join(5000)) { - Assert.Fail("Deadlock detected. Check logic in NRHttpClient.Send() and HttpClientWrapper.Send() for correct usage of .ConfigureAwait(false)"); - } + Assert.DoesNotThrow(() => + { + // start a thread that might deadlock - if the thread doesn't throw TaskCanceledException within 5 seconds, then we have a deadlock + var thread = new Thread(() => SendWithAsyncContext(client, request)); + thread.Start(); + if (!thread.Join(5000)) + throw new Exception("Deadlock detected."); + }, "Deadlock detected. Check logic in NRHttpClient.Send() and HttpClientWrapper.Send() for correct usage of .ConfigureAwait(false)"); } private static void SendWithAsyncContext(NRHttpClient client, HttpRequest request) { AsyncContext.Run(() => { + try + { var response = client.Send(request); + } + catch (TaskCanceledException) + { + // expected exception + } }); } From fb399157e3ac076f409f507874bfa8c0eab620bb Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Tue, 7 Jan 2025 10:55:00 -0600 Subject: [PATCH 23/27] Grr. formatting. --- .../Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs index c662ac0abd..01669f63d9 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs @@ -172,8 +172,8 @@ private static void SendWithAsyncContext(NRHttpClient client, HttpRequest reques AsyncContext.Run(() => { try - { - var response = client.Send(request); + { + var response = client.Send(request); } catch (TaskCanceledException) { From adfa1b5f7dba00ee0e673830dba0c31f5599ca47 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Tue, 7 Jan 2025 10:55:29 -0600 Subject: [PATCH 24/27] comment --- .../Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs index 01669f63d9..be0232d0b7 100644 --- a/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs +++ b/tests/Agent/UnitTests/Core.UnitTest/DataTransport/Client/NRHttpClientTests.cs @@ -159,7 +159,7 @@ public void Send_Does_Not_Deadlock() Assert.DoesNotThrow(() => { - // start a thread that might deadlock - if the thread doesn't throw TaskCanceledException within 5 seconds, then we have a deadlock + // start a thread that might deadlock - if the thread doesn't complete within 5 seconds, then we have a deadlock var thread = new Thread(() => SendWithAsyncContext(client, request)); thread.Start(); if (!thread.Join(5000)) From a04477a15262ca1f07dffbe0ae197396bc6afebd Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Tue, 7 Jan 2025 13:05:12 -0600 Subject: [PATCH 25/27] Rolled back a couple of changes --- .../Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs | 6 +++--- .../AspNetCore6Plus/BrowserInjectingStreamWrapper.cs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs b/src/Agent/NewRelic/Agent/Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs index 6257da2b4b..7b76a3c841 100644 --- a/src/Agent/NewRelic/Agent/Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs +++ b/src/Agent/NewRelic/Agent/Core/BrowserMonitoring/BrowserScriptInjectionHelper.cs @@ -25,7 +25,7 @@ public static async Task InjectBrowserScriptAsync(byte[] buffer, Stream baseStre { // not found, can't inject anything transaction?.LogFinest("Skipping RUM Injection: No suitable location found to inject script."); - await baseStream.WriteAsync(buffer, 0, buffer.Length).ConfigureAwait(false); + await baseStream.WriteAsync(buffer, 0, buffer.Length); return; } @@ -34,13 +34,13 @@ public static async Task InjectBrowserScriptAsync(byte[] buffer, Stream baseStre if (index < buffer.Length) // validate index is less than buffer length { // Write everything up to the insertion index - await baseStream.WriteAsync(buffer, 0, index).ConfigureAwait(false); + await baseStream.WriteAsync(buffer, 0, index); // Write the RUM script await baseStream.WriteAsync(rumBytes, 0, rumBytes.Length); // Write the rest of the doc, starting after the insertion index - await baseStream.WriteAsync(buffer, index, buffer.Length - index).ConfigureAwait(false); + await baseStream.WriteAsync(buffer, index, buffer.Length - index); } else transaction?.LogFinest($"Skipping RUM Injection: Insertion index was invalid."); diff --git a/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs b/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs index 06d74d8bd7..d0ecc59d0b 100644 --- a/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs +++ b/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs @@ -77,7 +77,7 @@ public override void Write(byte[] buffer, int offset, int count) // Set a flag on the context to indicate we're in the middle of injecting - prevents multiple recursions when response compression is in use StartInjecting(); _agent.TryInjectBrowserScriptAsync(_context.Response.ContentType, _context.Request.Path.Value, buffer, _baseStream) - .ConfigureAwait(false).GetAwaiter().GetResult(); + .GetAwaiter().GetResult(); } finally { From 53c045795bf0b91cf080296fb74514dfe3d17199 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Tue, 7 Jan 2025 13:06:34 -0600 Subject: [PATCH 26/27] formatting --- .../Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs b/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs index d0ecc59d0b..b2b1d2954b 100644 --- a/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs +++ b/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AspNetCore6Plus/BrowserInjectingStreamWrapper.cs @@ -77,7 +77,7 @@ public override void Write(byte[] buffer, int offset, int count) // Set a flag on the context to indicate we're in the middle of injecting - prevents multiple recursions when response compression is in use StartInjecting(); _agent.TryInjectBrowserScriptAsync(_context.Response.ContentType, _context.Request.Path.Value, buffer, _baseStream) - .GetAwaiter().GetResult(); + .GetAwaiter().GetResult(); } finally { From 2f67e2f88c63484c93762e99970ba420c5946356 Mon Sep 17 00:00:00 2001 From: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com> Date: Tue, 7 Jan 2025 13:14:20 -0600 Subject: [PATCH 27/27] More reverted changes --- src/Agent/NewRelic/Agent/Core/Agent.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Agent/NewRelic/Agent/Core/Agent.cs b/src/Agent/NewRelic/Agent/Core/Agent.cs index 5da8235af1..920906c93d 100644 --- a/src/Agent/NewRelic/Agent/Core/Agent.cs +++ b/src/Agent/NewRelic/Agent/Core/Agent.cs @@ -325,10 +325,10 @@ public async Task TryInjectBrowserScriptAsync(string contentType, string request if (rumBytes == null) { transaction.LogFinest("Skipping RUM Injection: No script was available."); - await baseStream.WriteAsync(buffer, 0, buffer.Length).ConfigureAwait(false); + await baseStream.WriteAsync(buffer, 0, buffer.Length); } else - await BrowserScriptInjectionHelper.InjectBrowserScriptAsync(buffer, baseStream, rumBytes, transaction).ConfigureAwait(false); + await BrowserScriptInjectionHelper.InjectBrowserScriptAsync(buffer, baseStream, rumBytes, transaction); } private string TryGetRUMScriptInternal(string contentType, string requestPath)