[Instrumentation.AspNetCore, Instrumentation.Http] Cache activity names

[joegoldman2]

Fixes #1759.

Changes

Introduce a static ConcurrentDictionary to 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:

• [ ] Appropriate CHANGELOG.md updated for non-trivial changes
• [ ] Design discussion issue #
• [ ] Changes in public API reviewed

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 72.10%. Comparing base (71655ce) to head (77319f3). Report is 376 commits behind head on main.

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     

Flag	Coverage Δ
unittests-Exporter.Geneva	53.20% <ø> (?)
unittests-Exporter.InfluxDB	94.65% <ø> (?)
unittests-Exporter.Instana	71.24% <ø> (?)
unittests-Exporter.OneCollector	91.29% <ø> (?)
unittests-Exporter.Stackdriver	75.73% <ø> (?)
unittests-Extensions	79.33% <ø> (?)
unittests-Extensions.AWS	77.24% <ø> (?)
unittests-Extensions.Enrichment	100.00% <ø> (?)
unittests-Instrumentation.AWS	87.56% <ø> (?)
unittests-Instrumentation.AWSLambda	87.96% <ø> (?)
unittests-Instrumentation.AspNetCore	85.27% <ø> (?)
unittests-Instrumentation.ElasticsearchClient	79.87% <ø> (?)
unittests-Instrumentation.EntityFrameworkCore	55.49% <ø> (?)
unittests-Instrumentation.EventCounters	76.36% <ø> (?)
unittests-Instrumentation.GrpcNetClient	79.61% <ø> (?)
unittests-Instrumentation.Hangfire	93.58% <ø> (?)
unittests-Instrumentation.Http	81.08% <ø> (?)
unittests-Instrumentation.Owin	83.43% <ø> (?)
unittests-Instrumentation.Process	100.00% <ø> (?)
unittests-Instrumentation.Quartz	78.94% <ø> (?)
unittests-Instrumentation.Runtime	100.00% <ø> (?)
unittests-Instrumentation.SqlClient	91.89% <ø> (?)
unittests-Instrumentation.StackExchangeRedis	67.02% <ø> (?)
unittests-Instrumentation.Wcf	78.47% <ø> (?)
unittests-PersistentStorage	65.78% <ø> (?)
unittests-Resources.AWS	75.88% <ø> (?)
unittests-Resources.Azure	77.94% <ø> (?)
unittests-Resources.Container	72.41% <ø> (?)
unittests-Resources.Gcp	72.54% <ø> (?)
unittests-Resources.Host	47.67% <ø> (?)
unittests-Resources.Process	81.81% <ø> (?)
unittests-Resources.ProcessRuntime	82.35% <ø> (?)
unittests-Sampler.AWS	87.97% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 325 files with indirect coverage changes

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[joegoldman2]

Re-checking - Can two http methods have the same route values?. if that happens, we will incorrectly set the display name here.

Right, so the cache key should take into account the http method.

How much performance would it gain? The new method seems to have enough if checks to counter the benefit. Plus the additional memory usage.

I did a benchmark and here is the result:

Method	HttpRoute	HttpMethod	Mean	Error	StdDev	Gen0	Allocated
Original		_OTHER	0.9941 ns	0.1721 ns	0.0094 ns	-	-
New_CacheKeyString		_OTHER	1.2971 ns	0.8686 ns	0.0476 ns	-	-
New_CacheKeyTuple		_OTHER	1.3056 ns	0.8290 ns	0.0454 ns	-	-
Original		GET	0.9614 ns	0.5957 ns	0.0327 ns	-	-
New_CacheKeyString		GET	0.8245 ns	1.0305 ns	0.0565 ns	-	-
New_CacheKeyTuple		GET	0.9863 ns	0.0562 ns	0.0031 ns	-	-
Original	/resources/{id}	_OTHER	8.8834 ns	4.1025 ns	0.2249 ns	0.0068	64 B
New_CacheKeyString	/resources/{id}	_OTHER	26.0081 ns	3.1322 ns	0.1717 ns	0.0076	72 B
New_CacheKeyTuple	/resources/{id}	_OTHER	19.5532 ns	7.1305 ns	0.3908 ns	-	-
Original	/resources/{id}	GET	9.0777 ns	4.2050 ns	0.2305 ns	0.0068	64 B
New_CacheKeyString	/resources/{id}	GET	25.6794 ns	3.3270 ns	0.1824 ns	0.0068	64 B
New_CacheKeyTuple	/resources/{id}	GET	17.8230 ns	4.8392 ns	0.2653 ns	-	-

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.

[joegoldman2]

I'm not sure what to do with this PR considering the benchmark result. Any suggestion?

[vishweshbankwar]

@joegoldman2 - Sorry for the delay on this. I will take a look at benchmark and get back.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions[bot]]

Closed as inactive. Feel free to reopen if this PR is still being worked on.