Add ICacheMonitor to allow monitoring extensions to be added

[mgoodfellow]

Hi @Turnerj

I had some time so I thought I would have a quick sketch out of what I was thinking the other day regarding #157

I haven't added tests/documentation etc. as it was more for a discussion first, rather than a PR being ready for merge.

Off the top of my head, this would meet the requirements for monitoring layer/key/performance and leaving it up to the consumer to aggregate the event/timing information as they see fit.

Open for discussion, feedback, whatever really. Benchmarks should be performed as well to make sure this isn't hampering performance. I guess the timing doesn't need to happen if no CacheMonitor - so maybe it is better to branch each operation instead of using NoOpCacheMonitor and only instantiate timers if monitoring is enabled.

I pushed the layer name to be exposed via ICacheLayer to avoid reflection being needed to resolve the layer name on each call.

Food for thought anyway.

[Turnerj]

Firstly, this is pretty awesome! I've had a quick look now and there are a lot of things I like and a few things to consider.

The biggest thing for me - it would be great if it was part of the extension system. My main reasoning for this is it keeps the handling of all the functionality that extends Cache Tower in one spot - this is similar to how I plan to add logging later too.

The second part relates to allocations with the timer - it would be interesting to see if we could either pool the timer or work out a way to have a low-allocation variant of it. Having it be on the hot path of a number of these methods could add GC pressure we might want to avoid. This is a problem to handle more down the line when it comes to benchmarking etc.

I'll have a second look at it tomorrow to see if I have any other thoughts but overall, I'm super appreciative of the effort you've put into doing this!

[mgoodfellow]

I was pondering the extensions and using them instead after you spoke about them on the linked issue - However, I see that they are evaluated after the actions are carried out. This means that you would be storing this timer/result within the calling function, and then needing to pass it down into the extension chain. I tried to think of a clean way of accomplishing that, and I failed on a first pass.

Furthermore, it is very useful to know about the hit/miss events per layer - the extensions run after all this has been evaluated, meaning this information isn't easy to convey. You would need to build up a whole "record" of layer/timing/result information, then pass this payload down into a special extension hook. That also adds to allocations as it would be generating lots of small objects.

I felt this is was sufficiently high level that it could slot into the hot paths. My main worry of it currently is the timer allocation as you have also mentioned.

With regards to a pooled timer - I was concerned about multithreading cases - we need to isolate the timer for a caller. However, I wonder if we can just use system ticks and fetch those before and after the calls, and derive the timing information from that. To be honest though - without benchmarking it is all guess work, so I guess we need to make a couple of versions and see what the tradeoffs are on each.

I figured that testing if CacheMonitor == null -> just use the old paths, otherwise if CacheMonitor != null -> allocate timer, and use new paths. Although making more branching logic, this would be faster than the NoOpCacheMonitor approach and give less GC pressure. As a consumer of the library I would take a performance hit in order to have detailed statistics - but if I needed raw performance, I would also have the option to turn it off.

[mgoodfellow]

All that being said, I would be very interested to see what you had in mind for the extensions - I feel like I might have missed something obvious, but I couldn't get my head around a way that make its nice and generic and open enough. This doesn't feel generic, it feels very specific to me.

Appreciate the feedback!

[Turnerj]

As a consumer of the library I would take a performance hit in order to have detailed statistics - but if I needed raw performance, I would also have the option to turn it off.

I reckon we should be able to achieve both - detailed statistics and blazing fast performance.

I'll think about what you've said re extensions, you're likely right but I'm still curious if there is still a neat way to use it without it being too complicated.

Will get back to you tomorrow!

[Turnerj]

So it took a little longer to get back to you but here is my thoughts...

I think it could still be as an extension but it requires a core rewrite of the extension system. Extensions currently work through fixed interfaces that are directly called through the extension container. Instead, most of the extensions could be done through proper C# events and then those could be called directly,

I know that doesn't really seem super relevant here but the idea then could be proper events at the points that make sense for metrics. With that in mind though, here is some more specific feedback to what you've suggested...

• I probably wouldn't capture events/metrics/timing for every layer. A cache hit/miss is most relevant across the entire cache stack. That said, the event arguments itself would contain which layer it actually was retrieved from (likely the instance of the layer).
• The TimingUtils.Time is good syntatically but creates a second overhead of a closure (basically the compiler will create a class just to pass in the variables we access outside of the internal function body). We can avoid this by doing something like TimingUtils.StartTimer() which returns us a timer instance, we do the work and manually end the timer, create the event args and call the event we want.

For example:

