From 027b7a76ea36d2adf682118b72b42b55287acdfa Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Wed, 15 Nov 2023 15:39:17 -0800 Subject: [PATCH] OTel: Fix duplicated attributes and suppression (#40027) * Fix duplicated attributes and suppression --- sdk/core/Azure.Core/samples/Diagnostics.md | 24 +++++----- .../Internal/RequestActivityPolicy.cs | 13 ++---- .../Azure.Core/src/Shared/DiagnosticScope.cs | 9 ++-- .../tests/ClientDiagnosticsTests.Net50.cs | 46 +++++++++++++++---- 4 files changed, 58 insertions(+), 34 deletions(-) diff --git a/sdk/core/Azure.Core/samples/Diagnostics.md b/sdk/core/Azure.Core/samples/Diagnostics.md index 0d8148a85820a..81d604e0c6d7e 100644 --- a/sdk/core/Azure.Core/samples/Diagnostics.md +++ b/sdk/core/Azure.Core/samples/Diagnostics.md @@ -164,18 +164,18 @@ If your are using Azure SDK libraries in ASP.NET Core application consider using ## Distributed tracing -Azure SDKs are instrumented for distributed tracing using OpenTelemetry or ApplicationInsights SDK. +Azure client libraries are instrumented for distributed tracing using OpenTelemetry or ApplicationInsights SDK. Distributed tracing relies on `ActivitySource` and `Activity` primitives defined in .NET. Check out [Adding distributed tracing instrumentation](https://learn.microsoft.com/dotnet/core/diagnostics/distributed-tracing-instrumentation-walkthroughs) guide for more details. -Azure SDKs produce the following kinds of activities: +Azure client libraries produce the following kinds of activities: -- *HTTP calls*: every HTTP call originating from Azure SDKs +- *HTTP calls*: every HTTP call originating from an Azure SDK - *client method calls*: for example, `BlobClient.DownloadTo` or `SecretClient.StartDeleteSecret`. - *messaging events*: Event Hubs and Service Bus message creation is traced and correlated with its sending, receiving, and processing. -Prior to November 2023, OpenTelemetry support was experimental for all Azure SDKs (see [Enabling experimental tracing features](#enabling-experimental-tracing-features) for the details). -Most of Azure SDKs released in or after November 2023 have OpenTelemetry support enabled by default. Tracing support in messaging libraries (`Azure.Messaging.ServiceBus` and `Azure.Messaging.EventHubs`) remains experimental. +Prior to November 2023, OpenTelemetry support was experimental for all Azure client libraries (see [Enabling experimental tracing features](#enabling-experimental-tracing-features) for the details). +Most of Azure client libraries released in or after November 2023 have OpenTelemetry support enabled by default. Tracing support in messaging libraries (`Azure.Messaging.ServiceBus` and `Azure.Messaging.EventHubs`) remains experimental. More detailed distributed tracing convention can be found at [Azure SDK semantic conventions](https://github.com/Azure/azure-sdk/blob/main/docs/tracing/distributed-tracing-conventions.md). @@ -195,7 +195,7 @@ builder.Services.AddOpenTelemetry() .AddOtlpExporter()) ``` -_Note: check out [Enable experimental tracing features](#enabling-experimental-tracing-features) in case you're using one of the messaging SDKs._ +_Note: check out [Enable experimental tracing features](#enabling-experimental-tracing-features) in case you're using one of the messaging libraries._ By calling into `AddSource("Azure.*")` we're telling OpenTelemetry SDK to listen to all sources which name starts with `Azure.`. @@ -210,7 +210,7 @@ _Note: if you use Azure Monitor Distro for ASP.NET Core, you may skip this secti Azure SDK traces all HTTP calls using `Azure.Core.Http` source. If you enable it along with generic HTTP client instrumentation such as `OpenTelemetry.Instrumentation.Http`, it would lead to double-collection of HTTP calls originating from Azure client libraries. -Unlike generic HTTP activities, Azure SDK HTTP activities include Azure-specific attributes such as request identifiers usually passed to and from Azure serices in `x-ms-client-request-id`, `x-ms-request-id` or similar request and respose headers. This data may be important when correlating client and server telemetry or creating support tickets. +Unlike generic HTTP activities, Azure SDK HTTP activities include Azure-specific attributes such as request identifiers usually passed to and from Azure services in `x-ms-client-request-id`, `x-ms-request-id` or similar request and response headers. This data may be important when correlating client and server telemetry or creating support tickets. To avoid double-collection you may either - enrich generic HTTP client activities with Azure request identifiers and disable Azure SDK HTTP activities. @@ -250,7 +250,7 @@ _Note: request identifiers are also stamped on HTTP logs emitted by `Azure.Core` #### Filtering out duplicated HTTP client activities -Another approach would be to filter out generic HTTP client activities with the following approach: +Another approach would be to filter out generic HTTP client activities: ```csharp .WithTracing(tracerProviderBuilder => tracerProviderBuilder @@ -267,7 +267,7 @@ _Note: filtering does not prevent new activity from being created and new [`trac ## HTTP metrics -Azure SDKs do not collect HTTP-level metrics. Please use metrics coming from .NET platform and/or generic HTTP client instrumentation. +Azure client libraries do not collect HTTP-level metrics. Please use metrics coming from .NET platform and/or generic HTTP client instrumentation. ## Azure Monitor @@ -279,7 +279,7 @@ If your application uses ApplicationInsights SDK (classic), automatic collection ### Sample -To see an example of distributed tracing in action, take a look at our [sample app](https://github.com/Azure/azure-sdk-for-net/blob/main/samples/linecounter/README.md) that combines several Azure SDKs. +To see an example of distributed tracing in action, take a look at our [sample app](https://github.com/Azure/azure-sdk-for-net/blob/main/samples/linecounter/README.md) that combines several Azure client libraries. ### Enabling experimental tracing features @@ -335,9 +335,9 @@ var secretClient = new SecretClient(new Uri("https://example.com"), new DefaultA secretClient.GetSecret(""); ``` -## Setting x-ms-client-request-id value sent with requests +## Setting `x-ms-client-request-id` value sent with requests -By default x-ms-client-request-id header gets a unique value per client method call. If you would like to use a specific value for a set of requests use the `HttpPipeline.CreateClientRequestIdScope` method. +By default `x-ms-client-request-id` header gets a unique value per client method call. If you would like to use a specific value for a set of requests use the `HttpPipeline.CreateClientRequestIdScope` method. ```C# Snippet:ClientRequestId var secretClient = new SecretClient(new Uri("http://example.com"), new DefaultAzureCredential()); diff --git a/sdk/core/Azure.Core/src/Pipeline/Internal/RequestActivityPolicy.cs b/sdk/core/Azure.Core/src/Pipeline/Internal/RequestActivityPolicy.cs index 655775f179f63..3c2cba9c51374 100644 --- a/sdk/core/Azure.Core/src/Pipeline/Internal/RequestActivityPolicy.cs +++ b/sdk/core/Azure.Core/src/Pipeline/Internal/RequestActivityPolicy.cs @@ -122,20 +122,13 @@ private async ValueTask ProcessAsync(HttpMessage message, ReadOnlyMemory(name, value!)); #else _tagCollection ??= new ActivityTagsCollection(); - _tagCollection.Add(name, value!); + _tagCollection[name] = value!; #endif } else @@ -550,7 +553,7 @@ private void AddObjectTag(string name, object value) #else if (_activitySource?.HasListeners() == true) { - _currentActivity?.AddTag(name, value); + _currentActivity?.SetTag(name, value); } else { diff --git a/sdk/core/Azure.Core/tests/ClientDiagnosticsTests.Net50.cs b/sdk/core/Azure.Core/tests/ClientDiagnosticsTests.Net50.cs index 40617cd063403..bf3110e0915f3 100644 --- a/sdk/core/Azure.Core/tests/ClientDiagnosticsTests.Net50.cs +++ b/sdk/core/Azure.Core/tests/ClientDiagnosticsTests.Net50.cs @@ -125,6 +125,34 @@ public void StableStartsActivityShortNameSourceActivity() CollectionAssert.Contains(activity.Tags, new KeyValuePair(DiagnosticScope.OpenTelemetrySchemaAttribute, DiagnosticScope.OpenTelemetrySchemaVersion)); } + [Test] + [NonParallelizable] + public void DuplicateTagsAreUpdatedActivitySource() + { + using var activityListener = new TestActivitySourceListener("Azure.Clients.ActivityName"); + DiagnosticScopeFactory clientDiagnostics = new DiagnosticScopeFactory("Azure.Clients", "Microsoft.Azure.Core.Cool.Tests", true, false, true); + + DiagnosticScope scope = clientDiagnostics.CreateScope("ActivityName"); + scope.AddAttribute("key1", "value1"); + scope.AddAttribute("key1", "value2"); + + scope.Start(); + scope.AddAttribute("key2", "value1"); + scope.AddAttribute("key2", "value2"); + + scope.Dispose(); + + Assert.AreEqual(1, activityListener.Activities.Count); + var activity = activityListener.Activities.Dequeue(); + + var key1 = activity.TagObjects.Where(kvp => kvp.Key == "key1").ToList(); + var key2 = activity.TagObjects.Where(kvp => kvp.Key == "key2").ToList(); + Assert.AreEqual(1, key1.Count); + Assert.AreEqual(1, key2.Count); + Assert.AreEqual("value2", key1.Single().Value); + Assert.AreEqual("value2", key2.Single().Value); + } + [TestCase(null)] [TestCase(false)] [TestCase(true)] @@ -176,7 +204,7 @@ public void NestedClientActivitiesSuppressed(int kind, bool expectSuppression) using DiagnosticScope scope = clientDiagnostics.CreateScope("ClientName.ActivityName", (ActivityKind)kind); scope.Start(); - DiagnosticScope nestedScope = clientDiagnostics.CreateScope("ClientName.NestedActivityName", (ActivityKind)kind); + DiagnosticScope nestedScope = clientDiagnostics.CreateScope("ClientName.NestedActivityName", ActivityKind.Client); nestedScope.Start(); if (expectSuppression) { @@ -199,7 +227,7 @@ public void NestedClientActivitiesSuppressed(int kind, bool expectSuppression) [TestCase(ActivityKind.Producer, false)] [TestCase(ActivityKind.Consumer, false)] [NonParallelizable] - public void NestedClientActivitiesSuppressionActivitySource(int kind, bool expectSuppression) + public void NestedInternalActivitiesSuppressionActivitySource(int kind, bool expectSuppression) { using var activityListener = new TestActivitySourceListener("Azure.Clients.ClientName"); @@ -208,7 +236,7 @@ public void NestedClientActivitiesSuppressionActivitySource(int kind, bool expec using DiagnosticScope scope = clientDiagnostics.CreateScope("ClientName.ActivityName", (ActivityKind)kind); scope.Start(); - DiagnosticScope nestedScope = clientDiagnostics.CreateScope("ClientName.NestedActivityName", (ActivityKind)kind); + DiagnosticScope nestedScope = clientDiagnostics.CreateScope("ClientName.NestedActivityName", ActivityKind.Internal); nestedScope.Start(); if (expectSuppression) { @@ -224,12 +252,12 @@ public void NestedClientActivitiesSuppressionActivitySource(int kind, bool expec Assert.AreEqual("ClientName.ActivityName", Activity.Current.OperationName); } - [TestCase(true, true, true)] - [TestCase(true, false, false)] - [TestCase(false, true, true)] - [TestCase(false, false, false)] + [TestCase(true, true)] + [TestCase(true, false)] + [TestCase(false, true)] + [TestCase(false, false)] [NonParallelizable] - public void NestedActivitiesSuppressionConfiguration(bool suppressOuter, bool suppressNested, bool expectSuppression) + public void NestedActivitiesSuppressionConfiguration(bool suppressOuter, bool suppressNested) { using var activityListener = new TestActivitySourceListener("Azure.Clients.ClientName"); DiagnosticScopeFactory clientDiagnostics = new DiagnosticScopeFactory("Azure.Clients", "Microsoft.Azure.Core.Cool.Tests", true, suppressOuter, true); @@ -241,7 +269,7 @@ public void NestedActivitiesSuppressionConfiguration(bool suppressOuter, bool su DiagnosticScope nestedScope = clientDiagnostics2.CreateScope("ClientName.NestedActivityName"); nestedScope.Start(); - if (expectSuppression) + if (suppressOuter && suppressNested) { Assert.IsFalse(nestedScope.IsEnabled); Assert.AreEqual("ClientName.ActivityName", Activity.Current.OperationName);