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

Adding metrics via the AppMetrics libraries. #9

Closed
wants to merge 13 commits into from

Conversation

JoeShook
Copy link
Contributor

Metrics are missing from opens source resiliency projects in the .NET ecosystem where in equivalent Java libraries they are common. I think this would be an excellent addition to your library.

I have been experimenting with the counters in one of my applications where I report to InfluxDb and it working very well. I should still add a test project to demo the observability. But wanted to get your feedback before continuing.

Currently tracking the following cache events: HIT, MISS, STALE_HIT, STALE_REFRESH

Below is how I setup an Asp.Net application in startup.cs.

var appMetrics = new MetricsBuilder()
    .Configuration.Configure(
        options =>
        {
            options.DefaultContextLabel = "appMetricsContextLabel";
        })
    .Report.ToInfluxDb(
        options => {
            var filter = new MetricsFilter();
            filter.WhereContext(c => c == "appMetricsContextLabel"); //remove default AppMetrics metrics.
            options.InfluxDb.BaseUri = new Uri($"http://{metricsConfig.Host}:{metricsConfig.Port}");
            options.InfluxDb.Database = "database";
            options.InfluxDb.RetentionPolicy = "autogen";
            options.InfluxDb.UserName = "username";
            options.InfluxDb.Password = "password";
            options.InfluxDb.CreateDataBaseIfNotExists = false;
            options.MetricsOutputFormatter = new MetricsInfluxDbLineProtocolOutputFormatter();
        })
    .Build();

    // Setup listener
    services.AddHostedService<MetricsReporterBackgroundService>(sp =>
    new MetricsReporterBackgroundService(appMetrics, appMetrics.Options, appMetrics.Reporters));

    services.AddFusionCache(options =>
    {
        options.DefaultEntryOptions = new FusionCacheEntryOptions
            {
                Duration = TimeSpan.FromSeconds(queryApiSettings.CacheInSeconds),
                Priority = CacheItemPriority.High
            }
            .SetFailSafe(true, TimeSpan.FromHours(1))
            .SetFactoryTimeouts(TimeSpan.FromMilliseconds(500), TimeSpan.FromSeconds(5));
    },
    metrics: appMetrics,
    cacheName: "FusionCache");

I also have a Metrics branch also where I experimented with adding metrics via MS EventSource and EventCounters. But the counters are not available in .NET Standard 2.0.

@jodydonetti
Copy link
Collaborator

jodydonetti commented Mar 25, 2021

Hi @JoeShook , first of all thanks for your time and effort!

Metrics (and stats) are in fact something I wanted to take a look at right after finishing the 2 features I'm currently on, which I hope will come out soon.

I have to say I'm not that well versed about the current metrics situation in .net (which approach or libraries are the standard, etc) but I'll try to take alook at it as soon as possible, along with your own pr, for which I thank you.

In general I'm not that inclined on taking a hard dependency on another package if not strictly necessary, but maybe this is on of the cases where it does in fact make sense. Another approach may be to model it as an extension/plugin packaged up as an additional nuget package (eg: like the 2 main serializers).

Please note though that I'm currently in the midst of a big push at my daily job, so I cannot guarantee an immediate review.

@jodydonetti jodydonetti self-assigned this Mar 25, 2021
@jodydonetti jodydonetti added the enhancement New feature or request label Mar 25, 2021
@jodydonetti
Copy link
Collaborator

Also I'd like to point out something more trivial: a couple of days ago I erroneously pushed in the main branch a commit containing a change I was working on (TryGetResult<T> became MaybeValue<T>) that should've been pushed onto a different branch to experiment with.
I just reverted it, but I noticed you originally branched after that mistake of mine: sorry for the hassle.

@JoeShook
Copy link
Contributor Author

Super cool that you are interested in metrics. I was hoping you would be. I lover the features in FusionCache and didn't want to write my own from scratch.

In general I'm not that inclined on taking a hard dependency on another package if not strictly necessary, but maybe this is on of the cases where it does in fact make sense. Another approach may be to model it as an extension/plugin packaged up as an additional nuget package (eg: like the 2 main serializers).

Yes, I agree. I like the plugin idea. I will experiment more with this pr and see how well I can adjust the abstraction and facilitate two different implementations.

…FusionCache into appmetrics

# Conflicts:
#	src/ZiggyCreatures.FusionCache/FusionCache.cs
…s. One uses App.Metrics libarary and the other is .NET Core EventSource/EventCounter implementation. Of course the providers should be pushed to there own repos and nuget packages.

Be warned I locally I packed them and consumed them as nuget packages.  But looks like it works well.  I experimented with the FusionCache.AppMetrics package in a project.  But still need to try the FusionCache.EventCounters in a project with multiple caches.  Not sure if what I did will be friendly to multiple caches yet.  But the bench marks for FusionCache.EventCounters is better than FusionCache.AppMetrics.
@jodydonetti
Copy link
Collaborator

