Skip to content

Commit

Permalink
Don't redact assemblies that we instrument
Browse files Browse the repository at this point in the history
  • Loading branch information
@andrewlock
andrewlock committed Nov 9, 2023
1 parent de82ccd commit ffad078
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 23 deletions.
30 changes: 19 additions & 11 deletions tracer/src/Datadog.Trace/Logging/Internal/ExceptionRedactor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#nullable enable
using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.Reflection;
Expand All @@ -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);
Expand All @@ -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++)
{
Expand All @@ -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);
Expand All @@ -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)
{
Expand Down
7 changes: 6 additions & 1 deletion tracer/src/Datadog.Trace/Logging/Internal/RedactedErrorLogSink.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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);
Expand Down
24 changes: 13 additions & 11 deletions tracer/test/Datadog.Trace.Tests/Logging/ExceptionRedactorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);

Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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());
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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()
Expand All @@ -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,
Expand Down

0 comments on commit ffad078

Please sign in to comment.