From 541dd2ccbb01533ac14b903d84394a02aaf84295 Mon Sep 17 00:00:00 2001 From: Chris Ventura <45495992+nrcventura@users.noreply.github.com> Date: Thu, 7 Sep 2023 08:54:30 -0700 Subject: [PATCH] Fix: Prevent broken traces when HttpClient content headers contain tracing headers. (#1843) (#1888) --- .../Providers/Wrapper/HttpClient/SendAsync.cs | 3 +++ .../Controllers/SecondCallController.cs | 12 ++++++++++- .../HttpClientW3CTestsNetCore.cs | 20 +++++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/HttpClient/SendAsync.cs b/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/HttpClient/SendAsync.cs index 944469389..de9d3deed 100644 --- a/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/HttpClient/SendAsync.cs +++ b/src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/HttpClient/SendAsync.cs @@ -135,6 +135,9 @@ private static void TryAttachHeadersToRequest(IAgent agent, HttpRequestMessage h { var setHeaders = new Action((carrier, key, value) => { + // Content headers should not contain the expected header keys, but sometimes they do, + // and their presence can cause problems downstream by having 2 values for the same key. + carrier.Content?.Headers?.Remove(key); // "Add" will throw if value exists, so we must remove it first carrier.Headers?.Remove(key); carrier.Headers?.Add(key, value); diff --git a/tests/Agent/IntegrationTests/Applications/AspNetCoreDistTracingApplication/Controllers/SecondCallController.cs b/tests/Agent/IntegrationTests/Applications/AspNetCoreDistTracingApplication/Controllers/SecondCallController.cs index 0dffae59c..aa67fd102 100644 --- a/tests/Agent/IntegrationTests/Applications/AspNetCoreDistTracingApplication/Controllers/SecondCallController.cs +++ b/tests/Agent/IntegrationTests/Applications/AspNetCoreDistTracingApplication/Controllers/SecondCallController.cs @@ -18,7 +18,17 @@ public async Task CallNext(string nextUrl) { using (var httpClient = new HttpClient()) { - var result = await httpClient.GetStringAsync(nextUrl); + var requestMessage = new HttpRequestMessage(HttpMethod.Get, nextUrl); + // Purposely include bad traceparent headers to try to break the distributed trace + requestMessage.Headers.TryAddWithoutValidation("traceparent", "bad-traceparent-requestheader"); + requestMessage.Content = new StringContent(string.Empty); + // This is not a valid content header, but there are some cases where this has happened + requestMessage.Content.Headers.TryAddWithoutValidation("traceparent", "bad-traceparent-contentheader"); + + //var result = await httpClient.GetStringAsync(nextUrl); + var response = await httpClient.SendAsync(requestMessage); + var result = await response.Content.ReadAsStringAsync(); + return result; } } diff --git a/tests/Agent/IntegrationTests/IntegrationTests/DistributedTracing/W3CInstrumentationTests/HttpClientW3CTestsNetCore.cs b/tests/Agent/IntegrationTests/IntegrationTests/DistributedTracing/W3CInstrumentationTests/HttpClientW3CTestsNetCore.cs index 11570f662..769815ea8 100644 --- a/tests/Agent/IntegrationTests/IntegrationTests/DistributedTracing/W3CInstrumentationTests/HttpClientW3CTestsNetCore.cs +++ b/tests/Agent/IntegrationTests/IntegrationTests/DistributedTracing/W3CInstrumentationTests/HttpClientW3CTestsNetCore.cs @@ -64,10 +64,15 @@ public void Test() var receiverAppTxEvents = _fixture.SecondCallApplication.AgentLog.GetTransactionEvents().FirstOrDefault(); Assert.NotNull(receiverAppTxEvents); + var lastCallAppTxEvents = _fixture.RemoteApplication.AgentLog.GetTransactionEvents().FirstOrDefault(); + Assert.NotNull(lastCallAppTxEvents); + var senderAppSpanEvents = _fixture.FirstCallApplication.AgentLog.GetSpanEvents(); var receiverAppSpanEvents = _fixture.SecondCallApplication.AgentLog.GetSpanEvents(); + var lastCallAppSpanEvents = _fixture.RemoteApplication.AgentLog.GetSpanEvents(); Assert.Equal(senderAppTxEvent.IntrinsicAttributes["guid"], receiverAppTxEvents.IntrinsicAttributes["parentId"]); + Assert.Equal(receiverAppTxEvents.IntrinsicAttributes["guid"], lastCallAppTxEvents.IntrinsicAttributes["parentId"]); foreach (var span in senderAppSpanEvents) { @@ -79,10 +84,18 @@ public void Test() Assert.Equal(TestTraceId, span.IntrinsicAttributes["traceId"]); } + foreach (var span in lastCallAppSpanEvents) + { + Assert.Equal(TestTraceId, span.IntrinsicAttributes["traceId"]); + } + var senderRootSpanEvent = senderAppSpanEvents.Where(@event => @event?.IntrinsicAttributes?["name"]?.ToString() == "WebTransaction/MVC/FirstCall/CallNext/{nextUrl}").FirstOrDefault(); var externalSpanEvent = senderAppSpanEvents.Where(@event => @event?.IntrinsicAttributes?["name"]?.ToString() == "External/localhost/Stream/GET").FirstOrDefault(); var receiverRootSpanEvent = receiverAppSpanEvents.Where(@event => @event?.IntrinsicAttributes?["name"]?.ToString() == "WebTransaction/MVC/SecondCall/CallNext/{nextUrl}").FirstOrDefault(); + var receiverExternalSpanEvent = receiverAppSpanEvents.Where(@event => @event?.IntrinsicAttributes?["name"]?.ToString() == "External/localhost/Stream/GET").FirstOrDefault(); + + var lastRootSpanEvent = lastCallAppSpanEvents.Where(@event => @event?.IntrinsicAttributes?["name"]?.ToString() == "WebTransaction/MVC/LastCall/CallEnd").FirstOrDefault(); Assert.NotNull(senderRootSpanEvent); Assert.Equal(TestTracingVendors, senderRootSpanEvent.IntrinsicAttributes["tracingVendors"]); @@ -97,6 +110,13 @@ public void Test() Assert.Equal(externalSpanEvent.IntrinsicAttributes["guid"], receiverRootSpanEvent.IntrinsicAttributes["trustedParentId"]); Assert.True(AttributeComparer.IsEqualTo(senderAppTxEvent.IntrinsicAttributes["priority"], receiverRootSpanEvent.IntrinsicAttributes["priority"])); + Assert.NotNull(lastRootSpanEvent); + Assert.Equal(TestTracingVendors, lastRootSpanEvent.IntrinsicAttributes["tracingVendors"]); + Assert.Equal(receiverExternalSpanEvent.IntrinsicAttributes["guid"], lastRootSpanEvent.IntrinsicAttributes["parentId"]); + Assert.Equal(receiverExternalSpanEvent.IntrinsicAttributes["guid"], lastCallAppTxEvents.IntrinsicAttributes["parentSpanId"]); + Assert.Equal(receiverExternalSpanEvent.IntrinsicAttributes["guid"], lastRootSpanEvent.IntrinsicAttributes["trustedParentId"]); + Assert.True(AttributeComparer.IsEqualTo(receiverAppTxEvents.IntrinsicAttributes["priority"], lastRootSpanEvent.IntrinsicAttributes["priority"])); + var senderExpectedMetrics = new List { new Assertions.ExpectedMetric { metricName = @"Supportability/DistributedTrace/CreatePayload/Success", callCount = 1 },