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

Allow developers to use built-in meters for tracking stats on caches #67770

Open
maryamariyan opened this issue Apr 8, 2022 · 7 comments
Open
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Caching feature-request
Milestone

Comments

@maryamariyan
Copy link
Member

maryamariyan commented Apr 8, 2022

Summary

Today, the user has to write their own metrics retrieval system. By adding built-in metrics, the library could hook into existing list of caches, and publish all stats so it could handle all that user would do otherwise in their code to support multiple caches. To help identify caches, MemoryCache would need a Name property to help support the meter scenario.

With the built-in metrics the name is shown per memory cache, and the onus is on user to provide a unique name otherwise the library could add a warning and either pick one or update the duplicated name with a warning.

To learn more check out this gist.

Goal

This issue helps focus on a good user experience for developers who wish to track statistics for multiple memory caches by having built-in meters added to the library. Today getting statistics for multiple caches is possible but requires developer to write their own meter.

Our focus here is to add support for cache names, a built-in Meter, and a default naming convention for the cache created by AddMemoryCache() in M.E.C (tracked in #67769).

NOTE:

This issue needs to also address #67769
For more information refer to #66479 (comment).

What is already available in Preview 4:

GetCurrentStatistics() API (based on #50406) allows app developers to use either event counters or metrics APIs to track statistics for one or more memory caches with code snippets.

With IMemoryCache.GetCurrentStatistics(), the user now has support for the following use cases:

  • One cache with either event counters or metrics APIs
  • Multiple caches with metrics API

Using IMemoryCache.GetCurrentStatistics() for one memory cache

Use AddMemoryCache API to instantiate a single memory cache and via DI get it injected to enable them calling GetCurrentStatistics.

Sample usage/screenshot for event counter:

// when using `services.AddMemoryCache(options => options.TrackStatistics = true);` to instantiate

    [EventSource(Name = "Microsoft-Extensions-Caching-Memory")]
    internal sealed class CachingEventSource : EventSource
    {
        public CachingEventSource(IMemoryCache memoryCache) { _memoryCache = memoryCache; }
        protected override void OnEventCommand(EventCommandEventArgs command)
        {
            if (command.Command == EventCommand.Enable)
            {
                if (_cacheHitsCounter == null)
                {
                    _cacheHitsCounter = new PollingCounter("cache-hits", this, () =>
                        _memoryCache.GetCurrentStatistics().CacheHits)
                    {
                        DisplayName = "Cache hits",
                    };
                }
            }
        }
    }

Helps them view stats below with dotnet-counters tool:

image

Using IMemoryCache.GetCurrentStatistics() for multiple memory caches

In order to get stats for more than one memory cache in the app, the user may use metrics APIs in their own code, so long as they have a way of distinguishing their caches by name or ID:

sample usage/screenshot for multiple caches using metrics APIs

 Meter s_meter = new Meter("Microsoft.Extensions.Caching.Memory.MemoryCache", "1.0.0");
 var cacheHitsMetrics = s_meter.CreateObservableGauge<int>("cache-hits", GetCacheHits);

// metrics callback for cache hits
static IEnumerable<Measurement<int>> GetCacheHits()
{
    return new Measurement<int>[]
    {
            // or measurements could be looped or read from a real queue somewhere:
            new Measurement<int>(mc1.GetCurrentStatistics().CacheHits, new KeyValuePair<string,object>("CacheName", "mc1")),
            new Measurement<int>(mc2.GetCurrentStatistics().CacheHits, new KeyValuePair<string,object>("CacheName", "mc2")),
            new Measurement<int>(mc3.GetCurrentStatistics().CacheHits, new KeyValuePair<string,object>("CacheName", "mc3")),
    };
}

Sample stats with dotnet-counters tool:

image

Each metrics would need to create its own observable gauge (one for hits, then misses, etc.) and each callback function for the gauge iterates through list of caches creating measurements.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 8, 2022
@dotnet-issue-labeler
Copy link

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.

@ghost
Copy link

ghost commented Apr 8, 2022

Tagging subscribers to this area: @dotnet/area-extensions-caching
See info in area-owners.md if you want to be subscribed.

Issue Details

In the first issue in #50406, we added support for adding metrics for IMemoryCache, by adding GetCurrentStatistics() API.

GetCurrentStatistics() API allows app developers to use either event counters or metrics APIs to track statistics with code snippets illustrated in this gist.

Next we want to focus on having a good user experience for developers who wish to track statistics for multiple memory caches built-in to the library. Today getting statistics for multiple caches is possible but requires developer to write their own meter.

Our focus here is to add support for cache names, a built-in Meter, and a default naming convention for the cache created by AddMemoryCache() in M.E.C (tracked in XXX).

Author: maryamariyan
Assignees: -
Labels:

untriaged, area-Extensions-Caching

Milestone: -

@maryamariyan maryamariyan added feature-request api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 8, 2022
@maryamariyan maryamariyan added this to the 7.0.0 milestone Apr 8, 2022
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Apr 8, 2022
@maryamariyan maryamariyan changed the title Allow tracking statistics for multiple instances of MemoryCache built-in in the library Allow developers to track statistics on multiple memory caches by adding built-in meters in the library Apr 8, 2022
@maryamariyan maryamariyan changed the title Allow developers to track statistics on multiple memory caches by adding built-in meters in the library Allow developers to use built-in meters to track statistics on multiple memory caches Apr 8, 2022
@maryamariyan maryamariyan changed the title Allow developers to use built-in meters to track statistics on multiple memory caches Allow developers to use built-in meters for tracking stat on multiple memory caches Apr 8, 2022
@maryamariyan maryamariyan changed the title Allow developers to use built-in meters for tracking stat on multiple memory caches Allow developers to use built-in meters for tracking stat on multiple caches Apr 8, 2022
@maryamariyan maryamariyan changed the title Allow developers to use built-in meters for tracking stat on multiple caches Allow developers to use built-in meters for tracking stats on caches Apr 8, 2022
@supertr0n
Copy link

Some feedback from my recent experimentation in using IMemoryCache.GetCurrentStatistics in a Blazor Server app.

This may or may not be the best place for this feedback, but I was too slow to comment on #50406 😄

I really appreciate the work here to add further metrics for IMemoryCache, this is great stuff!

My comment is around API naming and the possible need to clearly set expectations in docs/announcements regarding this new feature because I think many developers will wrongly assume what type of value CurrentEstimatedSize returns.

I was rather excited to try out this feature, with the mistaken expectation that CurrentEstimatedSize would be returning an estimated size of the cache in bytes rather than the sum of the arbitrary Size values that can be assigned when adding items to the cache.

Knowing that historically it was quite a challenge to calculate an estimation of real world memory usage for the in-memory cache, I was excited at the prospect of finally having a quick way to gauge this! (My fault for not reading the docs / changes closely enough!)

In retrospect, it seems totally understandable that CurrentEstimatedSize returns the total of all cache entries Size properties.

However, I think there are a couple of issues here which I believe will lead a lot of developers (perhaps most) to make the same incorrect assumption as me.

Firstly the use of "Estimated" led me to assume this was a measure of real-world memory usage because it wasn't initially intuitive to me that you could know the exact deterministic count of cache entries via CurrentEntryCount but not also know, at the same point in time, the exact total of the cache item "arbitrary" sizes, so I wrongly assumed from the property name that this would be an estimation of memory usage. (Reading further, I do now understand that CurrentEstimatedSize is necessarily a lagging measure which may not always be in sync).

The second challenge is the use of the terminology Size which elsewhere in the framework does often refer to memory usage in bytes. Eg. System.Diagnostics.Process has properties with a Size suffix which do in fact return a "size" in bytes.

The combination of "estimated" and "size" may lead a lot of developers to assume that this is a memory footprint estimation. (Perhaps with a dose of wishful thinking driving this too...)

Having watched the code review video regarding this, where the estimated size property was initially proposed, I noticed that the first assumption by one of the review team was also that this size property, would be a "bytes" measurement too. (Followed by lengthy discussion/clarification about what the "Size" terminology actually means in the context of IMemoryCache!).

Lastly, there was a comment in #50406 which suggested the addition of an estimated cache size. I'm not sure if this was the "request" that was mentioned (in the video) to support the addition of CurrentEstimatedSize but I think it's important to note that this comment specifically mentioned "estimate of the total cache size in memory" (emphasis mine) so I believe this request was also for a memory footprint estimation and not necessarily the arbitrary Size total.

For me, in a Blazor Server app, where memory usage is a primary concern and I am working on ways to both effectively monitor and regulate this usage, an estimated measurement of real-world memory usage would have been very useful. I do also fully understand why this is difficult to achieve.

In summary, I think the default assumption for most people, when encountering this property for the first time, (based on all of the above) is that CurrentEstimatedSize returns a memory usage estimation in bytes, and the purpose of this comment is just to suggest that any documentation and announcements regarding this new feature (and for any associated counters/meters) should make it very clear up front that this is not the case, just to avoid any potential confusion, unnecessary support questions and mismatch of developer expectations up front!

Having said all that, these new metrics are very useful and a great addition, thank you team!

If this feedback is best left elsewhere, please do let me know.

@eerhardt
Copy link
Member

Thank you, @basecde, for the feedback!

Do you happen to have a suggestion that you think would alleviate the problem? Can you think of a better name that would better describe the property?

@supertr0n
Copy link

supertr0n commented Jun 13, 2022

Well, you've called my bluff there @eerhardt 😄

No, given the historical context and the fact that we already have Size terminology when adding items to the cache, I think CurrentEstimatedSize is appropriate.

On the assumption that it's not possible to provide a property with a real memory usage estimate (and I'm guessing it's not, at least for .NET 7), my concrete suggestion would just be around setting developer expectations carefully in the announcement of this feature for .NET 7 and in accompanying docs.

Specifically, calling out (or reminding) potential consumers of this API that the Size in question is the total "arbitrary Size" value and not an absolute measurement in bytes.

I think there is no problem at all here if the potential confusion is anticipated and expectations are set up-front in the docs!

Here's a suggested tweak to the current feature announcement wording:

Current version:

For preview 4 we added metrics support for IMemoryCache. The main APIs being added for Preview 4 are:

  • MemoryCacheStatistics which holds cache hit/miss/estimated size and count for IMemoryCache

Proposed updated version:

For preview 4 we added metrics support for IMemoryCache. The main APIs being added for Preview 4 are:

  • MemoryCacheStatistics which holds totals for IMemoryCache, namely; cache hits, misses, item count and estimated cache size (the latter being the sum of each cache item's Size property)

If helpful, I'm happy to make concrete wording suggestions for any proposed docs for this feature too (not sure if these are written yet?)

@maryamariyan maryamariyan modified the milestones: 7.0.0, 8.0.0 Jun 16, 2022
@maryamariyan
Copy link
Member Author

@noahfalk @davidfowl as per offline conversation moving this out of 7.0 to first investigate proper design for metrics in DI aware systems in a non static way.

@maryamariyan maryamariyan modified the milestones: 8.0.0, Future Sep 28, 2022
maryamariyan added a commit to maryamariyan/runtime that referenced this issue Nov 18, 2022
This not only helps try out library but also encourages future contribution to issue dotnet#67770.
@julealgon
Copy link

This request has been stale for a while. I assume at this point that this won't hit .NET9 anymore as the window for that has now closed, so this is at least .NET10 timeframe? Or perhaps this is not necessarily tied with the .NET releases since this is all library code inside Microsoft.Extensions.Caching.Memory?

Our team has been struggling a bit with finding optimization opportunities and I stumbled upon this whole memory caching metrics thing after thinking about adding custom metrics to our caches and searching for something native first before doing that.

I landed on that MemoryCacheStatistics class on learn.microsoft and then found this related issue here.

So, my take thus far is that there is an idea to provide native metrics from the library itself, but that's not yet available (basically, this issue here). Thus, in the meantime, the expectation is that consumers would create their own custom instrumentation leveraging the GetCurrentStatistics method and observable gauges (using the modern metrics API).

Assuming we went with our own custom metrics for now, would the team recommend anything in particular? For example, should we consider using a Meter with a specific name for future compatibility with the native meters coming up, or should we actively avoid doing that and instead use our own names for the meter? Same question would go for the instrument names and types: would the team recommend using the names mentioned in the samples/linked gist, or would you suggest each implementor come up with their own naming scheme for now?

The reasons I ask these questions are of course I want to avoid unnecessary clashing when the "official" meters are exposed in case we need something before then, which is looking very likely now that this is seemingly out of scope for v9 of the library.

I'd also like to know if all of this work also applies to HybridCache, since we do intend to switch to that when it goes out of preview I assume in November with the launch of .NET9. I see it has a ReportTagMetrics property which I would assume is directly related to this subject, but the interface is not exactly the same as the one for MemoryCache which uses TrackStatistics instead. Some clarification on this would be very appreciated so we can minimize rework later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Caching feature-request
Projects
None yet
Development

No branches or pull requests

4 participants