Skip to content

FIX #87983 - Enable events for LogAlways#89326

Merged
davmason merged 1 commit into
dotnet:mainfrom
nickyMcDonald:dev/n77y/fix87983
Jul 25, 2023
Merged

FIX #87983 - Enable events for LogAlways#89326
davmason merged 1 commit into
dotnet:mainfrom
nickyMcDonald:dev/n77y/fix87983

Conversation

@nickyMcDonald

@nickyMcDonald nickyMcDonald commented Jul 21, 2023

Copy link
Copy Markdown
Contributor

Repro of Issue #87983 demonstrates that when an EventListener enables events for NativeRuntimeEventSource with EventLevel.LogAlways then OnEventWritten receives no events.

With a change in src/native/eventpipe/ep-provider.c most events should fire.
With a change in src/coreclr/vm/eventtrace.cpp the rest should fire.
With a change in System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventDispatcher.cs LogAlways will be treated as the highest level.

The issue was that the provider's level being LogAlways was not accounted for when enabling events and when firing them.

Test app that is helpful reproing the issue and providing useful information:

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.Tracing;
using System.Linq;
using System.Threading;

Listener listener = Listener.Create(/*
    EventLevel.Informational//*/
    );
Stopwatch stopwatch = Stopwatch.StartNew();

while (stopwatch.Elapsed.TotalSeconds < 5d)
{
    Thread.Sleep(Random.Shared.Next().ToString().GetHashCode() & sbyte.MaxValue);
}
listener.DumpEvents();

internal sealed class Listener : EventListener
{
    public static EventLevel NextLevel = EventLevel.LogAlways;
    private readonly ConcurrentDictionary<string, int> Distinct = new();
    private EventLevel Level = EventLevel.LogAlways;
    private Listener? listener = null;

    public static Listener Create(params EventLevel[] levels)
    {
        NextLevel = levels.FirstOrDefault(EventLevel.LogAlways);
        return new()
        {
            listener = (levels.Length > 1) ? Create(levels[1..]) : null,
        };
    }

    public void DumpEvents()
    {
        Dispose();
        Console.WriteLine($"\n{nameof(Level)}\t\t{Level}");
        Console.WriteLine($"{nameof(Distinct)}\t{Distinct.Count}");
        Console.WriteLine($"Total\t\t{Distinct.Values.Sum()}");

        foreach (KeyValuePair<string, int> e in Distinct.OrderByDescending(e => e.Value))
        {
            Console.WriteLine($"#{e.Value}\t{e.Key}");
        }
        listener?.DumpEvents();
    }

    protected override void OnEventSourceCreated(EventSource source)
    {
        if (source.Name is "Microsoft-Windows-DotNETRuntime")
        {
            Level = NextLevel;
            EnableEvents(source, Level, EventKeywords.All);
        }
    }

    protected override void OnEventWritten(EventWrittenEventArgs e)
    {
        string str = $"{e.EventId}\t{e.EventSource.Name}\t{e.EventName}";
        Distinct[str] = Distinct.GetValueOrDefault(str, 0) + 1;
    }
}

@ghost ghost added area-Tracing-coreclr community-contribution Indicates that the PR has been added by a community member labels Jul 21, 2023
@tommcdon
tommcdon requested review from davmason and lateralusX July 24, 2023 17:12
@tommcdon tommcdon added this to the 8.0.0 milestone Jul 24, 2023

@lateralusX lateralusX left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@davmason davmason left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@davmason
davmason merged commit d347315 into dotnet:main Jul 25, 2023
@lateralusX

Copy link
Copy Markdown
Member

Maybe we should add a CLI unit test (under src/tests/tracing) to better cover the different log levels and masks to make sure this doesn't regress? @n77y, something you would be interested in doing?

@nickyMcDonald

Copy link
Copy Markdown
Contributor Author

@lateralusX, I can take a look.

@nickyMcDonald

Copy link
Copy Markdown
Contributor Author

@lateralusX, #89816

@ghost ghost locked as resolved and limited conversation to collaborators Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Tracing-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants