serilog-contrib / serilog-sinks-richtextbox

A Serilog sink that writes log events to a WPF RichTextBox control with colors and theme support
Apache License 2.0
99 stars 25 forks source link

Make IRichTextBox public for Dependency Injection #18

Open LazerFX opened 2 years ago

LazerFX commented 2 years ago

I'm writing an application (Purely for myself, I don't intend to use this in production or anything), but I'd like to use RichTextBox sink - saves me having to have a console or spin up a file dropper or something like that every time I want to see logger output. However, the application is written in .NET 5 WPF, using Dependency Injection throughout. Serilog is registered through DI.

The issue is that I cannot configure the Logger after application start because it uses the UseSerilog extension on the IHostBuilder interface, but at that point the RichTextBox I want to target isn't instantiated.

My solution would be simple - instantiate an IRichTextBox interface that can be set up as a UseSingleton value. Then, I can inject that into the target Window, add the RichTextBox itself to it, and do a pass-through from the interface to the actual controls on the RichTextBox. I can't do this at the moment, as the IRichTextBox interface and the WriteTo.RichTextBox extension method that uses the IRichTextBox is internal sealed.

Before I go ahead and do this and put a PR together with the changes, I just thought I'd push this out for comment from the team working on this to see if anyone has any suggestions - maybe there's a better way of doing this?

augustoproiete commented 2 years ago

Thanks @LazerFX for the suggestion and detailed scenario explanation.

Conceptually I think your idea makes sense - i.e. Provide a way to delay providing the RichTextBox control to the sink for scenarios where the logging pipeline is set up before the UI is available.

The IRichTextBox interface specifically is a very raw API that reflects a minimum set of methods needed to write to the control based on the current implementation (which will change for sure as we add more features such as writing paragraphs instead of inlines, for example), so I'd be hesitant to make that particular interface public as it is today.

I'm thinking a way forward could be to create a new factory-like interface that returns the RichBoxControl directly (instead of an abstraction), and adapt RichTextBoxImpl to use that instead of receiving the control instance directly.

public interface IRichTextBoxProvider
{
    System.Windows.Controls.RichTextBox GetRichTextBox();
}

This should allow for the scenario you described above... The only difference is the public API is more high level and less likely to change.

Would that work?


If Yes the next question is what to do with log messages that are written to the log during the start-up of the app, before the IRichTextBoxProvider has been activated with the instance of the control.

or

or

LazerFX commented 2 years ago

Thanks for the quick response! I see what you mean - I can provide an IRichTextBoxProvider instantiation within the DI environment which then gets consumed by the sink... All I have to do is in the setup of the Window, take that provider and assign the real RichTextBox to whatever variable holds it within the DI container. That works...

For my scenario, I'm not fussed about the startup logging; however, I can understand that others would want those logged... I can see it getting tricky - if you've a nominally complex WPF app, you're going to have a massive volume of binding events logged if you've even slightly high levels of logging, and that will rapidly eat up cache space - you don't want people asking why suddenly there's a massive memory load during startup when they turn on logging to the RichTextBox... Perhaps allow it to be optionally caching? I mean - it's quite common to have multiple sinks hooked up to Serilog, and I can imagine this being used for specific message outputs that are use targeted - perhaps outputs of a specific type or from a few specific functions (That's what I'm doing). Just spitballing on what the use-cases could be, and whether it's possible to put a simple API to configure as many of those as possible :D

augustoproiete commented 2 years ago

Cool. Agreed that the caching part needs some more thought.

Let's not get blocked by that then, we can start with a first version that has no buffering and discards the messages if the control is not available (i.e. whenever IRichTextBoxProvider.GetRichTextBox returns null) , and in a future version we can add buffering and toggles to opt-in or opt-out (depending on what gets decided as the default later).

Feel free to send a PR at your convenience.

LazerFX commented 2 years ago

