Skip to content
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

[FEATURE] 📞 Events #14

Closed
jodydonetti opened this issue Apr 25, 2021 · 23 comments
Closed

[FEATURE] 📞 Events #14

jodydonetti opened this issue Apr 25, 2021 · 23 comments
Assignees
Labels
enhancement New feature or request

Comments

@jodydonetti
Copy link
Collaborator

jodydonetti commented Apr 25, 2021

Scenario

While playing with the implementation of the backplane (see #11) and talking about adding metrics (see #9) the need emerged to be able to react to core events happening inside FusionCache.

Proposal

The plan is to add a set of core events to FusionCache (like hit, miss, etc) so it will be possible to subscribe to them.

An example of these events (still a draft):

  • Hit: high-level, "the entry was in the cache" (either memory or distributed)
  • Miss: high-level, "the entry was not in the cache" (both memory and distributed)
  • HitMemory: level-specific, the entry was in the memory cache
  • FactoryTimeout: the factory was run, but it timed out
  • FailSafeActivation: fail-safe has been activated
  • etc...

These events will NOT be used for the core flow, that is FusionCache will call subscribers to them but it will NOT need them to function properly or wait for them to return some results.

Also, the order in which subscribers will be called will not be guaranteed (for potential perf optimizations).

Design for Subscribe/Unsubscribe flow

There are different ways to model this flow:

  • keeping the handler around: the ubiquitous StackExchange.Redis package for example uses a model where both the Subscribe and Unsubscribe methods accept the handler simply as a lambda param (see here). This is very simple and lightweight (no extra allocations), but when using it with anonymous methods (created on the fly) it requires storing them in a variable to be able to later unbubscribe
  • disposables: when subscribing to an event you get back an IDisposable that can be used to unsubscribe later on, by simply calling Dispose() on it. This is very straightforward an it allows to simply collect the results of each Subscribe() call, maybe in a list, and later on just call Dispose() on all of them to unsubscrive from them all, but it incurs in an additional allocation (the disposable itself). Maybe a struct implementing IDisposable may alleaviate the allocation cost? Does it feel right?
  • Rx: I used it in the past to handle something like this, with subjects and whatnot. It worked well, uses the same disposable approach if I remember correctly and it also gives you composability, but it seems a little bit overkill for this. Also, I don't know if nowadays it is widly used in the .NET ecosystem 🤷

Thoughts?

Any suggestion is more than welcome.

@jodydonetti jodydonetti added the enhancement New feature or request label Apr 25, 2021
@jodydonetti jodydonetti self-assigned this Apr 25, 2021
@jodydonetti jodydonetti pinned this issue Apr 25, 2021
@JoeShook
Copy link
Contributor

I am having troubles imagining what the disposables option is.
I have events and multicast delegates in my mind.

Like what will be the mechanics of describing the possible events for a plugin to decide what to listen too? I imagine IFusionCache or another interface would define the possible events. I guess if events existed there would be no need for a metrics plugin.

Will the mechanism be multicast delegates under the covers and the subscribe return is just a IDisposable instance of something that would remove the subscriptions on dispose so the "plugin" doesn't keep FusionCache allocated inadvertently?

@jodydonetti
Copy link
Collaborator Author

jodydonetti commented Apr 27, 2021

I am having troubles imagining what the disposables option is.
I have events and multicast delegates in my mind.

Yes, that is basically it, but ideally with more control on what happens if one of the subscribers throws while handling an event + a simpler way to unsubscribe.

Like what will be the mechanics of describing the possible events for a plugin to decide what to listen too? I imagine IFusionCache or another interface would define the possible events.

Yes, there will be events to subscribe to, probably directly on the IFusionCache plugin or in a sub object for better structuring.

I guess if events existed there would be no need for a metrics plugin.

On the contrary! The metrics plugin would be the bridge between these events and the metrics world. A metrics plugin would listen to those events and dispatch them to an AppMetrics or Open Telemetry collector.

Will the mechanism be multicast delegates under the covers and the subscribe return is just a IDisposable instance of something that would remove the subscriptions on dispose so the "plugin" doesn't keep FusionCache allocated inadvertently?

In the case of the disposables approach yes, that would be the idea.
I have not yet decided which approach to use, I'm reading around and talking with people to get a better idea.

Anyway I'm preparing a sample to clarify the approach.

@jodydonetti
Copy link
Collaborator Author

jodydonetti commented Apr 27, 2021

Regarding simply using a standard multicast delegate, take the example here.

What happens when one of the single delegates throws an exception?

As explained here by the great Jon Skeet the other delegates (the ones after the one that have thrown an exception) will not be executed.

In a library with code from different contributors/plugins working togheter you most probably don't want that: you don't want a single plugin to stop other plugins to be called and do their things. Instead you want to call them all and, maybe, catch the exception and log it for later detective work.

So, getting back to the initial example, it should become something like this gist.

Or, without using an explicit delegate type, this version.

So, when you look at this, the multicast part is not used in favor of using the GetInvocationList method and looping over each single handler, executing it directly.

Basically it can be implemented as a simple List<Action<string>> 🤷

Now, I'm not necessarily saying not to use the native delegate composition capabilities, I'm just saying that there is probably not a great advantage in using it.

What do you think?

@JoeShook
Copy link
Contributor

Thanks for the exhaustive response. I was looking around to see if there was a newer technique everyone was up to and I did have the great Jon Skeet's 4th edition book open trying to find inspiration.

I was wondering if we want to do fire and forget when calling the hooks. Not sure. Plus the plugin author will need to do their due diligence concerning benchmarking. I found that the EventSource plugin I experimented with had virtually no overhead.

I like where you are going. Code and experimenting with reveal the final path...

@JoeShook
Copy link
Contributor

JoeShook commented May 1, 2021

I know I am impatient. :) But how is it going?

