feat: Event system

[daveleek]

Is your feature request related to a problem? Please describe.
The task to implement Impression Event Data

Describe the solution you'd like
To add support for the Impression Event Data feature to the Unleash .NET SDK

Describe alternatives you've considered

Additional context
I'll start the discussion below with a couple of suggestions for the client facing part of implementation as requested
@sighphyre @chriswk @ivarconr

[daveleek]

Event system client facing API

Proposals for the way client code interacts with the event system.
There's quite a few ways to go about this in .NET, I'll propose the ones I've seen used the most.
Obviously feel free to pick on namings, mix and match implementations, suggest other approaches etc.

Other things of interest to discuss might be async invocation vs synchronous invocation of message handlers/callbacks, should we catch+log+rethrow? catch+log+swallow? etc.

1. Configured in a separate class on property in UnleashSettings initializer

var settings = new UnleashSettings()
{
    AppName = "event-system-client-facing-examples",
    InstanceTag = "instance z",
    UnleashApi = new Uri("http://localhost:4242/api/"),
    CustomHttpHeaders = new Dictionary()
    {
      {"Authorization","API token" }
    },

    //==== This is the part we're adding ====/
    Events = new EventsConfig
    {
        // A. Callback option
        Impression = evt => {  Console.WriteLine($"Impression: { evt.EventType }"); },
        // B. Class/Interface option
        Impression = new ImpressionEventHandler(),
    }
};

Infrastructure stuff

public class EventsConfig
{
    // A. Callback option
    public Action<ImpressionEvent> Impression { get; set; }

    // B. Class/Interface option
    public IEventHandler<ImpressionEvent> Impression { get; set; }
}

public class UnleashSettings
{
    //....

    // Setting added as property and set on initialization
    public EventsConfig Events { get; set; }
}

public class ExpressionEventHandler : IEventHandler<ImpressionEvent>
{
    public void Handle(ImpressionEvent evt)
    {
        Console.WriteLine($"Impression: { evt.EventType }");
    }
}

2. Register-method on UnleashSettings

Generics, marker interface, and inheritance gives us explicit method invoking for client and provides us with the metadata we need to keep track of handlers, but require a few polymorphic workarounds if storing in dictionaries/collections/...

Should this method be called On()? Register()? Any other name you'd rather want?

var settings = new UnleashSettings()
{
    AppName = "event-system-client-facing-examples",
    InstanceTag = "instance z",
    UnleashApi = new Uri("http://localhost:4242/api/"),
    CustomHttpHeaders = new Dictionary()
    {
      {"Authorization","API token" }
    },
};

//==== This is the part we're adding ====/

// A. Callback option
settings.On((ImpressionEvent evt) =>
{
    Console.WriteLine($"Impression: { evt.EventType }");
});

// B. Class/Interface option
settings.On(new ImpressionEventHandler());

Infrastructure stuff

// Marker interface
public interface IUnleashEvent { }

// Add registering methods to UnleashSettings
public class UnleashSettings
{
    //....

    // A. Callback option
    public void On<TEventType>(Action<TEventType> callback) where TEventType : IUnleashEvent {}

    // B. Class/Interface option
    public void On<TEventType>(IEventHandler<TEventType> evtHandler) where TEventType : IUnleashEvent {}
}

public class ExpressionEventHandler : IEventHandler<ImpressionEvent>
{
    public void Handle(ImpressionEvent evt)
    {
        Console.WriteLine($"Impression: { evt.EventType }");
    }
}

3. Configure -method

This has become more and more popular in .NET lately, with .NET Core/5/6 especially when configuring hosting/DI etc.

Essentially a flavour of Alt.1, a callback is called that configures an object passed to it.

var settings = new UnleashSettings()
{
    AppName = "event-system-client-facing-examples",
    InstanceTag = "instance z",
    UnleashApi = new Uri("http://localhost:4242/api/"),
    CustomHttpHeaders = new Dictionary()
    {
      {"Authorization","API token" }
    },
};

//==== This is the part we're adding ====/

settings.ConfigureEvents(evCfg =>
{
    // A. Callback option
    evCfg.Impression = evt => {  Console.WriteLine($"Impression: { evt.EventType }"); };
    // B. Class/Interface option
    evCfg.Impression = new ImpressionEventHandler();
});

Infrastructure stuff

// Add a configure method that takes a callback to UnleashSettings
public class UnleashSettings
{
    //....
    public void ConfigureEvents(Action<EventsConfig> configureCallback) {
        var config = this.EventsConfig ?? new EventsConfig()
        {
            // defaults?
        };

        // Call the provided callback
        configureCallback(config);
        // Stash it
        this.EventsConfig = config;
    }
}

