fix: Nested logging contexts should not disable context data for Microsoft.Extensions.Logging (2508) (#2516)

Author: tippmar-nr

src/Agent/NewRelic/Agent/Core/Logging/Logger.cs

@@ -34,24 +34,47 @@ public bool IsEnabledFor(Level level)
         public void Log(Level level, string message)
         {
             if (!IsEnabledFor(level)) return;
-            var messageString = message.ToString();
 
             switch (level)
             {
                 case Level.Finest:
-                    _logger.Verbose(messageString);
+                    _logger.Verbose(message);
                     break;
                 case Level.Debug:
-                    _logger.Debug(messageString);
+                    _logger.Debug(message);
                     break;
                 case Level.Info:
-                    _logger.Information(messageString);
+                    _logger.Information(message);
                     break;
                 case Level.Warn:
-                    _logger.Warning(messageString);
+                    _logger.Warning(message);
                     break;
                 case Level.Error:
-                    _logger.Error(messageString);
+                    _logger.Error(message);
+                    break;
+            }
+        }
+
+        public void Log(Level level, Exception ex, string message)
+        {
+            if (!IsEnabledFor(level)) return;
+
+            switch (level)
+            {
+                case Level.Finest:
+                    _logger.Verbose(ex, message);
+                    break;
+                case Level.Debug:
+                    _logger.Debug(ex, message);
+                    break;
+                case Level.Info:
+                    _logger.Information(ex, message);
+                    break;
+                case Level.Warn:
+                    _logger.Warning(ex, message);
+                    break;
+                case Level.Error:
+                    _logger.Error(ex, message);
                     break;
             }
         }

src/Agent/NewRelic/Agent/Extensions/NewRelic.Agent.Extensions/Logging/ILogger.cs

@@ -1,6 +1,8 @@
 // Copyright 2020 New Relic, Inc. All rights reserved.
 // SPDX-License-Identifier: Apache-2.0
 
+using System;
+
 namespace NewRelic.Agent.Extensions.Logging
 {
     public enum Level
@@ -16,5 +18,6 @@ public interface ILogger
     {
         bool IsEnabledFor(Level level);
         void Log(Level level, string message);
+        void Log(Level level, Exception ex,  string message);
     }
 }

src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/MicrosoftExtensionsLogging/MicrosoftLoggingWrapper.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;
@@ -59,7 +59,7 @@ private void RecordLogMessage(MethodCall methodCall, MEL.ILogger logger, IAgent
                 Func<object, string> getLevelFunc = mc => ((MethodCall)mc).MethodArguments[0].ToString();
                 Func<object, string> getRenderedMessageFunc = mc => ((MethodCall)mc).MethodArguments[2].ToString();
                 Func<object, Exception> getLogExceptionFunc = mc => ((MethodCall)mc).MethodArguments[3] as Exception; // using "as" since we want a null if missing
-                Func<object, Dictionary<string, object>> getContextDataFunc = nothx => GetContextData(logger, agent);
+                Func<object, Dictionary<string, object>> getContextDataFunc = _ => GetContextData(logger, agent);
 
                 var xapi = agent.GetExperimentalApi();
                 xapi.RecordLogMessage(WrapperName, methodCall, getTimestampFunc, getLevelFunc, getRenderedMessageFunc, getLogExceptionFunc, getContextDataFunc, agent.TraceMetadata.SpanId, agent.TraceMetadata.TraceId);
@@ -83,7 +83,7 @@ private static Dictionary<string, object> GetContextData(MEL.ILogger logger, IAg
 
                 // Get the last ScopeLogger in the array (logger.ScopeLoggers[loggers.Length-1])
                 // If there is more than one scope logger, the last logger is the one with the ExternalScopeProvider set
-                object lastLogger = loggers.GetValue(loggers.Length-1);
+                object lastLogger = loggers.GetValue(loggers.Length - 1);
 
                 // Get the scope provider from that logger (logger.ScopeLoggers[loggers.Length-1].ExternalScopeProvider)
                 var scopeProviderPI = _scopeProviderPropertyInfo ??= lastLogger.GetType().GetProperty("ExternalScopeProvider");
@@ -97,12 +97,12 @@ private static Dictionary<string, object> GetContextData(MEL.ILogger logger, IAg
                     {
                         foreach (var kvp in kvps)
                         {
-                            accumulatedKvps.Add(kvp.Key, kvp.Value);
+                            accumulatedKvps[kvp.Key] = kvp.Value; // use this notation to ensure we keep the last value in case nested scopes have the same key
                         }
                     }
                     else if (scopeObject is KeyValuePair<string, object> kvp)
                     {
-                        accumulatedKvps.Add(kvp.Key, kvp.Value);
+                        accumulatedKvps[kvp.Key] = kvp.Value; // use this notation to ensure we keep the last value in case nested scopes have the same key
                     }
                     // Possibly handle case of IEnumerable<KeyValuePair<object, object>>, etc (not now though)
                 }, harvestedKvps);
@@ -111,7 +111,7 @@ private static Dictionary<string, object> GetContextData(MEL.ILogger logger, IAg
             }
             catch (Exception e)
             {
-                agent.Logger.Log(Level.Warn, $"Unexpected exception while attempting to get context data. Context data is not supported for this logging framework. Exception: {e}");
+                agent.Logger.Log(Level.Warn, e, $"Unexpected exception while attempting to get context data. Context data is not supported for this logging framework.");
                 _contextDataNotSupported = true;
                 return null;
             }