jodydonetti commented Mar 28, 2021

Hey @JoeShook since you seem to be quite knowledgeable about metrics, do you know if the AppMetrics project is compatible or is looking into being compatible with the new Open Telemetry initiative?

A little bit more than a month ago the official specification reached the 1.0 landmark so I was wondering how is the situation in general.

This seems to be the official .NET implementation and skimming at the docs I see that the metrics part is still experimental :

image

@JoeShook
Copy link
Contributor Author

Not seeing any evidence of AppMetrics project working on Open Telemetry. I am finding the same things as you, it appears metrics is lagging at the specification level. It will be interesting to see how this all plays out in the very near future as the Metric support plans indicates they will support .NET Core and .NET Framework and cooperating with the .NET runtime team. There is a also this issue on core:: .NET applications are observable and easily diagnosable

I have really only dug into what I know so far in the last month or so. I actually only discovered your caching library because you commented on this issue. Are you also tracking the New Memory cache implementation?

@jodydonetti
Copy link
Collaborator

Are you also tracking the New Memory cache implementation?

No I missed that, thanks!

… in the ZiggyCreatures.Fusion.EventCounters. More work to do on validation but both AppMetrics and EventSource are usable.
Wiring in Item_Count.  Work still in progress.
…to increment and decrement for various events hasn't worked yet. It also made me question if it was the right way to do it. I think if the cache implements Count then that is the way to poll a current count. With the FusionCache.EventCounter I am committing some experimental work.

Friction Points:
  - IMemoryCache does not have a Count property.  I  have to Inject my own MemoryCache.
  - AddFusionCache extension method does not have a IMemoryCache parameter.  Not a big deal
  - Some usage pattern I have in a project involve some undesirable singletons that store IFusionMetrics and now I would have to take on the IMemoryCache also.  Kind of stuck with this pattern for a while.  Modernizing takes time :(.

The experiment results in accurate counts.  Still need to unwire the old way of tracking but this all brings me to a change to the way I inject IFusionMetrics.

My provider pattern of passing IFusionMetrics would be better implemented with a Plugin that is loaded based on assembly scanning at startup.  Then IFusionMetrics and IFusionCache wouldn’t both have to be passed an IMemoryCache.  The FusionCache implementation would give IFusionMetrics the IMemoryCache.   The implementation of IFusionMetrics would code for the existence of a Count property implementation.
…in FusionCache constructor. Need name to name metrics when a project has multiple caches that I want to measure via metrics. Maybe a first class cacheName property on FusionCacheOptions would be a better way to go?

This plugin technique made my consumption side much easier when using the FusionCache.EventCounter Plugin.
…FusionCache into appmetrics

# Conflicts:
#	src/ZiggyCreatures.FusionCache/FusionCache.cs
#	src/ZiggyCreatures.FusionCache/ZiggyCreatures.FusionCache.csproj
#	src/ZiggyCreatures.FusionCache/ZiggyCreatures.FusionCache.xml
…ainst it.

EventCounter plugin must provide a MemoryCache if a user wants ITEM_COUNT counters.
Core unit tests added for metrics that match all the SingleLevelTests tests.  This was good as I found a few places to change where measurements are triggered.
Added two new benchmark tests: one for AppMetrics plugin and one for EventCounters plugin.

Of course, we need to extract the plugins and probably associated benchmark project out into their own GitHub projects.
…etrics_with_plugins branch. For now.

Just trying to get this back to a clean PR state independent of external dependencies.
@JoeShook
Copy link
Contributor Author

JoeShook commented Apr 8, 2021

Hello @jodydonetti . I have cleaned up this PR removing the plugin implementations. Curios to get you take on this effort so far. The metrics are very useful to me at this point. I am starting to look at OTEL also. I would like to continue down that path as well but would like it if we could publish this and two plugins for metrics. I think hosting the plugins under ZiggyCreatures would be good.

So at this point I have created the IFusionMetrics interface, obviously very metrics centric. I wonder if something else more forward looking like IFusionTelemetry would be the way to go. Thinking the tree pillars of OpenTelemetry: Logging, Tracing and Metrics. Then at some point the custom logging and tracing you have in place is first class OpenTelemetry.

Anyway looking forward to your feedback. Thanks.

@jodydonetti
Copy link
Collaborator

Hello @JoeShook thank you very much for your effort, I appreciate it a lot and think it can contribute to FusionCache and the community a lot!

Sorry for being late 😩 but as I said I'm in the finishing phase of a pretty big release at my daily job, which should hopefully happen at the beginning of the next week: after that I'm eager to take a deep look at your contribution to push it forward.

Thanks again, see you at half next week!

@jodydonetti
Copy link
Collaborator

Update: I'm starting to look at this.

@JoeShook
Copy link
Contributor Author

@jodydonetti how is it going? Finding any time for review?

@jodydonetti
Copy link
Collaborator

@JoeShook sorry for the delay, the daily job big release has been quite a ride 😅
Btw now I'm on it, I'll update you in the afternoon with some thoughts on how to proceed!

@jodydonetti
Copy link
Collaborator

jodydonetti commented Apr 24, 2021

Ok, here's an update.

I see the work you've done: first of all thank you for that, really appreciate it 🙏

I think hosting the plugins under ZiggyCreatures would be good.

To me it would be more than welcome! And of course you'll be explicitly listed as a contributor (I'll add a CONTRIBUTORS.md as soon as we'll finish this sprint).

So at this point I have created the IFusionMetrics interface, obviously very metrics centric. I wonder if something else more forward looking like IFusionTelemetry would be the way to go. Thinking the tree pillars of OpenTelemetry: Logging, Tracing and Metrics. Then at some point the custom logging and tracing you have in place is first class OpenTelemetry.

I've thought about this, too: looking at your metrics contribution and the backplane #11 I was doing I'm thinking of a more general purpose approach.

For both of these cases (metrics and backplane) there's the need of some core "events" or similar, so my idea is to extend FusionCache with these events/hooks, so that other pieces of code can be "attached" to a FusionCache instance and would be able to listen to those events and do whatever they like.

Basically a plugin system + a set of events.

I've already created some of them in the past on other projects, and the important thing is to find the right "shape" (api surface area) so that it can be generic enough to be used in different scenarios, but not too generic to be basically useless.

As a first draft I'm thinking about something like this:

public interface IFusionCachePlugin
{
    void OnRegister(IFusionCache cache);
    void OnUnregister(IFusionCache cache);
}

where in the OnRegister method a plugin implementer will receive a cache instance and can subscribe to the events they'd like (and in the OnUnregister they can remove the events subscriptions they've created before, to keep a system clean).

