Skip to content
This repository has been archived by the owner on Jun 10, 2020. It is now read-only.

Do not set sampledOut flag if activity is recorded and parentId fixes #971

Merged
merged 2 commits into from
Sep 9, 2019
Merged

Do not set sampledOut flag if activity is recorded and parentId fixes #971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e04d60
Do not st sampledOut flag if activity is recorded and parentId fixes
Sep 6, 2019
15eae02
fix tests
Sep 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions ...cationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ public void OnHttpRequestInStart(HttpContext httpContext)
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.
Expand All @@ -201,13 +201,15 @@ public void OnHttpRequestInStart(HttpContext httpContext)
// 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(
Expand Down Expand Up @@ -282,7 +284,9 @@ public void OnHttpRequestInStart(HttpContext httpContext)
}

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);
}
Expand Down Expand Up @@ -315,7 +319,6 @@ public void OnBeginRequest(HttpContext httpContext, long timestamp)

// 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;
Expand All @@ -326,8 +329,8 @@ public void OnBeginRequest(HttpContext httpContext, long timestamp)
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);
Expand Down Expand Up @@ -367,13 +370,9 @@ public void OnBeginRequest(HttpContext httpContext, long timestamp)
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);
}
Expand Down Expand Up @@ -546,6 +545,20 @@ public void OnCompleted()
{
}

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] == '|')
Expand Down Expand Up @@ -718,10 +731,11 @@ private RequestTelemetry InitializeRequestTelemetry(HttpContext httpContext, Act
{
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))
Expand Down
81 changes: 58 additions & 23 deletions test/Microsoft.ApplicationInsights.AspNetCore.Tests/HostingDiagnosticListenerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,8 @@ public void RequestWithNonW3CCompatibleNonHierarchicalRequestIdCreateNewActivity
[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");
Expand All @@ -469,7 +470,7 @@ public void RequestWithW3CTraceParentCreateNewActivityAndPopulateRequestTelemetr
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;
Expand All @@ -479,7 +480,7 @@ public void RequestWithW3CTraceParentCreateNewActivityAndPopulateRequestTelemetr
{
Assert.Equal(ActivityCreatedByHostingDiagnosticListener, activity.OperationName);
}
else if (aspNetCoreMajorVersion == AspNetCoreMajorVersion.Two && IsW3C)
else if (aspNetCoreMajorVersion == AspNetCoreMajorVersion.Two && isW3C)
{
Assert.Equal(ActivityCreatedByHostingDiagnosticListener, activity.OperationName);
}
Expand All @@ -489,7 +490,7 @@ public void RequestWithW3CTraceParentCreateNewActivityAndPopulateRequestTelemetr
Assert.NotEqual(ActivityCreatedByHostingDiagnosticListener, activity.OperationName);
}

if (IsW3C)
if (isW3C)
{
Assert.Equal("w3cprop1=value1, w3cprop2=value2", activity.TraceStateString);
}
Expand All @@ -501,16 +502,16 @@ public void RequestWithW3CTraceParentCreateNewActivityAndPopulateRequestTelemetr
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);
}
}
}
Expand All @@ -522,7 +523,9 @@ public void RequestWithW3CTraceParentCreateNewActivityAndPopulateRequestTelemetr
[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");
Expand All @@ -531,14 +534,14 @@ public void RequestWithW3CTraceParentButInvalidEntryCreateNewActivityAndPopulate
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);
}
Expand All @@ -550,14 +553,14 @@ public void RequestWithW3CTraceParentButInvalidEntryCreateNewActivityAndPopulate
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);
}
}
}
Expand Down Expand Up @@ -598,7 +601,7 @@ public void RequestWithBothW3CAndRequestIdCreateNewActivityAndPopulateRequestTel
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
{
Expand All @@ -614,17 +617,22 @@ public void RequestWithBothW3CAndRequestIdCreateNewActivityAndPopulateRequestTel
[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");

Expand All @@ -639,9 +647,8 @@ public void OnHttpRequestInStartInitializeTelemetryIfActivityParentIdIsNotNull(b
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)
Expand Down Expand Up @@ -1014,7 +1021,35 @@ public void RequestTelemetryIsProactivelySampledOutIfFeatureFlagIsOn(AspNetCoreM
[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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps a test with -00 for Recorded flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one tests -01 request with sampled out flag set to 100% of items, so this is intentionally -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<RequestTelemetry>();
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);
Expand Down Expand Up @@ -1131,12 +1166,12 @@ private static string FormatTelemetryId(string traceId, string spanId)
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());
Expand Down
Loading