High Performance Logging

Feature name:

  • High Performance Logging

Feature Description

Microsoft recommends using LoggerMessage to define delegates that are cacheable and reduce the amount of allocations that are created when compared to the normal extension methods. But don’t just believe me here is their write-up.

There are two general ways I have seen this implemented:

  1. An encomposing LoggerExtensions class
    • This class would hold all the logging events for that project/namespace (design decision)
    • Depending on how many events there are in the chosen scope it could get large and hard to navigate/organize.
    • Invocation Example: _logger.UserCreated();
    • Example
  2. Inner static Log class
    • This would be a inner static class that would hold all the logging events for the class it is in.
    • This would be much more targeted so you would know where the events are but it would also create more code smell at the bottom of a lot of files.
    • Invocation Example: Log.UserCreated(_logger);
    • Example

An example of just one place this could be implemented in the server repo is here.

Example 1.

public static class LoggerExtensions
{
    private static Action<ILogger, string, Exception?> _sendEmailFailed;
    // More delegates

    static LoggerExtensions()
    {
        _sendEmailFailed = LoggerMessage.Define(LogLevel.Warning,
            new EventId(1, nameof(SendEmailFailed),
            "Failed to send email. Re-retying...");
    }

    public static void SendEmailFailed(this ILogger logger, Exception ex)
    {
        _sendEmailFailed(logger, ex);
    }

    // More event methods
}

// Usage:
// *Removed for brevity*
catch (Exception e)
{
    _logger.SendEmailFailed(e);
}

Example 2.

public class AmazonSesMailDeliveryService : IMailDeliveryService, IDisposable
{
    // Removed for brevity


    public async Task SendEmailAsync(MailMessage message)
    {
        // Removed for brevity
        catch (Exception e)
        {
             Log.SendEmailFailed(_logger, e);
        }
    }

    private static class Log
    {
        private static Action<ILogger, string, Exception?> _sendEmailFailed;

        static LoggerExtensions()
        {
            _sendEmailFailed = LoggerMessage.Define(LogLevel.Warning,
                new EventId(1, nameof(SendEmailFailed),
                "Failed to send email. Re-retying...");
        }

        public static void SendEmailFailed(ILogger logger, Exception ex)
        {
            _sendEmailFailed(logger, ex);
        }
    }
}

Some of the conventions I have used and described can be changed but those are what I have seen used. I am willing to work on this I would just want to settle on the conventions you would want used and some promise that it is something Bitwarden is interested in.

Clients / Repos Affected:

  • Server

Timeline to completion (estimate):

ETA: Q2/2021

Hi @jdbaur ,

Welcome to the forums and thank you for the suggestion and ask around a potential contribution.

I’m looking through your proposal and reading some of the supporting literature. This will really take some careful thought and consideration around design choices and code smell (agreed) and at least initially I’m not sure we’d be particularly keen on overhauling the logging implementation; at present moment we’re not seeing any performance indicators that signal we would need to look at optimizing this. Let me circle back after I have some time to review with the team.

1 Like

Hi @jdbaur ,

I would suggest we hold off on this for now. There’s currently not a high enough performance impact to justify the change in implementation or patterns for logging at the moment and I’m not sure the team would be able to agree on a means of implementation/patterns or even when it’s appropriate vs. not.

Thank you again for bringing it up, and especially happy you asked here before putting in the work of implementation and this coming out during the PR stage.

Please let me know if you have any questions.