@jodydonetti
Copy link
Collaborator Author

jodydonetti commented May 2, 2021

I'm on it 😉, still trying out different shapes of a couple of things.
Will push a branch to play with in a day or two.

@jodydonetti
Copy link
Collaborator Author

Welp, it seems I needed a little bit more time 😩
I should be able to push a first version in the we.

@jodydonetti
Copy link
Collaborator Author

jodydonetti commented May 9, 2021

Hi @JoeShook , I just pushed a first release of the events branch https://github.com/jodydonetti/ZiggyCreatures.FusionCache/tree/feature_events

There are still some things to decide and to do of course, but I wanted you to be able to have a first look around.

Now I'm too exhausted, but tomorrow I'll try to put down an explanation for why I'm desiging it this way, what pros/cons I see and how the system can be used.

Just to have a quick idea the usage should look something like this:

// HI-LEVEL "SET"
cache.Events.General.Set += handler;

// HI-LEVEL "MISS"
cache.Events.General.Miss += handler;

// "MISS" ON THE MEMORY LAYER
cache.Events.Memory.Miss += handler;

// "MISS" ON THE DISTRIBUTED LAYER
cache.Events.Distributed.Miss += handler;

// "REMOVE" ON THE DISTRIBUTED LAYER
cache.Events.Distributed.Remove += handler;

The event args for the "HIT" event also contains a bool IsStale prop to be able to know if the data returned was stale or not.
About this, it is still to be decided what should be considered "stale": only data returned when a fail-safe occurred or even during the "throttle" period? I would be willing to say the latter because a fail-safe activation could simply be another event that I'll add, whereas the "stale" flag can in fact be used to know how many requests used a piece of data that was not fresh, but I'm open to other opinions.

The system handles directly the multicast invocation to avoid a single failing handler blocking the others, but an hypothetical exception would be logged for further investigation (the log level used is configurable like the others already present in the global FusionCacheOptions object).
Also, every handler execution is non-blocking (fire-and-forget style) to avoid slowing down the cache because of a poorly implemented handler.
The event handlers' execution behaviour described above is done thanks to this small util method here .

@JoeShook
Copy link
Contributor

Good news! I have a packed week this week. But I should have a good two weeks after that.

@jodydonetti
Copy link
Collaborator Author

Great, so I'll use this time to explain the direction I've currently taken and push some more code to polish everything 👍
Thanks!

@jodydonetti
Copy link
Collaborator Author

jodydonetti commented May 15, 2021

Ok, a little bit of explanation of what I've done so far in the related branch.

Overview

After a bit of reading around it seemed to me that the standard approach to events is, still nowadays, to use events (as in multicast delegates marked with the event keyword) so that is what I did.

In general I've decided to group togheter all the events in a separate class, to avoid having each event popping up with code completion when is not needed, therefore requiring the developer to simply do _cache.Events. ... to be able to work with events.
I also decided to split events for each "layer", that is:

  • memory (the memory layer)
  • distributed (the distributed layer, if any)
  • general (the high-level thing as a whole)

Each of these has its own class with events specific to them, but all of the derives from the same base class (FusionCacheBaseEventsHub) for the common events (hit/miss/set/remove). Also, the general one contains the memory and the distributed ones, so that is easy to code something like _cache.Events.Distributed.CircuitBreakerChange += ... or the likes.

There's a FusionCacheEntryEventArgs event args class used by any event related to an entry, except for the Hit event that uses a more specific FusionCacheEntryHitEventArgs class that includes, on top of the cache key, also a bool IsStale prop that specify if the "hit" was with stale data or not.

Then I sprinkled events' raising calls throughout the codebase, split in each related place (the ones related to the memory layer in the MemoryCacheAccessor class, the ones for the distributed layer in the DistributedCacheAccessor class and the general ones directly in the FusionCache class).

