-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add metrics for caching #66479
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
Add metrics for caching #66479
Changes from all commits
68478c3
1c4d8b5
2a4743a
92c187b
f18b4cf
8f3c13c
d36af3f
d013412
da10ccd
86db1b3
e7be224
fede414
77e390d
4aefd1b
bdd6fb7
c900483
2eef441
e7b8d8f
e149d8f
f446293
1a5c7f4
3e2ce4f
fc10cc0
0cc5d7f
20cc800
b3c5653
663b66f
2a5b0c6
cc641ed
9d2c26a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
// The compiler emits a reference to the internal copy of this type in our non-NETCoreApp assembly | ||
// so we must include a forward to be compatible with libraries compiled against non-NETCoreApp Microsoft.Extensions.Caching.Abstractions | ||
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Runtime.CompilerServices.IsExternalInit))] | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// ------------------------------------------------------------------------------ | ||
// Changes to this file must follow the https://aka.ms/api-review process. | ||
// ------------------------------------------------------------------------------ | ||
|
||
namespace Microsoft.Extensions.Caching.Memory | ||
{ | ||
public partial interface IMemoryCache : System.IDisposable | ||
{ | ||
MemoryCacheStatistics? GetCurrentStatistics() => null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
|
||
namespace Microsoft.Extensions.Caching.Memory | ||
{ | ||
/// <summary> | ||
/// Holds a snapshot of statistics for a memory cache. | ||
/// </summary> | ||
public class MemoryCacheStatistics | ||
{ | ||
/// <summary> | ||
/// Initializes an instance of MemoryCacheStatistics. | ||
/// </summary> | ||
public MemoryCacheStatistics() { } | ||
|
||
/// <summary> | ||
/// Gets the number of <see cref="ICacheEntry" /> instances currently in the memory cache. | ||
/// </summary> | ||
public long CurrentEntryCount { get; init; } | ||
|
||
/// <summary> | ||
/// Gets an estimated sum of all the <see cref="ICacheEntry.Size" /> values currently in the memory cache. | ||
/// </summary> | ||
/// <returns>Returns <see langword="null"/> if size isn't being tracked. The common MemoryCache implementation tracks size whenever a SizeLimit is set on the cache.</returns> | ||
public long? CurrentEstimatedSize { get; init; } | ||
|
||
/// <summary> | ||
/// Gets the total number of cache misses. | ||
/// </summary> | ||
public long TotalMisses { get; init; } | ||
|
||
/// <summary> | ||
/// Gets the total number of cache hits. | ||
/// </summary> | ||
public long TotalHits { get; init; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
// The compiler emits a reference to the internal copy of this type in our non-NETCoreApp assembly | ||
// so we must include a forward to be compatible with libraries compiled against non-NETCoreApp Microsoft.Extensions.Caching.Abstractions | ||
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Runtime.CompilerServices.IsExternalInit))] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,7 @@ public void Compact(double percentage) { } | |
public void Dispose() { } | ||
protected virtual void Dispose(bool disposing) { } | ||
~MemoryCache() { } | ||
public Microsoft.Extensions.Caching.Memory.MemoryCacheStatistics? GetCurrentStatistics() { throw null; } | ||
public void Remove(object key) { } | ||
public bool TryGetValue(object key, out object? result) { throw null; } | ||
} | ||
|
@@ -45,6 +46,7 @@ public MemoryCacheOptions() { } | |
Microsoft.Extensions.Caching.Memory.MemoryCacheOptions Microsoft.Extensions.Options.IOptions<Microsoft.Extensions.Caching.Memory.MemoryCacheOptions>.Value { get { throw null; } } | ||
public long? SizeLimit { get { throw null; } set { } } | ||
public bool TrackLinkedCacheEntries { get { throw null; } set { } } | ||
public bool TrackStatistics { get { throw null; } set { } } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do people think about making the default value of this property true? I suspect most developers, even those writing high performance services, will still probably prefer being able to monitor their cache statistics rather than squeezing out a tiny bit more speed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. during the API review meeting today the ask was to disable it by default to not hurt the perf for those not tracking the stats There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO true would be the better default but I'm not going to die on that hill : ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can disable stats in the benchmark though, this is similar to what we do with logging. That should apply here as well right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the difference is that far more people want logging by default than will want I think defaulting to not tracking makes sense, but I'm not super opinionated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be fine either way, though this would need another API review churn. Would personally prefer to have this PR dealt with as-is and deal with this discussion on a separate issue. cc @sebastienros @adamsitnik do you have preference here of what default to choose? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be my speculation how projects break down:
I'm worried about the experience of people in group (3). I want them to be able to turn on telemetry very easily when they want it. Anything where they have to follow extra instructions or change more lines of code creates friction in that process. If that means a tiny set of projects in group (1) have to have some friction to disable stats they don't care about I think it is the right tradeoff given my guess at the relative sizes of those groups. Certainly if others think my guesses about the group sizes are wrong they might come to a different conclusion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd assume for case 3, when using dotnet-counters there would still be need for code change for setting up counters with the GetCurrentStatistics API and in which case also setting the flag should be not too much churn. But thinking perhaps with OT or AppInsights there is no need for code change and therefore, as you said once those solutions are in for cache metrics retrieval via OT, then it could be that extra instruction for them to also set TrackStatistics to true. |
||
} | ||
public partial class MemoryDistributedCacheOptions : Microsoft.Extensions.Caching.Memory.MemoryCacheOptions | ||
{ | ||
|
Uh oh!
There was an error while loading. Please reload this page.