Skip to content

FIX #87983 - Enable events for LogAlways #89326

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

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

nickyMcDonald
Copy link
Contributor

@nickyMcDonald nickyMcDonald commented Jul 21, 2023

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 lateralusX and davmason July 24, 2023 17:12
@tommcdon tommcdon added this to the 8.0.0 milestone Jul 24, 2023
Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@davmason davmason left a comment

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
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
Contributor Author

@lateralusX, I can take a look.

@nickyMcDonald
Copy link
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