// Store deep down in the depths of UnleashSettings after configuring
public class EventsConfig
{
    // A. Callback option
    public Action<ImpressionEvent> Impression { get; set; }

    // B. Class/Interface option
    public IEventHandler<ImpressionEvent> Impression { get; set; }
}

// No surprises here
public class ExpressionEventHandler : IEventHandler<ImpressionEvent>
{
    public void Handle(ImpressionEvent evt)
    {
        Console.WriteLine($"Impression: { evt.EventType }");
    }
}

[sighphyre]

Cool! This looks promising!

To add some more questions into the mix, another thing we need to consider fairly carefully is backwards compatibility, our docs say we support backwards to .Net 4.5, so whatever we do here, it should work on that (if we need to drop support for 4.5 we can do that but I think we'd need a fairly strong reason to do so.)

On the above note, I really like the 3rd option, it looks nice, and if this is where .NET is going it would be cool to be able to move with the times but backwards compatibility may be a problem there. It's worth checking, I think.

An Unleash client should have similar properties to a good logger when it comes to error handling - that is, don't crash the hosting process. So I think when it comes to error handling, we need to do our best to log and prevent that exception from bubbling. There might be something I haven't considered here, but I don't see a reason to throw/rethrow right now (tell me if I'm crazy).

I see that you've bound the events handler to the settings object in the examples. Is it crazy to do something like:

            var unleashFactory = new UnleashClientFactory();
            var unleash = await unleashFactory.CreateClientAsync(settings, synchronousInitialization: false);
            unleash.ConfigureEvents(evCfg =>
            {
               //something cool
            });

Kinda want to say having the events bound to the Unleash instance is a lot clearer than binding it to the settings object. WDYT?

[chriswk]

Agreed with Simon. This looks promising.

I'm inclined to agree that it seems more logical to bind the event handling/configuring to the unleash object; but I realise I've been away from .NET for a long time so maybe it's more natural to bind it using the settings object. Just as long as we document properly how to do it.

In the js library we make sure the Unleash instance being returned is an EventEmitter, which of course allows the .on('eventType', (event) => {}) syntax which I'm rather fond of. The subscriber interface we have in Java is close, but it's not quite as smooth.

Tbh, what you have here seems about as smooth as what you can expect in a statically typed language.

But yes, we do need to avoid crashing the hosting process, so event handling throwing an exception should still not break the polling nor the host application.

[daveleek]

@sighphyre @chriswk Awesome, thanks that was quick :)

Backwards compatibility

Language feature wise, this should be fairly vanilla and I'd be surprised if it fails to compile in .NET 4.5. But I will have a look at that to make sure. It's more of a coding pattern change I've seen a lot recently especially in .NET Core and later when configuring services and DI and hosting etc.

Crashing/..

First implementation of this will be for impression data events that to me feels very statistics/dashboardy, so I think I'd be surprised indeed as a consumer of this, if something in my bridging code would crash the current request/execution.

Settings vs Unleash instance

I realise I didn't write anything specifically about this, but I'm fully open to moving it. The reason I chose the UnleashSettings object was merely because that's where most of the configuration takes place. It would definitely be closer to where it would be used if scoped to the Unleash instance, and since you both point to it that seems to be the better idea!

In the js library we make sure the Unleash instance being returned is an EventEmitter, which of course allows the .on('eventType', (event) => {}) syntax which I'm rather fond of. The subscriber interface we have in Java is close, but it's not quite as smooth.

An early version of one of the above suggestions had the following format:
.On(Events.Impression, ImpressionEvent evt => { ... }) but was changed when I realised it wouldn't need that enum key as we already had a generics argument for the event type, and felt like a more compile time checking-friendly way. It is however obviously possible to revert back to that as a more explicit option if preferred/is more consistent/in line with how it works in Node and on other platforms

[sighphyre]

Sounds great! Be really great to have something like this in the .NET client!

Yeah, I want to say the suggestion should work in older .NET versions, it's definitely worth checking though, since I don't see any obvious backwards compat tests in this client (something for later probably)

I do think the events look nicer bound to the Unleash instance rather than the settings but I don't have very strong feelings here, I'm very open to you picking what feels right and then documenting that properly

[chriswk]

Again, just reinforcing what Simon said, as long as what you decide is properly documented I trust you to make the best choice 👍

[daveleek]

Thanks @chriswk @sighphyre! I'll have a look and see what i think fits the internal workings the best, and run with that!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[daveleek]

Implemented in #113