[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.

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.