Don't redact assemblies that we instrument

DataDog · Nov 8, 2023 · 25a8246 · 25a8246
1 parent aa4dc15
commit 25a8246
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 23 deletions.
diff --git a/tracer/src/Datadog.Trace/Logging/Internal/ExceptionRedactor.cs b/tracer/src/Datadog.Trace/Logging/Internal/ExceptionRedactor.cs
@@ -6,6 +6,7 @@
 #nullable enable
 using System;
 using System.Collections;
+using System.Collections.Generic;
 using System.Diagnostics;
 using System.Globalization;
 using System.Reflection;
@@ -29,8 +30,9 @@ internal static class ExceptionRedactor
     /// Records the _type_ of the Exception instead of the exception message
     /// </summary>
     /// <param name="exception">The exception to generate the redacted stack trace for</param>
+    /// <param name="instrumentedAssemblies">Names of assemblies that we instrument, that we don't want to redact</param>
     /// <returns>The redacted stack trace</returns>
-    public static string Redact(Exception exception)
+    public static string Redact(Exception exception, HashSet<string> instrumentedAssemblies)
     {
         var exceptionType = exception.GetType().FullName ?? "Unknown Exception";
         var stackTrace = new StackTrace(exception);
@@ -44,13 +46,13 @@ public static string Redact(Exception exception)
         var sb = StringBuilderCache.Acquire(StringBuilderCache.MaxBuilderSize);
         sb.AppendLine(exceptionType);
 
-        RedactStackTrace(sb, stackTrace);
+        RedactStackTrace(sb, stackTrace, instrumentedAssemblies);
 
         return StringBuilderCache.GetStringAndRelease(sb);
     }
 
     // internal for testing
-    internal static void RedactStackTrace(StringBuilder sb, StackTrace stackTrace)
+    internal static void RedactStackTrace(StringBuilder sb, StackTrace stackTrace, HashSet<string> instrumentedAssemblies)
     {
         for (var i = 0; i < stackTrace.FrameCount; i++)
         {
@@ -61,7 +63,7 @@ internal static void RedactStackTrace(StringBuilder sb, StackTrace stackTrace)
                 continue;
             }
 
-            if (ShouldRedactFrame(methodInfo))
+            if (ShouldRedactFrame(methodInfo, instrumentedAssemblies))
             {
                 sb.Append(StackFrameAt);
                 sb.AppendLine(Redacted);
@@ -72,19 +74,25 @@ internal static void RedactStackTrace(StringBuilder sb, StackTrace stackTrace)
         }
     }
 
-    private static bool ShouldRedactFrame(MethodBase mb)
+    private static bool ShouldRedactFrame(MethodBase mb, HashSet<string> instrumentedAssemblies)
         => mb.DeclaringType switch
         {
             null => true, // "global" function
-            // TODO: grab the list of assemblies we instrument from the trimming file (etc)
-            { Assembly.FullName: { } name } => !(name.StartsWith("Datadog.", StringComparison.Ordinal)
-                                              || name.StartsWith("mscorlib,", StringComparison.Ordinal)
-                                              || name.StartsWith("Microsoft.", StringComparison.Ordinal)
-                                              || name.StartsWith("System.", StringComparison.Ordinal)
-                                              || name.StartsWith("Azure.", StringComparison.Ordinal)),
+            { Assembly.FullName: { } name } when IsKnownAssemblyPrefix(name) => false,
+            { Assembly: { } assembly } when assembly.GetName().Name is { } name && instrumentedAssemblies.Contains(name) => false,
             _ => true, // no assembly
         };
 