var timer = TimingUtils.StartTiming();
// do work
eventName?.Invoke(this, new MyCustomEventArgs(foo, bar, timer.Elapsed);

The MyCustomEventArgs would potentially be a struct too (in my quick benchmarking, it is a little faster and doesn't allocate) but it will mean that event args don't actually inherit from EventArgs (personally I'm not fussed by that - that would be the cost of the highest performance). While it works in .NET 5, I don't know if it is supported that way in .NET Standard 2.0 so would need to look into that more...

Also I had an idea how to get a high precision non-allocating timer. I found out that Stopwatch exposes a GetTimestamp() method - this method calls the same thing that powers the stopwatch internally. Calling it at the start and end of the block we want to measure and subtracting the values gets us the timestamp without the allocations.

In the end, all of this would mean that a CacheStack (potentially ICacheStack too) would expose a variety of events that extensions and everything else can hook into.

Does that make any sense? I'm writing this late at night and am not sure.

[mgoodfellow]

Hey,

Sorry, delay on my end as well - I have been pondering it the last few days though:

I probably wouldn't capture events/metrics/timing for every layer. A cache hit/miss is most relevant across the entire cache stack. That said, the event arguments itself would contain which layer it actually was retrieved from (likely the instance of the layer).

Yup, that makes sense

The TimingUtils.Time is good syntatically but creates a second overhead of a closure (basically the compiler will create a class just to pass in the variables we access outside of the internal function body). We can avoid this by doing something like TimingUtils.StartTimer() which returns us a timer instance, we do the work and manually end the timer, create the event args and call the event we want.

Ah, good point. In fact, going on from your point later, we can have a "global" Stopwatch running, and our TimingUtils.StartTimer() could just GetTimestamp() to get the current timestamp to reduce allocations

In the end, all of this would mean that a CacheStack (potentially ICacheStack too) would expose a variety of events that extensions and everything else can hook into.

The issue with events is that the data flow is one way only - they fire and forget, and can only allow data flow out. This will not be compatible with your existing RedisLockExtension.

My initial feelings around events was thinking its a bit dirty on the API surface (they aren't nicely wrapped up into a component which can be replaced in CacheStack), but the more I have thought about it, the more it makes sense from a speed and performance perspective. Just have to be careful to not end up with loads of (almost duplicate) events because different extensions require different data payloads. I think for the existing setup however, most extensions (e.g. RedisRemoteEviction they just need the cacheKey which would be in the args anyway).

When an extension is hooked into CacheStack, I guess a dispose pattern could hide away the complexity of binding/unbinding events:

public class CacheStackMonitorExtension : IDisposable
{
    private readonly CacheStack _cacheStack;

    public CacheStackMonitorExtension(CacheStack cacheStack) 
    {
        _cacheStack = cacheStack;
        _cacheStack.SomeEvent += LocalHandler;
    }

    public Dispose()
    {
        _cacheStack.SomeEvent -= LocalHandler;
    }
}

This could be managed by a IoC container along with the CacheStack.

[Turnerj]

In the end, all of this would mean that a CacheStack (potentially ICacheStack too) would expose a variety of events that extensions and everything else can hook into.

The issue with events is that the data flow is one way only - they fire and forget, and can only allow data flow out. This will not be compatible with your existing RedisLockExtension.

Yeah, there would still likely need to be extensions closer to the same pattern I've already got for things like the RedisLockExtension but for some others (metrics, logging, RedisRemoteEviction) could be served by well-designed events. (I mean, I kinda have "events" of sorts with ICacheChangeExtension, just that I manually do it)

My initial feelings around events was thinking its a bit dirty on the API surface (they aren't nicely wrapped up into a component which can be replaced in CacheStack), but the more I have thought about it, the more it makes sense from a speed and performance perspective. Just have to be careful to not end up with loads of (almost duplicate) events because different extensions require different data payloads. I think for the existing setup however, most extensions (e.g. RedisRemoteEviction they just need the cacheKey which would be in the args anyway).

Yeah, absolutely. I want to make sure I've thought ahead enough to avoid large breaking changes while also giving flexibility to grow. Even having too many events could end up hurting performance unnecessarily.

When an extension is hooked into CacheStack, I guess a dispose pattern could hide away the complexity of binding/unbinding events:

public class CacheStackMonitorExtension : IDisposable
{
    private readonly CacheStack _cacheStack;

    public CacheStackMonitorExtension(CacheStack cacheStack) 
    {
        _cacheStack = cacheStack;
        _cacheStack.SomeEvent += LocalHandler;
    }

    public Dispose()
    {
        _cacheStack.SomeEvent -= LocalHandler;
    }
}

This could be managed by a IoC container along with the CacheStack.

I wonder how necessary it would be to need to remove the events later. Currently extensions are applied to a CacheStack and stay there for the life of the CacheStack. If the extensions only have the life of the cache stack, I'd think the GC should be able to clean up the events for us (unless there are subtleties of events that I don't know).