From 57ff3895467e5a8438ad1eaf8909870fbc973469 Mon Sep 17 00:00:00 2001 From: Andrew Omondi Date: Fri, 10 Nov 2023 12:17:42 +0300 Subject: [PATCH 1/5] WIP --- src/KiotaClientFactory.cs | 2 +- ...rosoft.Kiota.Http.HttpClientLibrary.csproj | 2 +- src/Middleware/ActivitySourceRegistry.cs | 37 +++++++++++++++++++ src/Middleware/ChaosHandler.cs | 7 +--- src/Middleware/CompressionHandler.cs | 7 +--- src/Middleware/HeadersInspectionHandler.cs | 7 +--- .../ParametersNameDecodingHandler.cs | 8 +--- src/Middleware/RedirectHandler.cs | 5 +-- src/Middleware/RetryHandler.cs | 5 +-- src/Middleware/UriReplacementHandler.cs | 8 +--- src/Middleware/UserAgentHandler.cs | 7 +--- 11 files changed, 55 insertions(+), 40 deletions(-) create mode 100644 src/Middleware/ActivitySourceRegistry.cs diff --git a/src/KiotaClientFactory.cs b/src/KiotaClientFactory.cs index bc41acf..48686a0 100644 --- a/src/KiotaClientFactory.cs +++ b/src/KiotaClientFactory.cs @@ -65,7 +65,7 @@ public static IList CreateDefaultHandlers() } if(finalHandler != null) handlers[handlers.Length-1].InnerHandler = finalHandler; - return handlers.First(); + return handlers[0];//first } /// /// Creates a to use for the from the provided instances. Order matters. diff --git a/src/Microsoft.Kiota.Http.HttpClientLibrary.csproj b/src/Microsoft.Kiota.Http.HttpClientLibrary.csproj index 96a86dd..45b7a06 100644 --- a/src/Microsoft.Kiota.Http.HttpClientLibrary.csproj +++ b/src/Microsoft.Kiota.Http.HttpClientLibrary.csproj @@ -14,7 +14,7 @@ https://aka.ms/kiota/docs true true - 1.3.0 + 1.3.1 true diff --git a/src/Middleware/ActivitySourceRegistry.cs b/src/Middleware/ActivitySourceRegistry.cs new file mode 100644 index 0000000..f9aa4fd --- /dev/null +++ b/src/Middleware/ActivitySourceRegistry.cs @@ -0,0 +1,37 @@ +// ------------------------------------------------------------------------------ +// Copyright (c) Microsoft Corporation. All Rights Reserved. Licensed under the MIT License. See License in the project root for license information. +// ------------------------------------------------------------------------------ + +using System; +using System.Collections.Concurrent; +using System.Diagnostics; + +namespace Microsoft.Kiota.Http.HttpClientLibrary.Middleware; + +/// +/// Internal static registry for activity sources during tracing. +/// +internal class ActivitySourceRegistry +{ + private readonly ConcurrentDictionary _activitySources = new (); + + /// + /// The default instance of the registry + /// + public static readonly ActivitySourceRegistry DefaultInstance = new(); + + /// + /// Get an a instance with the given name or create one. + /// + /// The name of the + /// + /// When the parameter is null or empty + public ActivitySource GetOrCreateActivitySource(string sourceName) + { + if(string.IsNullOrEmpty(sourceName)) + throw new ArgumentNullException(nameof(sourceName)); + + return _activitySources.GetOrAdd(sourceName, static source => new ActivitySource(source)); + + } +} diff --git a/src/Middleware/ChaosHandler.cs b/src/Middleware/ChaosHandler.cs index 0cc3865..ed791cb 100644 --- a/src/Middleware/ChaosHandler.cs +++ b/src/Middleware/ChaosHandler.cs @@ -56,18 +56,16 @@ protected override async Task SendAsync(HttpRequestMessage // Select global or per request options var chaosHandlerOptions = request.GetRequestOption() ?? _chaosHandlerOptions; - ActivitySource? activitySource; Activity? activity; - if(request.GetRequestOption() is ObservabilityOptions obsOptions) + if(request.GetRequestOption() is { } obsOptions) { - activitySource = new ActivitySource(obsOptions.TracerInstrumentationName); + var activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource(obsOptions.TracerInstrumentationName); activity = activitySource?.StartActivity($"{nameof(ChaosHandler)}_{nameof(SendAsync)}"); activity?.SetTag("com.microsoft.kiota.handler.chaos.enable", true); } else { activity = null; - activitySource = null; } try { @@ -94,7 +92,6 @@ protected override async Task SendAsync(HttpRequestMessage finally { activity?.Dispose(); - activitySource?.Dispose(); } } diff --git a/src/Middleware/CompressionHandler.cs b/src/Middleware/CompressionHandler.cs index 93f1ede..006c99b 100644 --- a/src/Middleware/CompressionHandler.cs +++ b/src/Middleware/CompressionHandler.cs @@ -31,18 +31,16 @@ protected override async Task SendAsync(HttpRequestMessage if(request == null) throw new ArgumentNullException(nameof(request)); - ActivitySource? activitySource; Activity? activity; - if(request.GetRequestOption() is ObservabilityOptions obsOptions) + if(request.GetRequestOption() is { } obsOptions) { - activitySource = new ActivitySource(obsOptions.TracerInstrumentationName); + var activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource(obsOptions.TracerInstrumentationName); activity = activitySource?.StartActivity($"{nameof(CompressionHandler)}_{nameof(SendAsync)}"); activity?.SetTag("com.microsoft.kiota.handler.compression.enable", true); } else { activity = null; - activitySource = null; } try @@ -79,7 +77,6 @@ protected override async Task SendAsync(HttpRequestMessage finally { activity?.Dispose(); - activitySource?.Dispose(); } } diff --git a/src/Middleware/HeadersInspectionHandler.cs b/src/Middleware/HeadersInspectionHandler.cs index 7af0b25..2da2a16 100644 --- a/src/Middleware/HeadersInspectionHandler.cs +++ b/src/Middleware/HeadersInspectionHandler.cs @@ -34,18 +34,16 @@ protected override async Task SendAsync(HttpRequestMessage var options = request.GetRequestOption() ?? _defaultOptions; - ActivitySource? activitySource; Activity? activity; - if(request.GetRequestOption() is ObservabilityOptions obsOptions) + if(request.GetRequestOption() is { } obsOptions) { - activitySource = new ActivitySource(obsOptions.TracerInstrumentationName); + var activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource(obsOptions.TracerInstrumentationName); activity = activitySource?.StartActivity($"{nameof(RedirectHandler)}_{nameof(SendAsync)}"); activity?.SetTag("com.microsoft.kiota.handler.headersInspection.enable", true); } else { activity = null; - activitySource = null; } try { @@ -79,7 +77,6 @@ protected override async Task SendAsync(HttpRequestMessage finally { activity?.Dispose(); - activitySource?.Dispose(); } } diff --git a/src/Middleware/ParametersNameDecodingHandler.cs b/src/Middleware/ParametersNameDecodingHandler.cs index 4daa866..513deb9 100644 --- a/src/Middleware/ParametersNameDecodingHandler.cs +++ b/src/Middleware/ParametersNameDecodingHandler.cs @@ -3,7 +3,6 @@ // ------------------------------------------------------------------------------ using System; -using System.Collections.Generic; using System.Diagnostics; using System.Linq; using System.Net.Http; @@ -35,15 +34,13 @@ public ParametersNameDecodingHandler(ParametersNameDecodingOption? options = def protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { var options = request.GetRequestOption() ?? EncodingOptions; - ActivitySource? activitySource; Activity? activity; - if (request.GetRequestOption() is ObservabilityOptions obsOptions) { - activitySource = new ActivitySource(obsOptions.TracerInstrumentationName); + if (request.GetRequestOption() is { } obsOptions) { + var activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource(obsOptions.TracerInstrumentationName); activity = activitySource.StartActivity($"{nameof(ParametersNameDecodingHandler)}_{nameof(SendAsync)}"); activity?.SetTag("com.microsoft.kiota.handler.parameters_name_decoding.enable", true); } else { activity = null; - activitySource = null; } try { if(!request.RequestUri!.Query.Contains('%') || @@ -60,7 +57,6 @@ protected override Task SendAsync(HttpRequestMessage reques return base.SendAsync(request, cancellationToken); } finally { activity?.Dispose(); - activitySource?.Dispose(); } } internal static string? DecodeUriEncodedString(string? original, char[] charactersToDecode) { diff --git a/src/Middleware/RedirectHandler.cs b/src/Middleware/RedirectHandler.cs index 994cce7..63f3281 100644 --- a/src/Middleware/RedirectHandler.cs +++ b/src/Middleware/RedirectHandler.cs @@ -49,9 +49,9 @@ protected override async Task SendAsync(HttpRequestMessage ActivitySource? activitySource; Activity? activity; - if(request.GetRequestOption() is ObservabilityOptions obsOptions) + if(request.GetRequestOption() is { } obsOptions) { - activitySource = new ActivitySource(obsOptions.TracerInstrumentationName); + activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource(obsOptions.TracerInstrumentationName); activity = activitySource?.StartActivity($"{nameof(RedirectHandler)}_{nameof(SendAsync)}"); activity?.SetTag("com.microsoft.kiota.handler.redirect.enable", true); } @@ -157,7 +157,6 @@ protected override async Task SendAsync(HttpRequestMessage finally { activity?.Dispose(); - activitySource?.Dispose(); } } diff --git a/src/Middleware/RetryHandler.cs b/src/Middleware/RetryHandler.cs index b580e11..56df4ca 100644 --- a/src/Middleware/RetryHandler.cs +++ b/src/Middleware/RetryHandler.cs @@ -55,9 +55,9 @@ protected override async Task SendAsync(HttpRequestMessage var retryOption = request.GetRequestOption() ?? RetryOption; ActivitySource? activitySource; Activity? activity; - if(request.GetRequestOption() is ObservabilityOptions obsOptions) + if(request.GetRequestOption() is { } obsOptions) { - activitySource = new ActivitySource(obsOptions.TracerInstrumentationName); + activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource(obsOptions.TracerInstrumentationName); activity = activitySource?.StartActivity($"{nameof(RetryHandler)}_{nameof(SendAsync)}"); activity?.SetTag("com.microsoft.kiota.handler.retry.enable", true); } @@ -83,7 +83,6 @@ protected override async Task SendAsync(HttpRequestMessage finally { activity?.Dispose(); - activitySource?.Dispose(); } } diff --git a/src/Middleware/UriReplacementHandler.cs b/src/Middleware/UriReplacementHandler.cs index 9c86409..daadd3b 100644 --- a/src/Middleware/UriReplacementHandler.cs +++ b/src/Middleware/UriReplacementHandler.cs @@ -1,7 +1,6 @@ using System.Diagnostics; using System.Net.Http; using System.Threading.Tasks; -using Microsoft.Kiota.Abstractions; using Microsoft.Kiota.Http.HttpClientLibrary.Extensions; using Microsoft.Kiota.Http.HttpClientLibrary.Middleware.Options; @@ -28,18 +27,16 @@ public UriReplacementHandler(TUriReplacementHandlerOption uriReplacement) protected override async Task SendAsync( HttpRequestMessage request, System.Threading.CancellationToken cancellationToken) { - ActivitySource? activitySource; Activity? activity; - if(request.GetRequestOption() is ObservabilityOptions obsOptions) + if(request.GetRequestOption() is { } obsOptions) { - activitySource = new ActivitySource(obsOptions.TracerInstrumentationName); + var activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource(obsOptions.TracerInstrumentationName); activity = activitySource.StartActivity($"{nameof(UriReplacementHandler)}_{nameof(SendAsync)}"); activity?.SetTag("com.microsoft.kiota.handler.uri_replacement.enable", uriReplacement.IsEnabled()); } else { activity = null; - activitySource = null; } try @@ -50,7 +47,6 @@ protected override async Task SendAsync( finally { activity?.Dispose(); - activitySource?.Dispose(); } } } diff --git a/src/Middleware/UserAgentHandler.cs b/src/Middleware/UserAgentHandler.cs index c262194..8b7a867 100644 --- a/src/Middleware/UserAgentHandler.cs +++ b/src/Middleware/UserAgentHandler.cs @@ -30,15 +30,13 @@ protected override Task SendAsync(HttpRequestMessage reques if(request == null) throw new ArgumentNullException(nameof(request)); - ActivitySource? activitySource; Activity? activity; - if (request.GetRequestOption() is ObservabilityOptions obsOptions) { - activitySource = new ActivitySource(obsOptions.TracerInstrumentationName); + if (request.GetRequestOption() is { } obsOptions) { + var activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource(obsOptions.TracerInstrumentationName); activity = activitySource?.StartActivity($"{nameof(UserAgentHandler)}_{nameof(SendAsync)}"); activity?.SetTag("com.microsoft.kiota.handler.useragent.enable", true); } else { activity = null; - activitySource = null; } try { var userAgentHandlerOption = request.GetRequestOption() ?? _userAgentOption; @@ -51,7 +49,6 @@ protected override Task SendAsync(HttpRequestMessage reques return base.SendAsync(request, cancellationToken); } finally { activity?.Dispose(); - activitySource?.Dispose(); } } } From f99c5fb5a3bdc9e914925ac65d80c82470ce43a4 Mon Sep 17 00:00:00 2001 From: Andrew Omondi Date: Fri, 10 Nov 2023 12:34:09 +0300 Subject: [PATCH 2/5] Adds tests --- .../Middleware/ActivitySourceRegistryTests.cs | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/ActivitySourceRegistryTests.cs diff --git a/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/ActivitySourceRegistryTests.cs b/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/ActivitySourceRegistryTests.cs new file mode 100644 index 0000000..02a747a --- /dev/null +++ b/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/ActivitySourceRegistryTests.cs @@ -0,0 +1,44 @@ +using System; +using Microsoft.Kiota.Http.HttpClientLibrary.Middleware; +using Xunit; + +namespace Microsoft.Kiota.Http.HttpClientLibrary.Tests.Middleware.Registries +{ + public class ActivitySourceRegistryTests + { + [Fact] + public void Defensive() + { + Assert.Throws(() => ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource("")); + Assert.Throws(() => ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource(null)); + } + + [Fact] + public void CreatesNewInstanceOnFirstCallAndReturnsSameInstance() + { + // Act + var activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource("sample source"); + Assert.NotNull(activitySource); + + var activitySource2 = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource("sample source"); + Assert.NotNull(activitySource); + + // They are the same instance + Assert.Equal(activitySource, activitySource2); + } + + [Fact] + public void CreatesDifferentInstances() + { + // Act + var activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource("sample source"); + Assert.NotNull(activitySource); + + var activitySource2 = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource("sample source 2"); + Assert.NotNull(activitySource); + + // They are not the same instance + Assert.NotEqual(activitySource, activitySource2); + } + } +} From f6ac9da393aad085d008ca4b3d1b9bdde060de98 Mon Sep 17 00:00:00 2001 From: Andrew Omondi Date: Fri, 10 Nov 2023 12:37:19 +0300 Subject: [PATCH 3/5] Update changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94b5bbe..44f17b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.3.1] - 2023-11-10 + +### Added + +- Fixes multiple initialization of `ActivitySource` instances on each request send(https://github.com/microsoft/kiota-http-dotnet/issues/161). + ## [1.3.0] - 2023-11-02 ### Added From 3afe819e92d862ab65d8a837dbb29cf2661b3bf9 Mon Sep 17 00:00:00 2001 From: Andrew Omondi Date: Fri, 10 Nov 2023 12:41:08 +0300 Subject: [PATCH 4/5] Enhance tests --- .../Middleware/ActivitySourceRegistryTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/ActivitySourceRegistryTests.cs b/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/ActivitySourceRegistryTests.cs index 02a747a..0314d88 100644 --- a/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/ActivitySourceRegistryTests.cs +++ b/Microsoft.Kiota.Http.HttpClientLibrary.Tests/Middleware/ActivitySourceRegistryTests.cs @@ -25,6 +25,8 @@ public void CreatesNewInstanceOnFirstCallAndReturnsSameInstance() // They are the same instance Assert.Equal(activitySource, activitySource2); + Assert.Equal("sample source", activitySource.Name); + Assert.Equal("sample source", activitySource2.Name); } [Fact] @@ -33,9 +35,11 @@ public void CreatesDifferentInstances() // Act var activitySource = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource("sample source"); Assert.NotNull(activitySource); + Assert.Equal("sample source", activitySource.Name); var activitySource2 = ActivitySourceRegistry.DefaultInstance.GetOrCreateActivitySource("sample source 2"); Assert.NotNull(activitySource); + Assert.Equal("sample source 2", activitySource2.Name); // They are not the same instance Assert.NotEqual(activitySource, activitySource2); From 1657eaab1112ccefcf7077c440c901528b468103 Mon Sep 17 00:00:00 2001 From: Vincent Biret Date: Fri, 10 Nov 2023 07:47:34 -0500 Subject: [PATCH 5/5] Update src/Middleware/ActivitySourceRegistry.cs --- src/Middleware/ActivitySourceRegistry.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Middleware/ActivitySourceRegistry.cs b/src/Middleware/ActivitySourceRegistry.cs index f9aa4fd..0cde89c 100644 --- a/src/Middleware/ActivitySourceRegistry.cs +++ b/src/Middleware/ActivitySourceRegistry.cs @@ -13,7 +13,7 @@ namespace Microsoft.Kiota.Http.HttpClientLibrary.Middleware; /// internal class ActivitySourceRegistry { - private readonly ConcurrentDictionary _activitySources = new (); + private readonly ConcurrentDictionary _activitySources = new (StringComparer.OrdinalIgnoreCase); /// /// The default instance of the registry