+    // NOTE: Keep this in sync with InstrumentationDefinitions SourceGenerator implementation in Sources.BuildInstrumentedAssemblies.IsKnownAssemblyPrefix()
+    private static bool IsKnownAssemblyPrefix(string assemblyName)
+    {
+        return assemblyName.StartsWith("Datadog.", StringComparison.Ordinal)
+            || assemblyName.StartsWith("mscorlib,", StringComparison.Ordinal)
+            || assemblyName.StartsWith("Microsoft.", StringComparison.Ordinal)
+            || assemblyName.StartsWith("System.", StringComparison.Ordinal)
+            || assemblyName.StartsWith("Azure.", StringComparison.Ordinal);
+    }
+
 #if NETFRAMEWORK
     private static void AppendFrame(StringBuilder sb, StackFrame sf)
     {

diff --git a/tracer/src/Datadog.Trace/Logging/Internal/RedactedErrorLogSink.cs b/tracer/src/Datadog.Trace/Logging/Internal/RedactedErrorLogSink.cs
@@ -4,6 +4,9 @@
 // </copyright>
 
 #nullable enable
+
+using System.Collections.Generic;
+using Datadog.Trace.ClrProfiler;
 using Datadog.Trace.Telemetry;
 using Datadog.Trace.Telemetry.Collectors;
 using Datadog.Trace.Telemetry.DTOs;
@@ -15,10 +18,12 @@ namespace Datadog.Trace.Logging.Internal;
 internal class RedactedErrorLogSink : ILogEventSink
 {
     private readonly RedactedErrorLogCollector _collector;
+    private readonly HashSet<string> _instrumentedAssemblies;
 
     public RedactedErrorLogSink(RedactedErrorLogCollector collector)
     {
         _collector = collector;
+        _instrumentedAssemblies = InstrumentationDefinitions.InstrumentedAssemblies;
     }
 
     // logs are immediately queued to channel
@@ -32,7 +37,7 @@ public void Emit(LogEvent? logEvent)
         // Note: we're using the raw message template here to remove any chance of including customer information
         var telemetryLog = new LogMessageData(logEvent.MessageTemplate.Text, ToLogLevel(logEvent.Level), logEvent.Timestamp)
         {
-            StackTrace = logEvent.Exception is { } ex ? ExceptionRedactor.Redact(ex) : null
+            StackTrace = logEvent.Exception is { } ex ? ExceptionRedactor.Redact(ex, _instrumentedAssemblies) : null
         };
 
         _collector.EnqueueLog(telemetryLog);

diff --git a/tracer/test/Datadog.Trace.Tests/Logging/ExceptionRedactorTests.cs b/tracer/test/Datadog.Trace.Tests/Logging/ExceptionRedactorTests.cs
@@ -18,14 +18,16 @@ namespace Datadog.Trace.Tests.Logging;
 
 public class ExceptionRedactorTests
 {
+    private static readonly HashSet<string> InstrumentedAssemblies = new(StringComparer.Ordinal) { "Serilog" };
+
     [Theory]
     [InlineData(typeof(Exception))]
     [InlineData(typeof(InvalidOperationException))]
     [InlineData(typeof(ArgumentException))]
     public void Redact_UsesExceptionTypeForEmptyException(Type exceptionType)
     {
         var ex = (Exception)Activator.CreateInstance(exceptionType);
-        var redacted = ExceptionRedactor.Redact(ex);
+        var redacted = ExceptionRedactor.Redact(ex, InstrumentedAssemblies);
 
         redacted.Should().Be(exceptionType.FullName);
     }
@@ -34,7 +36,7 @@ public void Redact_UsesExceptionTypeForEmptyException(Type exceptionType)
     public void Redact_IncludesExceptionTypeWhenHaveStackFrames()
     {
         var ex = InvokeException();
-        var redacted = ExceptionRedactor.Redact(ex);
+        var redacted = ExceptionRedactor.Redact(ex, InstrumentedAssemblies);
 
         redacted.Should().StartWith("System.Exception" + Environment.NewLine);
 
@@ -50,7 +52,7 @@ public void RedactStackTrace_IsEmptyForEmptyException()
         var stackTrace = new StackTrace(ex);
 
         var sb = new StringBuilder();
-        ExceptionRedactor.RedactStackTrace(sb, stackTrace);
+        ExceptionRedactor.RedactStackTrace(sb, stackTrace, InstrumentedAssemblies);
         var redacted = sb.ToString();
 
         redacted.Should().BeEmpty();
@@ -65,7 +67,7 @@ public void RedactStackTrace_RedactsUserCode(object method)
         var stackTrace = new StackTrace(stackFrame);
 
         var sb = new StringBuilder();
-        ExceptionRedactor.RedactStackTrace(sb, stackTrace);
+        ExceptionRedactor.RedactStackTrace(sb, stackTrace, InstrumentedAssemblies);
         var redacted = sb.ToString();
 
         redacted.Should().Be($"{ExceptionRedactor.StackFrameAt}{ExceptionRedactor.Redacted}" + Environment.NewLine);
@@ -80,7 +82,7 @@ public void RedactStackTrace_DoesNotRedactBclAndDatadog(object method)
         var stackTrace = new StackTrace(stackFrame);
 
         var sb = new StringBuilder();
-        ExceptionRedactor.RedactStackTrace(sb, stackTrace);
+        ExceptionRedactor.RedactStackTrace(sb, stackTrace, InstrumentedAssemblies);
         var redacted = sb.ToString();
 
         redacted.Should().Be(stackTrace.ToString());
@@ -91,7 +93,7 @@ public void RedactStackTrace_DoesNotRedactBclAndDatadog(object method)
     public void RedactStackTrace_ContainsExpectedStrings(StackTrace stackTrace, string expectedToString)
     {
         var sb = new StringBuilder();
-        ExceptionRedactor.RedactStackTrace(sb, stackTrace);
+        ExceptionRedactor.RedactStackTrace(sb, stackTrace, InstrumentedAssemblies);
         var redacted = sb.ToString();
 
         if (expectedToString.Length == 0)
@@ -112,7 +114,7 @@ public unsafe void RedactStackTrace_WorksWithFunctionPointerSignature()
         // This is separate from Redact_ContainsExpectedStrings since unsafe cannot be used for iterators
         var stackTrace = TestData.FunctionPointerParameter(null);
         var sb = new StringBuilder();
-        ExceptionRedactor.RedactStackTrace(sb, stackTrace);
+        ExceptionRedactor.RedactStackTrace(sb, stackTrace, InstrumentedAssemblies);
         var redacted = sb.ToString();
 
 #if NET8_0_OR_GREATER
@@ -173,10 +175,6 @@ public static class TestData
             typeof(VerifyTests.VerifierSettings).GetProperty(nameof(VerifyTests.VerifierSettings.StrictJson))?.GetMethod,
             typeof(VerifyTests.VerifierSettings).GetProperty(nameof(VerifyTests.VerifierSettings.StrictJson))?.SetMethod,
             typeof(VerifyTests.SerializationSettings).GetConstructor(Array.Empty<Type>()),
-            typeof(Serilog.Log).GetProperty(nameof(Serilog.Log.Logger))?.GetMethod,
-            typeof(Serilog.Log).GetProperty(nameof(Serilog.Log.Logger))?.SetMethod,
-            typeof(Serilog.Log).GetMethod(nameof(Serilog.Log.CloseAndFlush)),
-            typeof(Serilog.Log).GetMethod(nameof(Serilog.Log.Error), types: new[] { typeof(string) }),
         };
 
         public static TheoryData<object> MethodsToNotRedact() => new()
@@ -190,6 +188,10 @@ public static class TestData
             typeof(Datadog.Trace.Vendors.Serilog.Log).GetMethod(nameof(Serilog.Log.CloseAndFlush)),
             typeof(StringBuilder).GetMethod(nameof(StringBuilder.Clear)),
             typeof(string).GetMethod(nameof(string.IndexOf),  types: new[] { typeof(char) }),
+            typeof(Serilog.Log).GetProperty(nameof(Serilog.Log.Logger))?.GetMethod,
+            typeof(Serilog.Log).GetProperty(nameof(Serilog.Log.Logger))?.SetMethod,
+            typeof(Serilog.Log).GetMethod(nameof(Serilog.Log.CloseAndFlush)),
+            typeof(Serilog.Log).GetMethod(nameof(Serilog.Log.Error), types: new[] { typeof(string) }),
             typeof(System.Threading.Tasks.Task).GetMethod(nameof(System.Threading.Tasks.Task.Wait), types: Array.Empty<Type>()),
             typeof(System.Data.SqlClient.SqlCommand).GetMethod(nameof(System.Data.SqlClient.SqlCommand.Clone)),
             typeof(System.Data.SQLite.SQLiteCommand).GetProperty(nameof(System.Data.SQLite.SQLiteCommand.CommandType))?.GetMethod,

diff --git a/tracer/test/Datadog.Trace.Tests/Telemetry/Collectors/RedactedErrorLogCollectorTests.cs b/tracer/test/Datadog.Trace.Tests/Telemetry/Collectors/RedactedErrorLogCollectorTests.cs
@@ -4,6 +4,7 @@
 // </copyright>
 
 using System;
+using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using System.Text;
@@ -18,6 +19,8 @@ namespace Datadog.Trace.Tests.Telemetry.Collectors;
 
 public class RedactedErrorLogCollectorTests
 {
+    private static readonly HashSet<string> InstrumentedAssemblies = new();
+
     [Fact]
     public void DoesNotQueueMoreThanMaximumQueueSize()
     {