fix: Refactor StackExchange.Redis v2+ instrumentation to eliminate potential memory leaks.

Author: jaffinito

src/Agent/NewRelic/Agent/Core/Agent.cs

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 using NewRelic.Agent.Api;
+using NewRelic.Agent.Api.Experimental;
 using NewRelic.Agent.Configuration;
 using NewRelic.Agent.Core.AgentHealth;
 using NewRelic.Agent.Core.Aggregators;
@@ -64,6 +65,7 @@ public class Agent : IAgent // any changes to api, update the interface in exten
         private readonly ILogContextDataFilter _logContextDataFilter;
         private Extensions.Logging.ILogger _logger;
         private volatile IStackExchangeRedisCache _stackExchangeRedisCache;
+        private readonly ISimpleSchedulingService _simpleSchedulingService;
 
         public Agent(ITransactionService transactionService, ITransactionTransformer transactionTransformer,
             IThreadPoolStatic threadPoolStatic, ITransactionMetricNameMaker transactionMetricNameMaker, IPathHashMaker pathHashMaker,
@@ -72,7 +74,7 @@ public Agent(ITransactionService transactionService, ITransactionTransformer tra
             IBrowserMonitoringPrereqChecker browserMonitoringPrereqChecker, IBrowserMonitoringScriptMaker browserMonitoringScriptMaker,
             IConfigurationService configurationService, IAgentHealthReporter agentHealthReporter, IAgentTimerService agentTimerService,
             IMetricNameService metricNameService, Api.ITraceMetadataFactory traceMetadataFactory, ICATSupportabilityMetricCounters catMetricCounters,
-            ILogEventAggregator logEventAggregator, ILogContextDataFilter logContextDataFilter)
+            ILogEventAggregator logEventAggregator, ILogContextDataFilter logContextDataFilter, ISimpleSchedulingService simpleSchedulingService)
         {
             _transactionService = transactionService;
             _transactionTransformer = transactionTransformer;
@@ -93,6 +95,7 @@ public Agent(ITransactionService transactionService, ITransactionTransformer tra
             _catMetricCounters = catMetricCounters;
             _logEventAggregator = logEventAggregator;
             _logContextDataFilter = logContextDataFilter;
+            _simpleSchedulingService = simpleSchedulingService;
 
             Instance = this;
         }
@@ -389,6 +392,11 @@ public Dictionary<string, string> GetLinkingMetadata()
 
         #region ExperimentalApi
 
+        public ISimpleSchedulingService SimpleSchedulingService
+        {
+            get { return _simpleSchedulingService; }
+        }
+
         public IStackExchangeRedisCache StackExchangeRedisCache
         {
             get { return _stackExchangeRedisCache; }

src/Agent/NewRelic/Agent/Core/Aggregators/LogEventAggregator.cs

@@ -58,6 +58,11 @@ public override void Collect(LogEventWireModel loggingEventWireModel)
 
         public void CollectWithPriority(IList<LogEventWireModel> logEventWireModels, float priority)
         {
+            if (logEventWireModels == null)
+            {
+                return;
+            }
+
             for (int i = 0; i < logEventWireModels.Count; i++)
             {
                 _agentHealthReporter.ReportLoggingEventCollected();

src/Agent/NewRelic/Agent/Core/Configuration/DefaultConfiguration.cs

@@ -2208,6 +2208,30 @@ public TimeSpan UpdateLoadedModulesCycle
             }
         }
 
+        private TimeSpan? _stackExchangeRedisCleanupCycleOverride = null;
+        public TimeSpan StackExchangeRedisCleanupCycle
+        {
+            get
+            {
+                if (_stackExchangeRedisCleanupCycleOverride.HasValue)
+                {
+                    return _stackExchangeRedisCleanupCycleOverride.Value;
+                }
+
+                if (_newRelicAppSettings.TryGetValue("OverrideStackExchangeRedisCleanupCycle", out var harvestCycle))
+                {
+                    if (int.TryParse(harvestCycle, out var parsedHarvestCycle) && parsedHarvestCycle > 0)
+                    {
+                        Log.Info("StackExchange.Redis cleanup cycle overridden to " + parsedHarvestCycle + " seconds.");
+                        _stackExchangeRedisCleanupCycleOverride = TimeSpan.FromSeconds(parsedHarvestCycle);
+                        return _stackExchangeRedisCleanupCycleOverride.Value;
+                    }
+                }
+
+                return DefaultHarvestCycle;
+            }
+        }
+
         #endregion
 
         #region Helpers

src/Agent/NewRelic/Agent/Core/Configuration/ReportedConfiguration.cs

@@ -641,6 +641,9 @@ public ReportedConfiguration(IConfiguration configuration)
         [JsonProperty("update_loaded_modules.cycle")]
         public TimeSpan UpdateLoadedModulesCycle => _configuration.UpdateLoadedModulesCycle;
 
+        [JsonProperty("stackexchangeredis_cleanup.cycle")]
+        public TimeSpan StackExchangeRedisCleanupCycle => _configuration.StackExchangeRedisCleanupCycle;
+
         public IReadOnlyDictionary<string, string> GetAppSettings()
         {
             return _configuration.GetAppSettings();

src/Agent/NewRelic/Agent/Core/DependencyInjection/AgentServices.cs

@@ -203,6 +203,7 @@ public static void RegisterServices(IContainer container)
                 container.Register<INativeMethods, LinuxNativeMethods>();
             }
             container.Register<NewRelicCore.DistributedTracing.ITracePriorityManager, NewRelicCore.DistributedTracing.TracePriorityManager>();
+            container.Register<NewRelic.Agent.Api.Experimental.ISimpleSchedulingService, SimpleSchedulingService>();
 
             container.Register<UpdatedLoadedModulesService, UpdatedLoadedModulesService>();
 

src/Agent/NewRelic/Agent/Core/Segments/NoOpSegment.cs

@@ -18,6 +18,7 @@ public class NoOpSegment : ISegment, ISegmentExperimental, ISegmentDataState
 
         private readonly IAttributeDefinitions _attribDefs = new AttributeDefinitions(new AttributeFilter(new AttributeFilter.Settings()));
 
+        public bool IsDone => true; // the segment is technically done since it is does nothing.
         public bool IsValid => false;
         public bool DurationShouldBeDeductedFromParent { get; set; } = false;
         public bool AlwaysDeductChildDuration { private get; set; } = false;
@@ -64,5 +65,10 @@ public ISpan SetName(string name)
         {
             return this;
         }
+
+        public string GetCategory()
+        {
+            return string.Empty;
+        }
     }
 }

