Add telemetry to System.Net.NameResolution

[MihaZupan]

Added 2 counters:

• dns-lookups-requested - total number of lookups since process started*
• dns-lookups-duration - EventCounter for min/mean/max duration of requests in the last event counter interval

Added 3 EventSource events:

• ResolutionStart(hostNameOrAddress)
• ResolutionStop()
• ResolutionFailed()

*since telemetry was enabled. This can be changed to since process started by moving the Interlocked.Increment calls outside the IsEnabled() check. That would avoid the race condition mentioned for Net.Http at the cost of an extra Interlocked operation even when telemetry is disabled.

I've manually verified that the counters and events are emitted.

Contributes to #37428

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[MihaZupan]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[stephentoub]

I'd rather this be a separate async method, e.g.

return NameResolutionTelemetry.IsEnabled ?
    WithLogging(hostName, justReturnParsedIp, throwOnIPAny, justAddresses) :
    NameResolutionPal.GetAddrInfoAsync(hostName, justAddresses);

static async Task WithLogging(string hostName, bool ustAddresses)
{
    ValueStopwatch stopwatch = NameResolutionTelemetry.Log.ResolutionStart(hostName);
    bool successful = true;
    try
    {
        await NameResolutionPal.GetAddrInfoAsync(hostName, justAddresses);
    }
    catch
    {
        LogFailure(hostName, stopwatch);
        successful = false;
    }

    NameResolutionTelemetry.Log.AfterResolution(hostName, stopwatch, successful);
}

[stephentoub]

Ah, I just saw your comment at the end. Ok.

[MihaZupan]

But NameResolutionPal.GetAddrInfoAsync returns a Task object that is actually Task<IPHostEntry> / Task<IPAddress[]>, so we have to return that task itself

[noahfalk]

I suspect you could still do @stephentoub's suggestion by using a generic parameter and evaluating typeof(T) == typeof(IPAddress[]) in place of justAddresses parameter. For example:

class Program
{
    static async Task Main(string[] args)
    {
        await LogDoWork<IPAddress[]>();
        await LogDoWork<IPHostEntry>();
    }

    static Task<T> DoWork<T>()
    {
        Debug.Assert(typeof(T) == typeof(IPAddress[]) ||
                     typeof(T) == typeof(IPHostEntry));
        if (typeof(T) == typeof(IPAddress[]))
        {
            return Unsafe.As<Task<T>>(Task.FromResult(new IPAddress[1]));
        }
        else
        {
            return Unsafe.As<Task<T>>(Task.FromResult(new IPHostEntry()));
        }
    }

    static async Task<T> LogDoWork<T>()
    {
        Console.WriteLine("starting");
        T ret = await DoWork<T>();
        Console.WriteLine("ending:" + ret.GetType());
        return ret;
    }
}

class IPAddress { }
class IPHostEntry { }

I don't mean to push it though, its a style choice that you can feel free to use if you like it.

[noahfalk]

Unresolving this to make sure the comment I added is visible, resolve again any time

[MihaZupan]

Gist of new changes:

• Renamed ResolutionStart to ResolutionStarting
• Made all [Event] methods void-returning
• Added the IsEnabled(EventLevel.Informational) condition before writing events / fetching the timestamp
• Only calling ContinueWith and allocating the state if events would be fired (IsEnabled(EventLevel.Informational))
• Added a comment about ResolutionSuccess and ResolutionFailure signatures having to match
• Added a comment about the Span WriteEvent overloads mimicking the behavior of string ones

[noahfalk]

Great to see more telemetry being added : ) A few questions/recommendations inline

[noahfalk]

I suspect you could still do @stephentoub's suggestion by using a generic parameter and evaluating typeof(T) == typeof(IPAddress[]) in place of justAddresses parameter. For example:

class Program
{
    static async Task Main(string[] args)
    {
        await LogDoWork<IPAddress[]>();
        await LogDoWork<IPHostEntry>();
    }

    static Task<T> DoWork<T>()
    {
        Debug.Assert(typeof(T) == typeof(IPAddress[]) ||
                     typeof(T) == typeof(IPHostEntry));
        if (typeof(T) == typeof(IPAddress[]))
        {
            return Unsafe.As<Task<T>>(Task.FromResult(new IPAddress[1]));
        }
        else
        {
            return Unsafe.As<Task<T>>(Task.FromResult(new IPHostEntry()));
        }
    }

    static async Task<T> LogDoWork<T>()
    {
        Console.WriteLine("starting");
        T ret = await DoWork<T>();
        Console.WriteLine("ending:" + ret.GetType());
        return ret;
    }
}

class IPAddress { }
class IPHostEntry { }

I don't mean to push it though, its a style choice that you can feel free to use if you like it.

[ManickaP]

LGTM, no expert here though, you probably want other approvals as well.

[ManickaP]

Nice trick, I like it 😄

[alnikola]

Overall looks good, but please fix IsEnabled checks.

[MihaZupan]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[MihaZupan]

@brianrob You requested changes, I believe they were addressed. If so, please approve so that the PR can be merged.

[brianrob]

LGTM, thanks!

[MihaZupan]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[MihaZupan]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[MihaZupan]

Failure: #37540