Safe execution of event handlers

I created a little util method (here) to correctly execute event handlers with a couple of specific features:

  • 💣 resilient to handlers' errors: I'm not using the native .NET support for executing a multicast delegate, because that easily breaks in case of one single handler throwing an exception. This util method solves this problem and guarantee the execution of all registered handlers
  • 🕶️ background execution: since an handler can do anything, it can potentially be slow. Therefore I decided to run the handlers in a background thread, to avoid slowing down the entire thing
  • 📃 logging: in case an handler throws an exception it will be logged through the standard logging mechanism already used inside FusionCache, including the ability to configure globally which log level to use

One thing I'm still not convinced about is the fact that I'm doing a Task.Run inside the foreach loop, for every single handler, whereas I could've done only one, with the loop inside of it. That would have saved some resources, but at the cost of a slow handler potentially delaying the execution of the other handlers, which I felt was something more important to guard against.

Perf

Finally I'm doing some perf tuning and testing to avoid new cpu/memory consumption as much as possible, at least when no event handlers are used. FusionCache is already pretty fast and I would like to keep it that way as much as possible (or to be more precise make it even better, see #12 ).

Next steps

If I will not receive blocking opinions I will finish perf tuning, add some docs and publish a new release asap.

Thoughts? Opinions?

@jodydonetti
Copy link
Collaborator Author

@JoeShook have you been able to take a look at this maybe?

@JoeShook
Copy link
Contributor

@jodydonetti wrapping up some other work. But if I don't start this week I will this weekend. I anticipate needing a new repo for the first plugin I will write. I would be named something like one of the following ...

ZiggyCreatures.FusionCache.AppMetrics
ZiggyCreatures.FusionCache.Plugin.AppMetrics

The first plugin is going to be based on AppMetrics name for sure. So maybe you can create a new repo that I can contribute too as a starting point for the first Plugin?

After that I originally was working on another plugin with EventCounters in the name. It was based on EventSource But I might just go straight to the new metrics work MS is working on for .NET 6 in support of the OpenTelemetry specification.

I am pretty excited about getting back to this! I lost some momentum as I context switched away from this for the last few weeks.

@jodydonetti
Copy link
Collaborator Author

jodydonetti commented May 20, 2021

Good, everything makes sense to me!

Right now though I've done the "events" part and not the "plugin" part yet (even though it is quite there in a private branch where I'm designing it) so before proceeding what I would like to know - as soon as you'll have some time to put into it of course - is if the "events" part looks good to you or if you see something wrong/missing, like:

  • the events model: if using native events seems right to you and the "hub" aggregation class feels ok
  • events: maybe some missing events
  • event args: maybe some missing values in the event args
  • etc

I would like to push a first new release with the events, followed by another one right after that with the plugins system.

Thanks!

@JoeShook
Copy link
Contributor

Sorry I left you hanging. FYI I get what you are saying. I should have left plugin out of my sentence. I at least have the code open and keep trying to get back to it. I will let you no hopefully in the next day.

@jodydonetti
Copy link
Collaborator Author

Not sorry at all, it's all our own personal time we're putting in for free and when we can, and I for one surely appreciate that a lot!

@jodydonetti
Copy link
Collaborator Author

Hi @JoeShook , your pr #18 + some more commits of mine have been merged into the related branch.
I've then added all the missing xml comemnts for IntelliSense & friends, and am now adding an Events chapter in the docs.

If you don't find anything else to add to this sprint, I think I'll be able to push a new release in the next few days 🎉

@jodydonetti
Copy link
Collaborator Author

I've just added the docs related to events.

You can take a look at the main one here: if you happen to have some time I would like to know what you think about it, thanks!

@JoeShook
Copy link
Contributor

JoeShook commented Jun 2, 2021 via email

@jodydonetti
Copy link
Collaborator Author

Ok thanks, in the meantime I've just pushed a couple of commits (a new option + a couple of docs changes).

@jodydonetti
Copy link
Collaborator Author

Hey @JoeShook , I've finally published v0.1.4🎉

Thanks again for your help!

@jodydonetti jodydonetti unpinned this issue Jun 5, 2021
@JoeShook
Copy link
Contributor

JoeShook commented Jun 6, 2021

@jodydonetti that is great! Meanwhile I will continue to work on the metrics plugin projects and examples over here. Next up will be Plugins?

And then when the metrics plugins repo is up to the documentation quality of FusionCache we can create an official place for it in the ZiggyCreatures space.

@jodydonetti
Copy link
Collaborator Author

Awesome! I think in a few days I will push a branch with the initial impl of the plugins system, so you'll be able to work directly on that.

Will update you asap.

@jodydonetti jodydonetti changed the title 📞 Events [FEATURE] 📞 Events Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants