[EventListener] We should add a new method ListenEvents()

[John0King]

Description

when I looking this doc https://github.com/dotnet/diagnostics/blob/master/documentation/design-docs/eventcounters.md#sample-code, I find that there is a private field in the construct that this sample code do not use,
after I "correct" the code , the SimpleEventListener won't work. After debug , I find that the method OnEventSourceCreated called before the constructor code, and this is caused by the the base constructor eg. EventCounter()/base() were doing all the work to call OnEventSourceCreated , and the only way to fix is to use event EventSourceCreated.

this behavior is against the normal code execution, that constructor should run first , and cause error behaviors for private readonly field

 public class SimpleEventListener : EventListener
{
        private readonly int _intervalSec;

        public SimpleEventListener()
        {
            _intervalSec = 1;  // this field should be 1 only , but   it also can be 0
        }

         protected override void OnEventSourceCreated(EventSource source)
        {
            var a = _internalSec; //   the a  is  0  when this method call
        }
}

so I think we should add a new method ListenEvents() , to start listen after the instance being created

using(var listener = new SimpleListenner())
{
    listener.ListenEvents()
}

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
Notify danmosemsft if you want to be subscribed.

[tarekgh]

@noahfalk @tommcdon can triage this one as owners.

[tommcdon]

@sywhang @josalem

[noahfalk]

@John0King - Thanks for the suggestion! I agree that callbacks running before the derived constructor runs is confusing. I suspect the largest challenge here is how to handle back-compat. We wouldn't want to make a change that causes existing code which doesn't call ListenEvents() to stop working. Let me know if you have any thoughts on how we could address that problem?

[John0King]

Yes， the challenge is back-compat.
here are the ideas I thought:

1. use some like netstanrd. dll to redirect this type to new assembly and it's new implementation.
2. do introduce the blocking changes, and suggest multiple target for . net 5 and below
3. use a timer to start listen if user not call listen event after the instance been created
4. do introduce the blocking changes, and suggest to use event to support both version

---

because the event do we still need to create our child class? why not create a static method CreateEventListener() to just create a blanking event and use event to Enable and logging the data?

[noahfalk]

If I had to choose between breaking changes and not running the constructor beforehand I would consider not running the constructor to be the lesser evil. Unfortunately the options 1-4 don't appear to avoid the breaking changes - 2 and 4 accept them directly, 1 is breaking if the retargeting applies to any code automatically, and 3 is breaking for any events that would have otherwise been delivered before the timer expires.

CreateEventListener() could be done without a breaking change but down that road seems to be ignoring the existing API and starting over? New API design is an involved process and new/alternate variations of old APIs add complexity. Sometimes it is justified but this issue doesn't appear problematic enough yet that we'd want to do that.

As a potential workaround you could write something like this?

    class SimpleEventListener : EventListener
    {
        private readonly int _intervalSec = 1;

        protected override void OnEventSourceCreated(EventSource eventSource)
        {
            Debug.Assert(_intervalSec == 1);
            base.OnEventSourceCreated(eventSource);
        }
    }

Derived class constructors are run after the base class, but initializers are run beforehand. Sorry it may not be as satisfying as an updated API but hopefully it helps?

[KalleOlaviNiemitalo]

Possible API 5

partial class EventListener
{
    // Change the existing constructor to call the new one.
    protected EventListener() : this(listen: true) {}

    // Calls ListenEvents if listen == true.
    protected EventListener(bool listen);

    // Calls OnEventSourceCreated for existing event sources
    // and starts calling it for future event sources.
    // Throws InvalidOperationException if ListenEvents has
    // already been called, because that suggests the earlier
    // call was made before initialization was complete.
    // Throws ObjectDisposedException if Dispose has been called.
    protected void ListenEvents();
}

Usage:

public class SimpleEventListener : EventListener
{
    private readonly int _intervalSec;

    public SimpleEventListener() : base(listen: false)
    {
        _intervalSec = 1;
        this.ListenEvents();
    }

    protected override void OnEventSourceCreated(EventSource source)
    {
        var a = _internalSec; //  is reliably 1
    }
}

However, if you wanted to offer the same capability to classes derived from SimpleEventListener, then this API would require the following, which looks inconvenient.

public SimpleEventListener(bool listen) : base(listen: false)
{
    _intervalSec = 1;

    if (listen)
    {
        this.ListenEvents();
    }
}

Bad API 6

partial class EventListener
{
    protected virtual void ListenEvents(); // called by constructor
}

Usage:

public class SimpleEventListener : EventListener
{
    private int _intervalSec; // now cannot be readonly

    public SimpleEventListener()
    {
    }

    protected override void ListenEvents()
    {
        _intervalSec = 1; // set here, not in constructor
        base.ListenEvents();
    }

    protected override void OnEventSourceCreated(EventSource source)
    {
        var a = _internalSec; //  is reliably 1
    }
}

However, if you wanted to add a public SimpleEventListener(int intervalSec) constructor, you'd be unable to pass intervalSec to ListenEvents, except via a thread-static field or similar. So I don't think this is going to fly.

[noahfalk]

@KalleOlaviNiemitalo - thanks for the suggestion! I haven't scrutinized in detail but 'Possible API 5' looks like a pretty reasonable solution to me. It keeps complexity fairly low, largely reuses the existing API, and has no apparent breaking changes.

[John0King]

personally, I think auto listen in constructor is not a good idea, I'd like the void ListenEvents() is public , and I also see many api from history that try fix the old problem. for example TranscationScope and async, today async is everywhere but the default option for TranscationScope is not support async.

here's my suggestion:

• if the goals for .Net 6 (for .net 5 it's too late) is 100% back compatible with .net 5 code then tryPossible API 5 but with public , and mark this api [BackCompatibleAttribute] (just a mark , we can also find a txt file and write it down)
• if the goals for .Net7 (or 6) is not 100% back compatible, then let's make a big change to those historical api and make those api work as it should be.

[eatdrinksleepcode]

It appears to me that the only way to use EventListener correctly is to do what AzureEventSourceListener does: save any event sources raised during construction in a temporary list, and then wire them up after construction is done (although AzureEventSourceListener is wiring up those pre-existing sources twice, before and after construction, for reasons I don't understand).

Can this approach be documented? I have spent the better part of a day just trying to figure out how to use EventListener as intended.

[tommcdon]

Moving older issues to the Future milestone as it is unlikely we will have time to address this in the .NET 7 timeframe. Please feel free to move this issue back to .NET 7 if there is active work to address the feature request.

[John0King]

@tommcdon what's your suggestion for using the EventListener if you cut this from v7.0.0 ?

[noahfalk]

For anyone using EventListener in 7.0 (or any earlier release since it was first created) I'd suggest:

• do you have state that needs to be initialized in the constructor prior to receiving events?
  • no -> subscribe to EventSources directly in the OnEventSourceCreated callback.
  • yes -> in OnEventSourceCreated callback put relevant EventSources into an array if your state isn't initialized yet. Once your state has been initialized iterate over the array of cached sources and enable them

[skomis-mm]

Primary constructors come to the rescue along with @noahfalk's approach using fields initializer:

class SimpleEventListener(int intervalSec) : EventListener
{
     private readonly int _intervalSec = intervalSec;

     protected override void OnEventSourceCreated(EventSource eventSource)
     {
         Debug.Assert(_intervalSec == intervalSec);
         base.OnEventSourceCreated(eventSource);
     }
}

Or just reference primary constructor parameter (intervalSec) directly in the OnEventSourceCreated method.