src/Agent/NewRelic/Agent/Core/Segments/Segment.cs

@@ -17,6 +17,7 @@
 using System.Diagnostics;
 using NewRelic.Agent.Core.Configuration;
 using NewRelic.Agent.Core.Utils;
+using NewRelic.Agent.Extensions.Providers.Wrapper;
 
 namespace NewRelic.Agent.Core.Segments
 {
@@ -106,7 +107,13 @@ public Segment(TimeSpan relativeStartTime, TimeSpan? duration, Segment segment,
             SpanId = segment.SpanId;
         }
 
+        public bool IsDone
+        {
+            get { return RelativeEndTime.HasValue; }
+        }
+
         public bool IsValid => true;
+
         public bool DurationShouldBeDeductedFromParent { get; set; } = false;
 
         public bool AlwaysDeductChildDuration { private get; set; } = false;
@@ -138,14 +145,11 @@ public void End()
             // this segment may have already been forced to end
             if (RelativeEndTime.HasValue == false)
             {
+                // This order is to ensure the segment end time is correct, but also not mark the segment as IsDone so that CleanUp ignores it.
                 var endTime = _transactionSegmentState.GetRelativeTime();
+                Agent.Instance.StackExchangeRedisCache?.Harvest(this);
                 RelativeEndTime = endTime;
 
-                if (Agent.Instance.StackExchangeRedisCache != null)
-                {
-                    Agent.Instance.StackExchangeRedisCache.Harvest(this);
-                }
-
                 Finish();
 
                 _transactionSegmentState.CallStackPop(this, true);
@@ -432,5 +436,10 @@ public ISpan SetName(string name)
             SegmentNameOverride = name;
             return this;
         }
+
+        public string GetCategory()
+        {
+            return EnumNameCache<SpanCategory>.GetName(Data.SpanCategory);
+        }
     }
 }

