[dotnet] Internal logging

[nvborisenko]

We need logging for .net.

Description

There is single entry point Log, but Context aware.

Usage:

public class Tests
{
    ChromeDriver driver;

    [OneTimeSetUp]
    public void OneTimeSetup()
    {
        // configure global logging
        Log.SetLevel(LogEventLevel.Trace);
    }

    [SetUp]
    public void Setup()
    {
        driver = new ChromeDriver();
    }

    [TearDown]
    public void Tear()
    {
        driver?.Dispose();
    }

    [Test]
    public async Task Test1()
    {
        // adjust logging per test
        using Log.CreateContext(LogEventLevel.Info).Handlers.Add(new ConsoleLogHandler());

        driver.Url = "https://www.yahoo.com/";
    }

    [Test]
    public async Task Test2()
    {
        // It uses globally configured logging
        driver.Url = "https://www.yahoo.com/";
    }
}

Motivation and Context

We need logging for .net.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[YevgeniyShunevych]

Thank you for this feature implementation, @nvborisenko. Let me please review this PR a bit.

Overall implementation is fine, as for me, "ambient context" looks like a good way to inject logging in the current state of WebDriver without breaking changes. I believe in the future WebDriver versions we'll be able to have it injected/configured through WebDriver property or constructor parameter.

Anyway, I have a couple of recommendations regarding the logging API.

I would merge Log and LogContext classes into one public non-static LogContext (with few static members) and remove ILogContext. Both Log and LogContext contain similar methods SetLevel and AddHandler. In case of merge the API can be changed the following way.

Configure global logging

Log.SetLevel(Level.Trace);
// ->
LogContext.Global.SetLevel(Level.Trace);

Where LogContext.Global property and SetLevel return the same instance of LogContext.

Adjust logging per test

Log.ForContext.SetLevel(Level.Trace).AddHandler(new ConsoleLogHandler());
// ->
LogContext.ConfigureAmbient().SetLevel(Level.Trace).AddHandler(new ConsoleLogHandler());

Currently, if you call twice Log.ForContext.AddHandler(new ConsoleLogHandler()); within the same thread, you'll register ConsoleLogHandler twice. This definitely can happen for users, and will produce duplicated log entries.

So I would replace Log.ForContext property with LogContext.ConfigureAmbient(). Calling LogContext.ConfigureAmbient() should reset current _ambientLogContext.Value with clone of _globalLogContext as it is now.

Logger initialization

private readonly ILogger _logger = Log.ForContext.GetLogger<HttpCommandExecutor>();
// ->
private readonly ILogger _logger = LogContext.ResolveLogger<HttpCommandExecutor>();

ResolveLogger under the hood, if _ambientLogContext.Value is null, should not initialize it, but use _globalLogContext.
If someone doesn't use internal logging at all or use only global logging, there is no need to clone LogContext (initialize ambient context) for each thread.

[codecov-commenter]

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (240e17b) 57.53% compared to head (b97b525) 57.95%.
Report is 8 commits behind head on trunk.

❗ Current head b97b525 differs from pull request most recent head 9b61d8a. Consider uploading reports for the commit 9b61d8a to get more accurate results

Files	Patch %	Lines
py/selenium/webdriver/safari/webdriver.py	50.00%	3 Missing ⚠️
py/selenium/webdriver/chromium/webdriver.py	83.33%	1 Missing and 1 partial ⚠️
py/selenium/webdriver/firefox/remote_connection.py	0.00%	1 Missing ⚠️
py/selenium/webdriver/firefox/webdriver.py	83.33%	1 Missing ⚠️
py/selenium/webdriver/ie/webdriver.py	87.50%	1 Missing ⚠️
py/selenium/webdriver/remote/webdriver.py	66.66%	1 Missing ⚠️
py/selenium/webdriver/safari/service.py	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #13140      +/-   ##
==========================================
+ Coverage   57.53%   57.95%   +0.42%     
==========================================
  Files          86       88       +2     
  Lines        5310     5333      +23     
  Branches      221      224       +3     
==========================================
+ Hits         3055     3091      +36     
+ Misses       2034     2018      -16     
- Partials      221      224       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[titusfortner]

To direct output to a console, you need to add a handler:

1. Where are logs sent by default?
2. How do we send logs to a file?

[titusfortner]

What is the thought behind naming it SetMinimumLevel instead of just SetLevel?

[nvborisenko]

Where are logs sent by default?

No logs by default at all. User should explicitly turn it on.

How do we send logs to a file?

Should we implement it as well?

What is the thought behind naming it SetMinimumLevel instead of just SetLevel?

It is clearer that SetMinimumLevel(Trace) will emit logs with Debug level also, not only with Trace level. I think we can rename it to SetLevel just to be aligned with others bindings.

[titusfortner]

I really think we should turn it on by default, and just not log anything at the default level unless it is important.

Yes, ideally we have a Handler implementation that can stream to stdout, stderr or file

[nvborisenko]

I returned back SetLevel(). By default now we use ConsoleLogHandler which outputs to strerr.

[titusfortner]

Do you know what the test failures are from?

[nvborisenko]

This PR is ready for review. API is stabilized, user is able to do whatever he wants. We still don't have the implementation of FileLogHandler, let's leave it for next releases (writing to a file is not easy task in terms of efficiency).

Build is failing, I have no ideas why.

[titusfortner]

Thanks for all of your work on this @YevgeniyShunevych & @nvborisenko !!