Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Feature: Basic Tracing/Logging #60

Closed
7 tasks done
MichaCo opened this issue Mar 5, 2020 · 0 comments
Closed
7 tasks done

New Feature: Basic Tracing/Logging #60

MichaCo opened this issue Mar 5, 2020 · 0 comments

Comments

@MichaCo
Copy link
Owner

MichaCo commented Mar 5, 2020

With the lack of good tracing it is currently relatively hard to figure out rare errors in remove applications.

It seems there are a couple of issues when running in certain container environments.
While trying to improve the resiliency of this library, I'd also like to give you some way to get more information about what's going (wr)on(g). (#51 #52)

Adding logging is always a fun one... I don't want to add any external Nuget dependency, so even the fact that Microsoft.Extensions.Logging.Abstractions doesn't have any dependencies to anything else, and it would probably ok to use it. I still don't want to add it now in a minor version update and then deal with different versions of that package...

I will probably go for the following:
(see #58)

  • An internal logging abstraction which is an even simpler version of the Microsoft framework to have at least a nice API to log errors, warnings of debug messages...
  • Offer different options to hook into those log messages which will be disabled if the consumer doesn't do anything.
    • Simple static TraceSource
    • Simple adapter to Microsoft.Extensions.Logging if you want to forward DnsClient's logs to your already existing logging setup
      This will probably be just a code example consumers can copy&paste, I'll probably not create a NuGet for just that
  • Add more logging to
    • parsing errors
    • nameserver resolution
    • Udp and Tcp handlers

I opted for this way over adding it to the configuration of the LookupClient because we need a way to hook into logs even if you don't have access to the LookupClient instance.
For example, if DnsClient is used in the MongoDriver, you don't control the instance or configuration that library uses, but you still need log messages.

Those APIs might be subject to change if it turns out that there are better alternatives or no-one has anything against adding a dependency to Microsoft.Extensions.Logging.Abstractions I'll be happy to just use that instead.

Option 1 - TraceSource

To use the first option, you'll be able to hook into TraceSource like this:

DnsClient.Tracing.Source.Switch.Level = SourceLevels.Information;
DnsClient.Tracing.Source.Listeners.Add(new ConsoleTraceListener());

That't all you need to get the logs send to a TraceListener.
(I never used that Tracing framework to expose trace messages, so if there is a better way then a static instance, I'm happy to hear feedback ^^)

Option 2 - Forward to Microsoft.Extensions.Logging

Forwarding the log messages from my internal logging to the Microsoft framework just requires some stitching.
There is a static instance of my own LoggerFactory which can be set to a new implementation.
All you have to do is setup the Microsoft logging configuration and such and then use the wrapper below and make DnsClient use that LoggerFactory instead of the default one.

// Replace with your logging setup..
var services = new ServiceCollection();
services.AddLogging(o =>
{
    o.AddConsole();
    o.SetMinimumLevel(Microsoft.Extensions.Logging.LogLevel.Trace);
});
var provider = services.BuildServiceProvider();

// get an instance of ILoggerFactory and pass it to the wrapper
var factory = provider.GetRequiredService<Microsoft.Extensions.Logging.ILoggerFactory>();

// Re-define the LoggerFactory used by the library to the new wrapper
DnsClient.Logging.LoggerFactory = new LoggerFactoryWrapper(factory);

The wrapper code (just an example, feel free to improve it ^^)

internal class LoggerFactoryWrapper : DnsClient.Internal.ILoggerFactory
{
    private readonly Microsoft.Extensions.Logging.ILoggerFactory _microsoftLoggerFactory;

    public LoggerFactoryWrapper(Microsoft.Extensions.Logging.ILoggerFactory microsoftLoggerFactory)
    {
        _microsoftLoggerFactory = microsoftLoggerFactory ?? throw new ArgumentNullException(nameof(microsoftLoggerFactory));
    }

    public DnsClient.Internal.ILogger CreateLogger(string categoryName)
    {
        return new DnsLogger(_microsoftLoggerFactory.CreateLogger(categoryName));
    }

    private class DnsLogger : DnsClient.Internal.ILogger
    {
        private readonly Microsoft.Extensions.Logging.ILogger _logger;

        public DnsLogger(Microsoft.Extensions.Logging.ILogger logger)
        {
            _logger = logger ?? throw new ArgumentNullException(nameof(logger));
        }

        public bool IsEnabled(DnsClient.Internal.LogLevel logLevel)
        {
            return _logger.IsEnabled((Microsoft.Extensions.Logging.LogLevel)logLevel);
        }

        public void Log(DnsClient.Internal.LogLevel logLevel, int eventId, Exception exception, string message, params object[] args)
        {
            _logger.Log((Microsoft.Extensions.Logging.LogLevel)logLevel, eventId, exception, message, args);
        }
    }
}
@MichaCo MichaCo added this to the 1.3.0 milestone Mar 5, 2020
@MichaCo MichaCo self-assigned this Mar 5, 2020
MichaCo added a commit that referenced this issue Mar 5, 2020
Adding the original Srv record to the ServiceHostEntry results #34
MichaCo added a commit that referenced this issue Mar 7, 2020
* Adding filtering of resolved nameservers in case the native fallback is used to remove site local addresses.
* Also changed the cache slightly to not cache results without answers
* Adding basic logging, see #60
* Adding a parser exception 
* Changing the ServiceHostEntry to directly have priority and weigth properties fixes #34
* Adding netstandard2.1 target which doesn't need the System.Buffers reference anymore.
MichaCo added a commit that referenced this issue Mar 10, 2020
…g. see #60.

+ unit tests for all the good and bad truncation handling added see #52
@MichaCo MichaCo changed the title Adding Basic Tracing/Logging New Feature: Basic Tracing/Logging Mar 11, 2020
MichaCo added a commit that referenced this issue Mar 14, 2020
* changed how opt records are created and used. Added configuration to disable EDNS and to set the requested buffer size and DnsSec
* Changes the behavior in case of bad responses which were truncated by some middleman proxy or router - fixes #52
* Changing default unknown record handling to preserve the original data so that users can work with those records.
* Reworking error handling see #60
* Adding new setting ContinueOnEmptyResponse #64
@MichaCo MichaCo closed this as completed Jun 20, 2021
Repository owner locked and limited conversation to collaborators Jun 20, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

1 participant