src/Agent/NewRelic/Agent/Core/Time/SimpleSchedulingService.cs

@@ -0,0 +1,46 @@
+// Copyright 2020 New Relic, Inc. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+using System;
+using System.Collections.Generic;
+using NewRelic.Agent.Api.Experimental;
+using NewRelic.Agent.Core.Utilities;
+
+namespace NewRelic.Agent.Core.Time
+{
+    public class SimpleSchedulingService : DisposableService, ISimpleSchedulingService
+    {
+        private readonly IScheduler _scheduler;
+        private readonly List<Action> _executingActions;
+
+        public SimpleSchedulingService(IScheduler scheduler)
+        {
+            _scheduler = scheduler;
+            _executingActions = new List<Action>();
+        }
+
+        public void StartExecuteEvery(Action action, TimeSpan timeBetweenExecutions, TimeSpan? optionalInitialDelay = null)
+        {
+            _scheduler.ExecuteEvery(action, timeBetweenExecutions, optionalInitialDelay);
+            _executingActions.Add(action);
+        }
+
+        public void StopExecuting(Action action)
+        {
+            _scheduler.StopExecuting(action);
+            _executingActions.Remove(action);
+        }
+
+        public override void Dispose()
+        {
+            foreach (var executingAction in _executingActions)
+            {
+                _scheduler.StopExecuting(executingAction);
+            }
+
+            _executingActions.Clear();
+
+            base.Dispose();
+        }
+    }
+}

src/Agent/NewRelic/Agent/Extensions/NewRelic.Agent.Extensions/Api/Experimental/IAgentExperimental.cs

@@ -33,5 +33,7 @@ public interface IAgentExperimental
         void RecordLogMessage(string frameworkName, object logEvent, Func<object,DateTime> getTimestamp, Func<object,object> getLogLevel, Func<object,string> getLogMessage, Func<object, Exception> getLogException, Func<object, Dictionary<string, object>> getContextData, string spanId, string traceId);
 
         Extensions.Helpers.IStackExchangeRedisCache StackExchangeRedisCache { get; set; }
+
+        ISimpleSchedulingService SimpleSchedulingService { get; }
     }
 }

src/Agent/NewRelic/Agent/Extensions/NewRelic.Agent.Extensions/Api/Experimental/ISegmentExperimental.cs

@@ -44,5 +44,15 @@ public interface ISegmentExperimental
         /// </summary>
         string UserCodeNamespace { get; set; }
 
+        /// <summary>
+        /// Returns the category of the segment.
+        /// </summary>
+        /// <returns>Category of the segment.</returns>
+        string GetCategory();
+
+        /// <summary>
+        /// Will be true if a relative end time has been set on the segment.  In most situations, this is only set when a segment is ended.
+        /// </summary>
+        bool IsDone { get; }
     }
 }

src/Agent/NewRelic/Agent/Extensions/NewRelic.Agent.Extensions/Api/Experimental/ISimpleSchedulingService.cs

@@ -0,0 +1,14 @@
+// Copyright 2020 New Relic, Inc. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+using System;
+
+namespace NewRelic.Agent.Api.Experimental
+{
+    public interface ISimpleSchedulingService
+    {
+        void StartExecuteEvery(Action action, TimeSpan timeBetweenExecutions, TimeSpan? optionalInitialDelay = null);
+
+        void StopExecuting(Action action);
+    }
+}

src/Agent/NewRelic/Agent/Extensions/NewRelic.Agent.Extensions/Configuration/IConfiguration.cs

@@ -203,5 +203,6 @@ public interface IConfiguration
         TimeSpan DefaultHarvestCycle { get; }
         TimeSpan SqlTracesHarvestCycle { get; }
         TimeSpan UpdateLoadedModulesCycle { get; }
+        TimeSpan StackExchangeRedisCleanupCycle { get; }
     }
 }