From 8add3db43ae54dc2823f22b4a46d32755f501ef1 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 28 Oct 2022 10:48:24 -0700 Subject: [PATCH 1/3] [Http] Fix propagation issues (#3828) * Fix propagation issues with http client instrumentation. * More comments and asserts. * Fixes. * Improve tests. * Test cleanup. * Simplify things. * CHANGELOG update. * Code review. * Lint. * Code review. * Code review. Co-authored-by: Vishwesh Bankwar --- .../CHANGELOG.md | 14 ++-- .../HttpHandlerDiagnosticListener.cs | 33 +++----- .../HttpClientTests.Basic.cs | 75 +++++++++++++++++++ 3 files changed, 94 insertions(+), 28 deletions(-) diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index ce0ac8eb52c..5ece3f84ac8 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -2,7 +2,10 @@ ## Unreleased -* *Breaking change** The `Enrich` callback option has been removed. For better +* Added back `netstandard2.0` target. + ([#3787](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3787)) + +* **Breaking change**: The `Enrich` callback option has been removed. For better usability, it has been replaced by three separate options: In case of `HttpClient` the new options are `EnrichWithHttpRequestMessage`, `EnrichWithHttpResponseMessage` and `EnrichWithException` and in case of @@ -15,11 +18,12 @@ `HttpClient` and `HttpWebRequest`,`HttpWebResponse` and `Exception` in case of `HttpWebRequest`. The separate callbacks make it clear what event triggers them and there is no longer the need to cast the argument to the expected -type. -([#3792](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3792)) + type. + ([#3792](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3792)) -* Added back `netstandard2.0` target. -([#3787](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3787)) +* Fixed an issue which prevented custom propagators from being called on .NET 7+ + runtimes for non-sampled outgoing `HttpClient` spans. + ([#3828](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3828)) ## 1.0.0-rc9.8 diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs index 4a0ed555d9c..6183b62a76a 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs @@ -100,11 +100,7 @@ public void OnStartActivity(Activity activity, object payload) // By this time, samplers have already run and // activity.IsAllDataRequested populated accordingly. - // For .NET7.0 or higher versions, activity is created using activity source - // However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener. - // To prevent processing such activities we first check the source name to confirm if it was created using - // activity source or not. - if (Sdk.SuppressInstrumentation || (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name))) + if (Sdk.SuppressInstrumentation) { return; } @@ -122,6 +118,15 @@ public void OnStartActivity(Activity activity, object payload) textMapPropagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpRequestMessageContextPropagation.HeaderValueSetter); } + // For .NET7.0 or higher versions, activity is created using activity source. + // However the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener. + // To prevent processing such activities we first check the source name to confirm if it was created using + // activity source or not. + if (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name)) + { + activity.IsAllDataRequested = false; + } + // enrich Activity from payload only if sampling decision // is favorable. if (activity.IsAllDataRequested) @@ -171,15 +176,6 @@ public void OnStartActivity(Activity activity, object payload) public void OnStopActivity(Activity activity, object payload) { - // For .NET7.0 or higher versions, activity is created using activity source - // However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener. - // To prevent processing such activities we first check the source name to confirm if it was created using - // activity source or not. - if (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name)) - { - return; - } - if (activity.IsAllDataRequested) { // https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs @@ -229,15 +225,6 @@ public void OnStopActivity(Activity activity, object payload) public void OnException(Activity activity, object payload) { - // For .NET7.0 or higher versions, activity is created using activity source - // However, the framework will fallback to creating activity if the sampler's decision is to drop and there is a active diagnostic listener. - // To prevent processing such activities we first check the source name to confirm if it was created using - // activity source or not. - if (IsNet7OrGreater && string.IsNullOrEmpty(activity.Source.Name)) - { - return; - } - if (activity.IsAllDataRequested) { if (!this.stopExceptionFetcher.TryFetch(payload, out Exception exc) || exc == null) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs index 2716ff7da49..3b2877dc021 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpClientTests.Basic.cs @@ -584,6 +584,81 @@ public async Task HttpClientInstrumentationDoesNotReportExceptionEventOnErrorRes Assert.Empty(exportedItems[0].Events); } + [Theory] + [InlineData(true, true)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public async Task CustomPropagatorCalled(bool sample, bool createParentActivity) + { + ActivityContext parentContext = default; + ActivityContext contextFromPropagator = default; + + var propagator = new Mock(); + propagator.Setup(m => m.Inject(It.IsAny(), It.IsAny(), It.IsAny>())) + .Callback>((context, carrier, setter) => + { + contextFromPropagator = context.ActivityContext; + + setter(carrier, "custom_traceparent", $"00/{contextFromPropagator.TraceId}/{contextFromPropagator.SpanId}/01"); + setter(carrier, "custom_tracestate", contextFromPropagator.TraceState); + }); + + var previousDefaultTextMapPropagator = Propagators.DefaultTextMapPropagator; + Sdk.SetDefaultTextMapPropagator(propagator.Object); + + var exportedItems = new List(); + + using (var traceprovider = Sdk.CreateTracerProviderBuilder() + .AddHttpClientInstrumentation() + .AddInMemoryExporter(exportedItems) + .SetSampler(sample ? new ParentBasedSampler(new AlwaysOnSampler()) : new AlwaysOffSampler()) + .Build()) + { + Activity parent = null; + if (createParentActivity) + { + parent = new Activity("parent") + .SetIdFormat(ActivityIdFormat.W3C) + .Start(); + + parent.TraceStateString = "k1=v1,k2=v2"; + parent.ActivityTraceFlags = ActivityTraceFlags.Recorded; + + parentContext = parent.Context; + } + + var request = new HttpRequestMessage + { + RequestUri = new Uri(this.url), + Method = new HttpMethod("GET"), + }; + + using var c = new HttpClient(); + await c.SendAsync(request); + + parent?.Stop(); + } + + if (!sample) + { + Assert.Empty(exportedItems); + } + else + { + Assert.Single(exportedItems); + } + + // Make sure custom propagator was called. + Assert.True(contextFromPropagator != default); + if (sample) + { + Assert.Equal(contextFromPropagator, exportedItems[0].Context); + } + + Sdk.SetDefaultTextMapPropagator(previousDefaultTextMapPropagator); + } + public void Dispose() { this.serverLifeTime?.Dispose(); From 14794b8022f7c55a9a219e2cbe6aae1565d67d0e Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 28 Oct 2022 11:03:04 -0700 Subject: [PATCH 2/3] Fix typo. (#3838) --- docs/trace/extending-the-sdk/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/trace/extending-the-sdk/README.md b/docs/trace/extending-the-sdk/README.md index da2a6cfae7f..d9324d21812 100644 --- a/docs/trace/extending-the-sdk/README.md +++ b/docs/trace/extending-the-sdk/README.md @@ -274,7 +274,7 @@ being exported. Such a "FilteringProcessor" can be written to toggle the `Activity.Recorded` flag. An example "FilteringProcessor" is shown [here](./MyFilteringProcessor.cs). -When using such a filtering processor if should be registered BEFORE the +When using such a filtering processor it should be registered BEFORE the processor containing the exporter which should be bypassed: ```csharp From c7dff42dbc13f1215db617e007d57c1a971f5918 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Fri, 28 Oct 2022 14:53:11 -0400 Subject: [PATCH 3/3] Minor fixes to logging scope examples (#3837) --- docs/logs/customizing-the-sdk/Program.cs | 16 +++++++++------- .../ConsoleLogRecordExporter.cs | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/docs/logs/customizing-the-sdk/Program.cs b/docs/logs/customizing-the-sdk/Program.cs index b6b3afd07d8..6964a4613d1 100644 --- a/docs/logs/customizing-the-sdk/Program.cs +++ b/docs/logs/customizing-the-sdk/Program.cs @@ -14,8 +14,8 @@ // limitations under the License. // +using System.Collections.Generic; using Microsoft.Extensions.Logging; - using OpenTelemetry.Logs; using OpenTelemetry.Resources; @@ -37,15 +37,17 @@ public static void Main() var logger = loggerFactory.CreateLogger(); - logger.LogInformation("Hello Information"); - logger.LogWarning("Hello Warning"); - logger.LogError("Hello Error"); + logger.LogInformation("Hello from {name} {price}.", "tomato", 2.99); + logger.LogWarning("Hello from {name} {price}.", "tomato", 2.99); + logger.LogError("Hello from {name} {price}.", "tomato", 2.99); // log with scopes - using (logger.BeginScope("operation")) - using (logger.BeginScope("hardware")) + using (logger.BeginScope(new List> + { + new KeyValuePair("store", "Seattle"), + })) { - logger.LogError("{name} is broken.", "refrigerator"); + logger.LogInformation("Hello from {food} {price}.", "tomato", 2.99); } } } diff --git a/src/OpenTelemetry.Exporter.Console/ConsoleLogRecordExporter.cs b/src/OpenTelemetry.Exporter.Console/ConsoleLogRecordExporter.cs index 45f5a5ca545..8cb88b81028 100644 --- a/src/OpenTelemetry.Exporter.Console/ConsoleLogRecordExporter.cs +++ b/src/OpenTelemetry.Exporter.Console/ConsoleLogRecordExporter.cs @@ -97,7 +97,7 @@ public override ExportResult Export(in Batch batch) ? new KeyValuePair("OriginalFormat (a.k.a Body)", listKvp[i].Value) : listKvp[i]; - if (ConsoleTagTransformer.Instance.TryTransformTag(listKvp[i], out var result)) + if (ConsoleTagTransformer.Instance.TryTransformTag(valueToTransform, out var result)) { this.WriteLine($"{string.Empty,-4}{result}"); }