Been looking through this - might take me a while to put a PR together for this; got pulled onto some things at work which means personal projects are slipped onto the back burner. Will close this issue as there's a plan if I want to go ahead on this, and say thanks for the support in deciding the best route :)

augustoproiete commented 2 years ago

No worries at all.

I think others might be interested in this feature as well, so let's leave the issue open as up-for-grabs in case anyone wants to take a stab at it in the short-term. Hacktoberfest is just around the corner and this one looks like low-hanging fruit.

KayakFisher205 commented 2 years ago

I'm not a WPF programmer but I took the sample for the net core 5 winforms/WPF and incorporated it into an existing winform (csproj edited as with the code from WinFormsHostNet50Sample) that works with DI of Serliog. I was able to get the DI to the MainForm, from the program.cs. Here is what I did.

I made a static public reference for the RichTextBox and the ElementHost in program.cs. I populated most of it in the Main().

I setup Serilog in the ConfigureServices(), along with using the Reference to the RIchTextBox and reading my other Serilog setting form my appsettings.json.

In the MainForm, where I had placed the _richTextBoxPanel (see sample code for WinFormsHostNet50Sample), after the InitializeComponent() line, I added

        _richTextBoxPanel.Controls.Add(Program.richTextBoxHost);

So the partial header of MainForm.cs is: public partial class MainForm : Form {

    private readonly Settings _settings;
    private readonly ILogger<MainForm> _logger;
    public BackgroundWorker bgw;

    public bool Developer;
    public int SleepFor;
    public bool DelayStart = false;
    public bool StartUp = true;

    public MainForm(IOptions<Settings> settings,  ILogger<MainForm> logger)
    {
        InitializeComponent();

        _richTextBoxPanel.Controls.Add(Program.richTextBoxHost);

} }

I was able to use _logger.LogInformation from this point. All of the other classes have the ILogger Injection as Ilogger I have attached my cleaned up program.cs file.

program.zip

LazerFX commented 2 years ago

If I'm understanding you right, you basically made a global static singleton RichTextBox reference that's outside any DI framework? That's not Dependency Injection, that's a global variable. Which, if you're looking for a testable, maintainable environment is... very much a no-no; though I might be misunderstanding you're demonstration here, and I'll be honest I've not looked at the code yet.

augustoproiete commented 2 years ago

Thanks @KayakFisher205. Interesting hack, thanks for sharing! I will say though it's not exactly something I'd recommend as seems too early in the process for creating UI controls, but if it works for you.

@LazerFX That's it. The code sample holds a global static instance of a detached RichTextBox control that is created at the start and later shared with the view that will display it. That said, it would technically be possible to avoid using globals by registering the RichTextBox instance with the container, and having it as a dependency of the view (resolved by the DI container) - still a hack and not ideal, but "testable"

KayakFisher205 commented 2 years ago

Agreed, I misspoke about DI. As you both said, it is definitely a hack and not good for production. Hopefully, it will be a catalyst for the really smart people to figure something out.

frankhommers commented 2 years ago

I would like this feature as well.

What also would be an option is to supply a Func to the WriteTo Extension method, so it can be resolved.

augustoproiete commented 2 years ago

@frankhommers I'm not sure a Func would work in this scenario because at the time you set up the Serilog pipeline, you might not have access to the RichTextBox control yet (e.g. It has not yet been created).

Either way, feel free to send a PR at your convenience.

thargy commented 1 year ago

If I'm understanding you right, you basically made a global static singleton RichTextBox reference that's outside any DI framework? That's not Dependency Injection, that's a global variable. Which, if you're looking for a testable, maintainable environment is... very much a no-no; though I might be misunderstanding you're demonstration here, and I'll be honest I've not looked at the code yet.

You can always add the RichTextBox as a 'named instance' to the service provider, that way the RTB will collect logs from the start, and you can retrieve the RTB in the constructor of the window/control you want to add it to. If named instances aren't supported in your DI framework, then you can create a wrapped object to hold the RTB.