From 5e04d601937830a401f47d988d8fba751b0ffa6e Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Thu, 5 Sep 2019 22:21:47 -0700 Subject: [PATCH 1/2] Do not st sampledOut flag if activity is recorded and parentId fixes --- .../HostingDiagnosticListener.cs | 40 ++++++--- .../HostingDiagnosticListenerTest.cs | 81 +++++++++++++------ .../FunctionalTest/RequestCorrelationTests.cs | 4 +- 3 files changed, 87 insertions(+), 38 deletions(-) diff --git a/src/Microsoft.ApplicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs b/src/Microsoft.ApplicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs index ab112b5..fce797e 100644 --- a/src/Microsoft.ApplicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs +++ b/src/Microsoft.ApplicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs @@ -192,7 +192,7 @@ bool traceParentPresent = false; var headers = httpContext.Request.Headers; - // 3 posibilities when TelemetryConfiguration.EnableW3CCorrelation = true + // 3 possibilities when TelemetryConfiguration.EnableW3CCorrelation = true // 1. No incoming headers. originalParentId will be null. Simply use the Activity as such. // 2. Incoming Request-ID Headers. originalParentId will be request-id, but Activity ignores this for ID calculations. // If incoming ID is W3C compatible, ignore current Activity. Create new one with parent set to incoming W3C compatible rootid. @@ -201,13 +201,15 @@ // 3a - 2.XX Need to ignore current Activity, and create new from incoming W3C TraceParent header. // 3b - 3.XX Use Activity as such because 3.XX is W3C Aware. - // Another 3 posibilities when TelemetryConfiguration.EnableW3CCorrelation = false + // Another 3 possibilities when TelemetryConfiguration.EnableW3CCorrelation = false // 1. No incoming headers. originalParentId will be null. Simply use the Activity as such. // 2. Incoming Request-ID Headers. originalParentId will be request-id, Activity uses this for ID calculations. // 3. Incoming TraceParent header. Will simply Ignore W3C headers, and Current Activity used as such. // Attempt to find parent from incoming W3C Headers which 2.XX Hosting is unaware of. - if (this.aspNetCoreMajorVersion != AspNetCoreMajorVersion.Three && currentActivity.IdFormat == ActivityIdFormat.W3C && headers.TryGetValue(W3CConstants.TraceParentHeader, out StringValues traceParentValues) + if (this.aspNetCoreMajorVersion != AspNetCoreMajorVersion.Three + && currentActivity.IdFormat == ActivityIdFormat.W3C + && headers.TryGetValue(W3CConstants.TraceParentHeader, out StringValues traceParentValues) && traceParentValues != StringValues.Empty) { var parentTraceParent = StringUtilities.EnforceMaxLength( @@ -282,7 +284,9 @@ } var requestTelemetry = this.InitializeRequestTelemetry(httpContext, currentActivity, Stopwatch.GetTimestamp(), legacyRootId); - requestTelemetry.Context.Operation.ParentId = originalParentId; + + requestTelemetry.Context.Operation.ParentId = + GetParentId(currentActivity, originalParentId, requestTelemetry.Context.Operation.Id); this.AddAppIdToResponseIfRequired(httpContext, requestTelemetry); } @@ -315,7 +319,6 @@ // 1.XX does not create Activity and SDK is responsible for creating Activity. var activity = new Activity(ActivityCreatedByHostingDiagnosticListener); - string sourceAppId = null; IHeaderDictionary requestHeaders = httpContext.Request.Headers; string originalParentId = null; string legacyRootId = null; @@ -326,8 +329,8 @@ traceParentValues != StringValues.Empty) { var parentTraceParent = StringUtilities.EnforceMaxLength(traceParentValues.First(), InjectionGuardConstants.TraceParentHeaderMaxLength); + activity.SetParentId(parentTraceParent); originalParentId = parentTraceParent; - activity.SetParentId(originalParentId); ReadTraceState(requestHeaders, activity); ReadCorrelationContext(requestHeaders, activity); @@ -367,13 +370,9 @@ activity.Start(); var requestTelemetry = this.InitializeRequestTelemetry(httpContext, activity, timestamp, legacyRootId); - if (this.enableW3CHeaders && sourceAppId != null) - { - requestTelemetry.Source = sourceAppId; - } - // fix parent that may be modified by non-W3C operation correlation - requestTelemetry.Context.Operation.ParentId = originalParentId; + requestTelemetry.Context.Operation.ParentId = + GetParentId(activity, originalParentId, requestTelemetry.Context.Operation.Id); this.AddAppIdToResponseIfRequired(httpContext, requestTelemetry); } @@ -546,6 +545,20 @@ { } + private string GetParentId(Activity activity, string originalParentId, string operationId) + { + if (activity.IdFormat == ActivityIdFormat.W3C && activity.ParentSpanId != default) + { + var parentSpanId = activity.ParentSpanId.ToHexString(); + if (parentSpanId != "0000000000000000") + { + return FormatTelemetryId(operationId, parentSpanId); + } + } + + return originalParentId; + } + private static string ExtractOperationIdFromRequestId(string originalParentId) { if (originalParentId[0] == '|') @@ -718,10 +731,11 @@ { requestTelemetry.Context.Operation.Id = activity.RootId; requestTelemetry.Id = activity.Id; - AspNetCoreEventSource.Instance.RequestTelemetryCreated("Hierrarchical", requestTelemetry.Id, requestTelemetry.Context.Operation.Id); + AspNetCoreEventSource.Instance.RequestTelemetryCreated("Hierarchical", requestTelemetry.Id, requestTelemetry.Context.Operation.Id); } if (this.proactiveSamplingEnabled + && !activity.Recorded && this.configuration != null && !string.IsNullOrEmpty(requestTelemetry.Context.Operation.Id) && SamplingScoreGenerator.GetSamplingScore(requestTelemetry.Context.Operation.Id) >= this.configuration.GetLastObservedSamplingPercentage(requestTelemetry.ItemTypeFlag)) diff --git a/test/Microsoft.ApplicationInsights.AspNetCore.Tests/HostingDiagnosticListenerTest.cs b/test/Microsoft.ApplicationInsights.AspNetCore.Tests/HostingDiagnosticListenerTest.cs index e7bed55..21613f2 100644 --- a/test/Microsoft.ApplicationInsights.AspNetCore.Tests/HostingDiagnosticListenerTest.cs +++ b/test/Microsoft.ApplicationInsights.AspNetCore.Tests/HostingDiagnosticListenerTest.cs @@ -459,7 +459,8 @@ [InlineData(AspNetCoreMajorVersion.One, false)] [InlineData(AspNetCoreMajorVersion.Two, false)] [InlineData(AspNetCoreMajorVersion.Three, false)] - public void RequestWithW3CTraceParentCreateNewActivityAndPopulateRequestTelemetry(AspNetCoreMajorVersion aspNetCoreMajorVersion, bool IsW3C) + public void RequestWithW3CTraceParentCreateNewActivityAndPopulateRequestTelemetry(AspNetCoreMajorVersion aspNetCoreMajorVersion, + bool isW3C) { // Tests Request correlation when incoming request has only Request-ID headers. HttpContext context = CreateContext(HttpRequestScheme, HttpRequestHost, "/Test", method: "POST"); @@ -469,7 +470,7 @@ context.Request.Headers[Microsoft.ApplicationInsights.W3C.W3CConstants.TraceStateHeader] = "w3cprop1=value1, w3cprop2=value2"; context.Request.Headers[RequestResponseHeaders.CorrelationContextHeader] = "prop1=value1, prop2=value2"; - using (var hostingListener = CreateHostingListener(aspNetCoreMajorVersion, isW3C: IsW3C)) + using (var hostingListener = CreateHostingListener(aspNetCoreMajorVersion, isW3C: isW3C)) { HandleRequestBegin(hostingListener, context, 0, aspNetCoreMajorVersion); var activity = Activity.Current; @@ -479,7 +480,7 @@ { Assert.Equal(ActivityCreatedByHostingDiagnosticListener, activity.OperationName); } - else if (aspNetCoreMajorVersion == AspNetCoreMajorVersion.Two && IsW3C) + else if (aspNetCoreMajorVersion == AspNetCoreMajorVersion.Two && isW3C) { Assert.Equal(ActivityCreatedByHostingDiagnosticListener, activity.OperationName); } @@ -489,7 +490,7 @@ Assert.NotEqual(ActivityCreatedByHostingDiagnosticListener, activity.OperationName); } - if (IsW3C) + if (isW3C) { Assert.Equal("w3cprop1=value1, w3cprop2=value2", activity.TraceStateString); } @@ -501,16 +502,16 @@ Assert.Single(sentTelemetry); var requestTelemetry = (RequestTelemetry)this.sentTelemetry.Single(); - if (IsW3C) + if (isW3C) { // parentid populated only in W3C mode - ValidateRequestTelemetry(requestTelemetry, activity, IsW3C, expectedParentId: traceParent, expectedSource: null); + ValidateRequestTelemetry(requestTelemetry, activity, true, expectedParentId: "|4e3083444c10254ba40513c7316332eb.e2a5f830c0ee2c46.", expectedSource: null); Assert.Equal("value1", requestTelemetry.Properties["prop1"]); Assert.Equal("value2", requestTelemetry.Properties["prop2"]); } else { - ValidateRequestTelemetry(requestTelemetry, activity, IsW3C, expectedParentId: null, expectedSource: null); + ValidateRequestTelemetry(requestTelemetry, activity, false, expectedParentId: null, expectedSource: null); } } } @@ -522,7 +523,9 @@ [InlineData(AspNetCoreMajorVersion.One, false)] [InlineData(AspNetCoreMajorVersion.Two, false)] [InlineData(AspNetCoreMajorVersion.Three, false)] - public void RequestWithW3CTraceParentButInvalidEntryCreateNewActivityAndPopulateRequestTelemetry(AspNetCoreMajorVersion aspNetCoreMajorVersion, bool IsW3C) + public void RequestWithW3CTraceParentButInvalidEntryCreateNewActivityAndPopulateRequestTelemetry( + AspNetCoreMajorVersion aspNetCoreMajorVersion, + bool isW3C) { // Tests Request correlation when incoming request has only Request-ID headers. HttpContext context = CreateContext(HttpRequestScheme, HttpRequestHost, "/Test", method: "POST"); @@ -531,14 +534,14 @@ context.Request.Headers[Microsoft.ApplicationInsights.W3C.W3CConstants.TraceParentHeader] = traceParent; context.Request.Headers[Microsoft.ApplicationInsights.W3C.W3CConstants.TraceStateHeader] = "prop1=value1, prop2=value2"; - using (var hostingListener = CreateHostingListener(aspNetCoreMajorVersion, isW3C: IsW3C)) + using (var hostingListener = CreateHostingListener(aspNetCoreMajorVersion, isW3C: isW3C)) { HandleRequestBegin(hostingListener, context, 0, aspNetCoreMajorVersion); var activity = Activity.Current; Assert.NotNull(activity); Assert.NotEqual(traceParent, activity.Id); - if (IsW3C) + if (isW3C) { Assert.Equal("prop1=value1, prop2=value2", activity.TraceStateString); } @@ -550,14 +553,14 @@ Assert.Single(sentTelemetry); var requestTelemetry = (RequestTelemetry)this.sentTelemetry.Single(); - if (IsW3C) + if (isW3C) { // parentid populated only in W3C mode - ValidateRequestTelemetry(requestTelemetry, activity, IsW3C, expectedParentId: traceParent, expectedSource: null); + ValidateRequestTelemetry(requestTelemetry, activity, true, expectedParentId: traceParent, expectedSource: null); } else { - ValidateRequestTelemetry(requestTelemetry, activity, IsW3C, expectedParentId: null, expectedSource: null); + ValidateRequestTelemetry(requestTelemetry, activity, false, expectedParentId: null, expectedSource: null); } } } @@ -598,7 +601,7 @@ if (IsW3C) { Assert.Equal("4e3083444c10254ba40513c7316332eb", requestTelemetry.Context.Operation.Id); - ValidateRequestTelemetry(requestTelemetry, activity, IsW3C, expectedParentId: traceParent, expectedSource:null); + ValidateRequestTelemetry(requestTelemetry, activity, IsW3C, expectedParentId: "|4e3083444c10254ba40513c7316332eb.e2a5f830c0ee2c46.", expectedSource:null); } else { @@ -614,17 +617,22 @@ [Theory] [InlineData(true)] [InlineData(false)] - public void OnHttpRequestInStartInitializeTelemetryIfActivityParentIdIsNotNull(bool IsW3C) + public void OnHttpRequestInStartInitializeTelemetryIfActivityParentIdIsNotNull(bool isW3C) { var context = CreateContext(HttpRequestScheme, HttpRequestHost, "/Test", method: "POST"); - string parentId = "|8ee8641cbdd8dd280d239fa2121c7e4e.df07da90a5b27d93."; + + var parent = "|8ee8641cbdd8dd280d239fa2121c7e4e.df07da90a5b27d93."; + context.Request.Headers[RequestResponseHeaders.RequestIdHeader] = parent; + Activity activity; Activity activityBySDK; - using (var hostingListener = CreateHostingListener(AspNetCoreMajorVersion.Two, isW3C: IsW3C)) + using (var hostingListener = CreateHostingListener(AspNetCoreMajorVersion.Two, isW3C: isW3C)) { activity = new Activity("operation"); - activity.SetParentId(parentId); + + // pretend ASP.NET Core read it + activity.SetParentId(parent); activity.AddBaggage("item1", "value1"); activity.AddBaggage("item2", "value2"); @@ -639,9 +647,8 @@ Assert.Single(sentTelemetry); var requestTelemetry = this.sentTelemetry.First() as RequestTelemetry; - ValidateRequestTelemetry(requestTelemetry, activityBySDK, IsW3C, expectedParentId: parentId); + ValidateRequestTelemetry(requestTelemetry, activityBySDK, isW3C, expectedParentId: "|8ee8641cbdd8dd280d239fa2121c7e4e.df07da90a5b27d93."); Assert.Equal("8ee8641cbdd8dd280d239fa2121c7e4e", requestTelemetry.Context.Operation.Id); - Assert.Equal(requestTelemetry.Context.Operation.ParentId, activity.ParentId); Assert.Equal(requestTelemetry.Properties.Count, activity.Baggage.Count()); foreach (var prop in activity.Baggage) @@ -1014,7 +1021,35 @@ [InlineData(AspNetCoreMajorVersion.One)] [InlineData(AspNetCoreMajorVersion.Two)] [InlineData(AspNetCoreMajorVersion.Three)] - public void RequestTelemetryIsNotProactivelySampledOutIfFeatureFlasIfOff(AspNetCoreMajorVersion aspNetCoreMajorVersion) + public void RequestTelemetryIsProactivelySampledInIfFeatureFlagIsOnButActivityIsRecorded(AspNetCoreMajorVersion aspNetCoreMajorVersion) + { + TelemetryConfiguration config = TelemetryConfiguration.CreateDefault(); + config.ExperimentalFeatures.Add("proactiveSampling"); + config.SetLastObservedSamplingPercentage(SamplingTelemetryItemTypes.Request, 0); + + HttpContext context = CreateContext(HttpRequestScheme, HttpRequestHost, "/Test", method: "POST"); + var traceParent = "00-4e3083444c10254ba40513c7316332eb-e2a5f830c0ee2c46-01"; + context.Request.Headers[Microsoft.ApplicationInsights.W3C.W3CConstants.TraceParentHeader] = traceParent; + + using (var hostingListener = CreateHostingListener(aspNetCoreMajorVersion, config, true)) + { + HandleRequestBegin(hostingListener, context, 0, aspNetCoreMajorVersion); + + Assert.NotNull(Activity.Current); + Assert.True(Activity.Current.Recorded); + + var requestTelemetry = context.Features.Get(); + Assert.NotNull(requestTelemetry); + Assert.False(requestTelemetry.IsSampledOutAtHead); + ValidateRequestTelemetry(requestTelemetry, Activity.Current, true, "|4e3083444c10254ba40513c7316332eb.e2a5f830c0ee2c46."); + } + } + + [Theory] + [InlineData(AspNetCoreMajorVersion.One)] + [InlineData(AspNetCoreMajorVersion.Two)] + [InlineData(AspNetCoreMajorVersion.Three)] + public void RequestTelemetryIsNotProactivelySampledOutIfFeatureFlagIfOff(AspNetCoreMajorVersion aspNetCoreMajorVersion) { TelemetryConfiguration config = TelemetryConfiguration.CreateDefault(); config.SetLastObservedSamplingPercentage(SamplingTelemetryItemTypes.Request, 0); @@ -1131,12 +1166,12 @@ return string.Concat("|", traceId, ".", spanId, "."); } - private void ValidateRequestTelemetry(RequestTelemetry requestTelemetry, Activity activity, bool IsW3C, string expectedParentId = null, string expectedSource = null) + private void ValidateRequestTelemetry(RequestTelemetry requestTelemetry, Activity activity, bool isW3C, string expectedParentId = null, string expectedSource = null) { Assert.NotNull(requestTelemetry); Assert.Equal(expectedParentId, requestTelemetry.Context.Operation.ParentId); Assert.Equal(expectedSource, requestTelemetry.Source); - if (IsW3C) + if (isW3C) { Assert.Equal(requestTelemetry.Id, FormatTelemetryId(activity.TraceId.ToHexString(), activity.SpanId.ToHexString())); Assert.Equal(requestTelemetry.Context.Operation.Id, activity.TraceId.ToHexString()); diff --git a/test/WebApi20.FunctionalTests/FunctionalTest/RequestCorrelationTests.cs b/test/WebApi20.FunctionalTests/FunctionalTest/RequestCorrelationTests.cs index 86b13b2..d490877 100644 --- a/test/WebApi20.FunctionalTests/FunctionalTest/RequestCorrelationTests.cs +++ b/test/WebApi20.FunctionalTests/FunctionalTest/RequestCorrelationTests.cs @@ -214,7 +214,7 @@ var actualRequest = this.ValidateRequestWithHeaders(server, RequestPath, headers, expectedRequestTelemetry); Assert.Equal("4bf92f3577b34da6a3ce929d0e0e4736", actualRequest.tags["ai.operation.id"]); - Assert.Contains("00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01", actualRequest.tags["ai.operation.parentId"]); + Assert.Equal("|4bf92f3577b34da6a3ce929d0e0e4736.00f067aa0ba902b7.", actualRequest.tags["ai.operation.parentId"]); // Correlation-Context will be read if either Request-Id or TraceParent available. Assert.True(actualRequest.data.baseData.properties.ContainsKey("k1")); @@ -261,7 +261,7 @@ Assert.Equal("4bf92f3577b34da6a3ce929d0e0e4736", actualRequest.tags["ai.operation.id"]); Assert.NotEqual("8ee8641cbdd8dd280d239fa2121c7e4e", actualRequest.tags["ai.operation.id"]); - Assert.Contains("00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01", actualRequest.tags["ai.operation.parentId"]); + Assert.Equal("|4bf92f3577b34da6a3ce929d0e0e4736.00f067aa0ba902b7.", actualRequest.tags["ai.operation.parentId"]); // Correlation-Context will be read if either Request-Id or traceparent is present. Assert.True(actualRequest.data.baseData.properties.ContainsKey("k1")); From 15eae02fef70cdc4123692e765f7fe47a7d53250 Mon Sep 17 00:00:00 2001 From: Liudmila Molkova Date: Fri, 6 Sep 2019 16:08:53 -0700 Subject: [PATCH 2/2] fix tests --- test/TestApp30.Tests/BasicTestWithCorrelation.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/TestApp30.Tests/BasicTestWithCorrelation.cs b/test/TestApp30.Tests/BasicTestWithCorrelation.cs index 4e9ba51..30c07f6 100644 --- a/test/TestApp30.Tests/BasicTestWithCorrelation.cs +++ b/test/TestApp30.Tests/BasicTestWithCorrelation.cs @@ -1,7 +1,5 @@ using Microsoft.ApplicationInsights.Channel; using Microsoft.ApplicationInsights.DataContracts; -using Microsoft.AspNetCore.Mvc.Testing; -using Microsoft.Extensions.DependencyInjection; using System; using System.Collections.Concurrent; using System.Collections.Generic; @@ -65,7 +63,7 @@ namespace TestApp30.Tests Assert.NotNull(trace); Assert.Equal("4e3083444c10254ba40513c7316332eb", req.Context.Operation.Id); - Assert.Equal("00-4e3083444c10254ba40513c7316332eb-e2a5f830c0ee2c46-00", req.Context.Operation.ParentId); + Assert.Equal("|4e3083444c10254ba40513c7316332eb.e2a5f830c0ee2c46.", req.Context.Operation.ParentId); Assert.Equal("4e3083444c10254ba40513c7316332eb", trace.Context.Operation.Id); Assert.Contains("|4e3083444c10254ba40513c7316332eb.", req.Id); Assert.Equal(req.Id, trace.Context.Operation.ParentId); @@ -111,7 +109,7 @@ namespace TestApp30.Tests Assert.Equal("4e3083444c10254ba40513c7316332eb", req.Context.Operation.Id); Assert.Equal("4e3083444c10254ba40513c7316332eb", exception.Context.Operation.Id); - Assert.Equal("00-4e3083444c10254ba40513c7316332eb-e2a5f830c0ee2c46-00", req.Context.Operation.ParentId); + Assert.Equal("|4e3083444c10254ba40513c7316332eb.e2a5f830c0ee2c46.", req.Context.Operation.ParentId); Assert.Equal(req.Id, exception.Context.Operation.ParentId); Assert.Contains("|4e3083444c10254ba40513c7316332eb.", req.Id);