A plugin will be registered with a method on the FusionCache instance like RegisterPlugin(IFusionCachePlugin plugin) (similar to the one for the distributed cache), which will be automatically called in a DI (Dependency Injection) scenario by passing all the IFusionCachePlugin services discovered like here but with potentially multiple implementations.

The auto-registration in the DI ext method would look something like this:

[...]
var plugins = serviceProvider.GetServices<IFusionCachePlugin>();
foreach (var plugin in plugins)
{
    cache.RegisterPlugin(plugin);
}
[...]

In case specific functionalities of a type of plugin may be directly needed by FusionCache, a more specialized plugin type may be created, like this:

public interface IFusionCacheSpecialCasePlugin:
    : IFusionCachePlugin
{
    Task<bool> DoSpecialStuffAsync(int param1, string param2, [etc...]);
}

I'm still playing with the backplane impl, and it could very well fit into this as a normal plugin.

Reading this you may now be wondering (as I did) "why would logging and the distributed cache be directly known by FusionCache, whereas a metrics system would not be?" : the answer to that is that they are both a core part of FusionCache and require strict coordination and perf optimizations because they have a big impact on the system as a whole. Additional functionalities not strictly needed by FusionCache to function properly can instead be more decoupled from the core.

What are your thoughts on all of this?

If you don't see potential problems, I can follow up by creating 2 issues to track the design and development of:

  • a plugin system
  • core events

Once these are in place, you will then be able to try and create a plugin of yours in your projects (since you said you needed that) and see if they work well, without having to use a forked version of FusionCache.

Once that is done and you feel comfortable about that, we can then officially add these plugins here, so that everybody can benefit 🎉

Thanks again, and let me know.

@JoeShook
Copy link
Contributor Author

This all sounds like a good plan. Looking forward to testing this out!

Just to restate the obvious I have two plugins planned for the experiment.

  1. AppMetrics based that just works nice with old framework code and easy to integrate with InfluxDb.
  2. EventSource/EventListener/DiagnosticCounter style.

And now that I peeked at the world of OpenTelemetry obviously I am going to build a plugin for that too. :)

This was referenced Apr 25, 2021
@jodydonetti
Copy link
Collaborator

jodydonetti commented Apr 25, 2021

I created #14 and #15 to track them.

Any suggestion is more than welcome.

@jodydonetti
Copy link
Collaborator

I think we can now close this PR since the work has taken a different shape with plugins.

@jodydonetti jodydonetti closed this Aug 1, 2021
@JoeShook
Copy link
Contributor Author

JoeShook commented Aug 3, 2021

Anyone looking for metrics plugins for Fusioncache can find them in nuget searching for ZiggyCreatures and the source with Examples are in Github.

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

Successfully merging this pull request may close these issues.

3 participants