Make it easier to use default destructurer for custom exceptions

[kmcclellan]

Describe the feature

If an exception type isn't recognized by this library, the fallback is to use the ReflectionBasedDestructurer. It's a common case to have custom exception types that I want to use with the base ExceptionDestructurer instead. Currently there's no easy way to configure this to occur.

My current workaround

new DestructuringOptionsBuilder()
    .WithDefaultDestructurers()
    .WithDestructurers(new[]
    {
        new DefaultExceptionDestructurer<MyException>()
    });

class DefaultExceptionDestructurer<T> : ExceptionDestructurer
{
    public override Type[] TargetTypes { get; } = { typeof(T) };
}

While this isn't much code, it's counterintuitive and required analysis of this library's internals. It'd be better to have more obvious support for this use case.

Possible enhancements

Probably the simplest way to address this would be to leverage the existing IDestructuringOptions.DisableReflectionBasedDestructurer. Currently, setting this option causes the enricher to return null for unrecognized exception types. It could be changed to fall back to ExceptionDestructurer instead (I doubt anyone is depending upon that null value).

Alternatively (or additionally), the options could support selecting between ExceptionDestructurer and ReflectionBasedDestructurer for individual types. Something like this maybe:

new DestructuringOptionsBuilder()
    .WithDefaultException<MySimpleException>()
    .WithDefaultException<MyComplexException>(useReflection: true);

public DestructuringOptionsBuilder WithDefaultException<TException>(bool useReflection = false)
{
    // Implementation
}

Let me know if there is interest in a PR for one or both of these enhancements.

[RehanSaeed]

Not sure about the naming WithDefaultException but that method seems like a reasonable idea. How about something like ConfigureException(bool useReflection)? Thoughts @krajek?

[krajek]

@kmcclellan Thanks for reaching out. Let me understand your case better. Correct me if I am wrong, please.

You have some custom exceptions for which you intentionally would like to avoid capturing all the properties and would rather just log the "common" exception properties like Message and StackTrace. In your case, the flexibility of reflection based destructurer is a downside.

Is this the correct description of what you are trying to achieve? If so, why exactly is reflection based destructurer wrong solution in your case? You want to hide sensitive data, save space, or for some other reason?

[kmcclellan]

@krajek That's a good question, actually, since it points to deeper functionality that I found unexpected.

My case is that Exception.Data contains instances of models for which I've configured custom destructuring policies. They destructure correctly for the standard exceptions, but are overridden by the ReflectionBasedDestructurer.

using Serilog;
using Serilog.Exceptions;
using Serilog.Formatting.Json;
using System;

namespace SerilogExceptionDestructuring
{
    class Program
    {
        static void Main()
        {
            Log.Logger = new LoggerConfiguration()
                .WriteTo.Console(formatter: new JsonFormatter())
                .Destructure.ByTransforming<Token>(
                    x => new { x.Id, Token = new string('*', x.Value.Length) })
                .Enrich.WithExceptionDetails()
                .CreateLogger();

            try
            {
                var token = new Token { Id = 1, Value = "Don't show me!" };
                Log.Information("Token created: {@Token}", token);

                var exception = new TokenException();
                exception.Data["@Token"] = token;
                throw exception;
            }
            catch (Exception exception)
            {
                Log.Error(exception, "An exception occurred");
            }
        }
    }

    class Token
    {
        public int Id { get; set; }
        public string Value { get; set; }
    }

    class TokenException : Exception { }
}

{"Timestamp":"2020-12-23T14:41:15.2260722-05:00","Level":"Information","MessageTemplate":"Token created: {@Token}","Properties":{"Token":{"Id":1,"Token":"**************"}}}
{"Timestamp":"2020-12-23T14:41:15.2804143-05:00","Level":"Error","MessageTemplate":"An exception occurred","Exception":"SerilogExceptionDestructuring.TokenException: Exception of type 'SerilogExceptionDestructuring.TokenException' was thrown.\r\n   at SerilogExceptionDestructuring.Program.Main() in C:\\Users\\kmcclellan\\sandbox\\SerilogExceptionDestructuring\\Program.cs:line 26","Properties":{"ExceptionDetail":{"Data":{"@Token":{"Id":1,"Value":"Don't show me!"}},"HResult":-2146233088,"Message":"Exception of type 'SerilogExceptionDestructuring.TokenException' was thrown.","Source":"SerilogExceptionDestructuring","Type":"SerilogExceptionDestructuring.TokenException"}}}

You'll notice that "Don't show me!" appears in the TokenException details. Throwing a base Exception instead has the desired behavior:

{"ExceptionDetail":{"Type":"System.Exception","Data":{"@Token":{"Id":1,"Token":"**************"}},"HResult":-2146233088,"Message":"Exception of type 'System.Exception' was thrown.","Source":"SerilogExceptionDestructuring"}}}

This is because the base ExceptionDestructurer hands the exception Data dictionary as-is to the property bag, rather than trying to destructure it. For any exception destructurer, arguably, the core IDestructuringPolicys should take precedence for non-exception values within the exception tree. If ReflectionBasedDestructurer adhered to this, it would work fine for my case.

[asgerhallas]

+1 for this "For any exception destructurer, arguably, the core IDestructuringPolicys should take precedence for non-exception values within the exception tree. If ReflectionBasedDestructurer adhered to this, it would work fine for my case." :)