search

open-telemetry / opentelemetry-dotnet-contrib

This repository contains set of components extending functionality of the OpenTelemetry .NET SDK. Instrumentation libraries, exporters, and other components can find their home here.
https://opentelemetry.io
Apache License 2.0
421 stars 251 forks source link

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

Open joegoldman2 opened 3 weeks ago

joegoldman2 commented 3 weeks ago

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:

codecov[bot] commented 3 weeks ago

Codecov Report

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

Project coverage is 74.23%. Comparing base (71655ce) to head (aec213e). Report is 340 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/graphs/tree.svg?width=650&height=150&src=pr&token=DG2DEROH83&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ Coverage Diff @@ ## main #1854 +/- ## ========================================== + Coverage 73.91% 74.23% +0.32% ========================================== Files 267 297 +30 Lines 9615 11059 +1444 ========================================== + Hits 7107 8210 +1103 - Misses 2508 2849 +341 ``` | [Flag](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | | |---|---|---| | [unittests-Exporter.Geneva](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `64.78% <ø> (?)` | | | [unittests-Exporter.InfluxDB](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `94.65% <ø> (?)` | | | [unittests-Exporter.Instana](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `68.22% <ø> (?)` | | | [unittests-Exporter.OneCollector](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `91.29% <ø> (?)` | | | [unittests-Exporter.Stackdriver](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `75.73% <ø> (?)` | | | [unittests-Extensions](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `79.83% <ø> (?)` | | | [unittests-Extensions.AWS](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `83.41% <ø> (?)` | | | [unittests-Extensions.Enrichment](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `100.00% <ø> (?)` | | | [unittests-Instrumentation.AWS](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `80.59% <ø> (?)` | | | [unittests-Instrumentation.AWSLambda](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `87.96% <ø> (?)` | | | [unittests-Instrumentation.AspNetCore](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `85.27% <ø> (?)` | | | [unittests-Instrumentation.ElasticsearchClient](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `79.87% <ø> (?)` | | | [unittests-Instrumentation.EntityFrameworkCore](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `55.49% <ø> (?)` | | | [unittests-Instrumentation.EventCounters](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `76.36% <ø> (?)` | | | [unittests-Instrumentation.GrpcNetClient](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `79.61% <ø> (?)` | | | [unittests-Instrumentation.Hangfire](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `93.58% <ø> (?)` | | | [unittests-Instrumentation.Http](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `81.08% <ø> (?)` | | | [unittests-Instrumentation.Process](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `100.00% <ø> (?)` | | | [unittests-Instrumentation.Quartz](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `78.94% <ø> (?)` | | | [unittests-Instrumentation.Runtime](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `100.00% <ø> (?)` | | | [unittests-Instrumentation.SqlClient](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `91.89% <ø> (?)` | | | [unittests-Instrumentation.StackExchangeRedis](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `68.02% <ø> (?)` | | | [unittests-Instrumentation.Wcf](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `48.91% <ø> (?)` | | | [unittests-PersistentStorage](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `65.44% <ø> (?)` | | | [unittests-Resources.AWS](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `76.02% <ø> (?)` | | | [unittests-Resources.Azure](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `78.35% <ø> (?)` | | | [unittests-Resources.Container](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `72.41% <ø> (?)` | | | [unittests-Resources.Gcp](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `72.54% <ø> (?)` | | | [unittests-Resources.Host](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `51.16% <ø> (?)` | | | [unittests-Resources.Process](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `81.81% <ø> (?)` | | | [unittests-Resources.ProcessRuntime](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `72.91% <ø> (?)` | | | [unittests-Sampler.AWS](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `87.97% <ø> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#carryforward-flags-in-the-pull-request-comment) to find out more. [see 303 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet-contrib/pull/1854/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
github-actions[bot] commented 1 week ago

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

joegoldman2 commented 5 days ago

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 ```cs [MemoryDiagnoser] [ShortRunJob] public class Tests { private static readonly ConcurrentDictionary DisplayNameCache = new ConcurrentDictionary(); 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.