[Instrumentation.AspNetCore, Instrumentation.Http] Cache activity names #1854
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1854 +/- ##
==========================================
- Coverage 73.91% 72.10% -1.81%
==========================================
Files 267 302 +35
Lines 9615 11175 +1560
==========================================
+ Hits 7107 8058 +951
- Misses 2508 3117 +609 Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -26,6 +27,7 @@ internal sealed class RequestDataHelper | |||
| private const string OtherHttpMethod = "_OTHER"; | |||
|
|
|||
| private static readonly char[] SplitChars = new[] { ',' }; | |||
| private static readonly ConcurrentDictionary<string, string> DisplayNameCache = new ConcurrentDictionary<string, string>(); | |||
There was a problem hiding this comment.
Should be the size limit for this collection?
There was a problem hiding this comment.
@joegoldman2 - What happens if a service receives huge number of requests with invalid routes?
There was a problem hiding this comment.
You're right, the dictionary will grow without limit. Let me try to think about a solution.
There was a problem hiding this comment.
My understanding is that there's no built-in "bounded" collection in .NET. While it's possible to implement a custom one, probably by wrapping ConcurrentDictionary with additional checks, I'm not entirely enthusiastic about it. I'm open to suggestions.
There was a problem hiding this comment.
@joegoldman2 - One possible option is to check the Count of items within the dictionary before adding a new item. The Count could be some pre-defined number. If the Count exceeds that number, then we could simply skip adding items to the Dictionary.
vishweshbankwar
added
comp:instrumentation.aspnetcore
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
github-actions
bot
removed
comp:instrumentation.aspnetcore
|
|
||
| if (DisplayNameCache.Count < DisplayNameCacheSize) | ||
| { | ||
| return DisplayNameCache.GetOrAdd(httpRoute!, $"{namePrefix} {httpRoute}"); |
There was a problem hiding this comment.
Re-checking - Can two http methods have the same route values?. if that happens, we will incorrectly set the display name here.
e.g.
Put /api/document/{id}
Get api/document/{id}
| @@ -84,7 +87,27 @@ public void SetActivityDisplayName(Activity activity, string originalHttpMethod, | |||
| var normalizedHttpMethod = this.GetNormalizedHttpMethod(originalHttpMethod); | |||
| var namePrefix = normalizedHttpMethod == "_OTHER" ? "HTTP" : normalizedHttpMethod; | |||
|
|
|||
| activity.DisplayName = string.IsNullOrEmpty(httpRoute) ? namePrefix : $"{namePrefix} {httpRoute}"; | |||
| activity.DisplayName = GetDisplayName(); | |||
There was a problem hiding this comment.
How much performance would it gain? The new method seems to have enough if checks to counter the benefit. Plus the additional memory usage.
Right, so the cache key should take into account the http method.
I did a benchmark and here is the result:
Benchmark code[MemoryDiagnoser]
[ShortRunJob]
public class Tests
{
private static readonly ConcurrentDictionary<string, string> DisplayNameCache = new ConcurrentDictionary<string, string>();
private static readonly ConcurrentDictionary<(string, string), string> DisplayNameCache2 = new ConcurrentDictionary<(string, string), string>();
private const int DisplayNameCacheSize = 1000;
[Params("", "/resources/{id}")]
public string? HttpRoute { get; set; }
[Params("GET", "_OTHER")]
public string HttpMethod { get; set; }
[Benchmark]
public string Original()
{
var namePrefix = HttpMethod == "_OTHER" ? "HTTP" : HttpMethod;
return string.IsNullOrEmpty(HttpRoute) ? namePrefix : $"{namePrefix} {HttpRoute}";
}
[Benchmark]
public string New_CacheKeyString()
{
var namePrefix = HttpMethod == "_OTHER" ? "HTTP" : HttpMethod;
return GetDisplayName();
string GetDisplayName()
{
if (string.IsNullOrEmpty(HttpRoute))
{
return namePrefix;
}
if (DisplayNameCache.TryGetValue($"{HttpMethod} {HttpRoute!}", out var displayName))
{
return displayName;
}
if (DisplayNameCache.Count < DisplayNameCacheSize)
{
return DisplayNameCache.GetOrAdd($"{HttpMethod} {HttpRoute!}", $"{namePrefix} {HttpRoute}");
}
return $"{namePrefix} {HttpRoute}";
}
}
[Benchmark]
public string New_CacheKeyTuple()
{
var namePrefix = HttpMethod == "_OTHER" ? "HTTP" : HttpMethod;
return GetDisplayName();
string GetDisplayName()
{
if (string.IsNullOrEmpty(HttpRoute))
{
return namePrefix;
}
if (DisplayNameCache2.TryGetValue((HttpMethod, HttpRoute!), out var displayName))
{
return displayName;
}
if (DisplayNameCache2.Count < DisplayNameCacheSize)
{
return DisplayNameCache2.GetOrAdd((HttpMethod, HttpRoute!), $"{namePrefix} {HttpRoute}");
}
return $"{namePrefix} {HttpRoute}";
}
}
}The original version is better in all cases. Either my approach isn't right, or this optimization isn't worth it. |
Kielek
added
comp:instrumentation.aspnetcore
github-actions
bot
removed
comp:instrumentation.aspnetcore
|
I'm not sure what to do with this PR considering the benchmark result. Any suggestion? |
| return displayName; | ||
| } | ||
|
|
||
| if (DisplayNameCache.Count < DisplayNameCacheSize) |
There was a problem hiding this comment.
.Count gets a lock on all buckets, so this won't be efficient on the hot path
There was a problem hiding this comment.
Yes, that's what the benchmark above shows.
|
@joegoldman2 - Sorry for the delay on this. I will take a look at benchmark and get back. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Fixes #1759.
Changes
Introduce a static
ConcurrentDictionaryto cache the activity display name. I don't think it's worth adding an entry in the changelog for this change.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes