Skip to content

Commit 8b1a9ff

Browse files
authored
[Tracer] Fix resource-based sampling for ASP.NET spans in IIS (#6936)
## Summary of changes Set's a preliminary `Span.ResourceName` for `ASP.NET` spans in `OnBeginRequest` as well as in `OnEndRequest` to ensure that resource-based sampling will work. ## Reason for change Within `TracingHttpModule` we do not set a `ResourceName` for the span in `OnBeginRequest`, this is to try and ensure that the resource names from the parent `ASP.NET` span and it's child span will be equivalent. We instead set it in the `OnEndRequest`. This poses an issue though in IIS as we then (commonly) call `Inject` which ends up making a sampling decision _without_ a `Span.ResourceName`. What this ends up causing is that resource-based sampling decisions / rules will never be applied. Once a sampling decision has been made it is not changed at a later point. The following code path gets hit for `ASP.NET` in IIS which causes the behavior. ```csharp // Decorate the incoming HTTP Request with distributed tracing headers // in case the next processor cannot access the stored Scope // (e.g. WCF being hosted in IIS) if (HttpRuntime.UsingIntegratedPipeline) { var injectedContext = new PropagationContext(scope.Span.Context, Baggage.Current); tracer.TracerManager.SpanContextPropagator.Inject(injectedContext, requestHeaders.Wrap()); } ``` ## Implementation details I've extracted _a part_ of the logic to build the resource name in the `OnEndRequest` and am using it in the `OnBeginRequest` phase. This will allow us to have some semblance of a relatively accurate resource name at the time that we are making the sampling decision. The part that is different is that we do not in the `OnBeginRequest` have access to the child span resource name, which is the path -> ```csharp if (app.Context.Items[SharedItems.HttpContextPropagatedResourceNameKey] is string resourceName && !string.IsNullOrEmpty(resourceName)) { currentSpan.ResourceName = resourceName; } ``` So while I _think_ that the resource name computed now will be relatively accurate there may be instances where the child HTTP span from it has a different resource name that overrides it. ## Test coverage Updated MVC4 ASP net tests to have a resource that should be sampled out (see the snapshot sampling value) based on its `aspnet` span (not the `MVC`) span resource name. ## Other details It may be beneficial to tag the original resource name as it may look a bit different to help determine what resource was used in the sampling decision (if any), but we can come back to that later. <!-- Fixes #{issue} --> <!-- ⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
1 parent d11dddc commit 8b1a9ff

8 files changed

+303
-13
lines changed

tracer/src/Datadog.Trace/AspNet/TracingHttpModule.cs

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,22 @@ public void Dispose()
8585
{
8686
}
8787

88+
private static string BuildResourceName(Tracer tracer, HttpRequest httpRequest)
89+
{
90+
var url = tracer.Settings.BypassHttpRequestUrlCachingEnabled
91+
? RequestDataHelper.BuildUrl(httpRequest)
92+
: RequestDataHelper.GetUrl(httpRequest);
93+
if (url is not null)
94+
{
95+
var path = UriHelpers.GetCleanUriPath(url, httpRequest.ApplicationPath);
96+
return $"{httpRequest.HttpMethod.ToUpperInvariant()} {path.ToLowerInvariant()}";
97+
}
98+
else
99+
{
100+
return $"{httpRequest.HttpMethod.ToUpperInvariant()}";
101+
}
102+
}
103+
88104
internal static void AddHeaderTagsFromHttpResponse(HttpContext httpContext, Scope scope)
89105
{
90106
if (!Tracer.Instance.Settings.HeaderTags.IsNullOrEmpty() &&
@@ -174,7 +190,12 @@ private void OnBeginRequest(object sender, EventArgs eventArgs)
174190
var tags = new WebTags();
175191
scope = tracer.StartActiveInternal(_requestOperationName, extractedContext.SpanContext, tags: tags);
176192
// Leave resourceName blank for now - we'll update it in OnEndRequest
177-
scope.Span.DecorateWebServerSpan(resourceName: null, httpMethod, host, url, userAgent, tags);
193+
194+
// Attempt to set Resource Name to something that will be close to what is expected
195+
// Note: we will go and re-do it in OnEndRequest, but doing it here will allow for resource-based sampling
196+
// this likely won't be perfect - but we need something to try and allow resource-based sampling to function
197+
var resourceName = BuildResourceName(tracer, httpRequest);
198+
scope.Span.DecorateWebServerSpan(resourceName: resourceName, httpMethod, host, url, userAgent, tags);
178199
tracer.TracerManager.SpanContextPropagator.AddHeadersToSpanAsTags(scope.Span, headers, tracer.Settings.HeaderTags, defaultTagPrefix: SpanContextPropagator.HttpRequestHeadersTagPrefix);
179200

180201
if (tracer.Settings.IpHeaderEnabled || Security.Instance.AppsecEnabled)
@@ -386,18 +407,7 @@ private void OnEndRequest(object sender, EventArgs eventArgs)
386407
}
387408
else
388409
{
389-
var url = tracer.Settings.BypassHttpRequestUrlCachingEnabled
390-
? RequestDataHelper.BuildUrl(app.Request)
391-
: RequestDataHelper.GetUrl(app.Request);
392-
if (url is not null)
393-
{
394-
string path = UriHelpers.GetCleanUriPath(url, app.Request.ApplicationPath);
395-
currentSpan.ResourceName = $"{app.Request.HttpMethod.ToUpperInvariant()} {path.ToLowerInvariant()}";
396-
}
397-
else
398-
{
399-
currentSpan.ResourceName = $"{app.Request.HttpMethod.ToUpperInvariant()}";
400-
}
410+
currentSpan.ResourceName = BuildResourceName(tracer, app.Request);
401411
}
402412
}
403413
finally

tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/AspNet/AspNetMvc4Tests.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
using System.Text;
1414
using System.Threading.Tasks;
1515
using Datadog.Trace.Configuration;
16+
using Datadog.Trace.Sampling;
1617
using Datadog.Trace.TestHelpers;
1718
using FluentAssertions;
1819
using VerifyXunit;
@@ -80,6 +81,11 @@ public AspNetMvc4Tests(IisFixture iisFixture, ITestOutputHelper output, bool cla
8081
SetEnvironmentVariable(ConfigurationKeys.FeatureFlags.RouteTemplateResourceNamesEnabled, enableRouteTemplateResourceNames.ToString());
8182
SetEnvironmentVariable(ConfigurationKeys.ExpandRouteTemplatesEnabled, enableRouteTemplateExpansion.ToString());
8283

84+
// these two are to test resource-based sampling on the parent ASP.NET span (non-MVC) one.
85+
// it should not show up at all
86+
SetEnvironmentVariable(ConfigurationKeys.CustomSamplingRules, """[{"sample_rate":0.0, "service":"*", "resource":"GET /home/pleasesamplemeout/?"}]""");
87+
SetEnvironmentVariable(ConfigurationKeys.CustomSamplingRulesFormat, SamplingRulesFormat.Glob.ToString()); // for ease of use
88+
8389
_classicMode = classicMode;
8490
_iisFixture = iisFixture;
8591
_iisFixture.ShutdownPath = "/home/shutdown";
@@ -110,6 +116,7 @@ public AspNetMvc4Tests(IisFixture iisFixture, ITestOutputHelper output, bool cla
110116
{ "/Home/BadRequestWithStatusCode?statuscode=401&TransferRequest=true", 401 },
111117
{ "/Home/BadRequestWithStatusCode?statuscode=503&TransferRequest=true", 503 },
112118
{ "/graphql/GetAllFoo", 200 }, // Slug in route template
119+
{ "/Home/PleaseSampleMeOut/5555", 200 }, // test resource-based sampling for parent ASP.NET span (non-MVC) "GET /home/pleaseSampleMeOut/?" <- note the ? it isn't {id}
113120
};
114121

115122
public override Result ValidateIntegrationSpan(MockSpan span, string metadataSchemaVersion) =>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
[
2+
{
3+
TraceId: Id_1,
4+
SpanId: Id_2,
5+
Name: aspnet-mvc.request,
6+
Resource: GET /home/pleasesamplemeout/?,
7+
Service: sample,
8+
Type: web,
9+
ParentId: Id_3,
10+
Tags: {
11+
aspnet.action: pleasesamplemeout,
12+
aspnet.controller: home,
13+
aspnet.route: {controller}/{action}/{id},
14+
env: integration_tests,
15+
http.method: GET,
16+
http.request.headers.host: localhost:00000,
17+
http.status_code: 200,
18+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
19+
http.useragent: testhelper,
20+
language: dotnet,
21+
span.kind: server,
22+
version: 1.0.0
23+
}
24+
},
25+
{
26+
TraceId: Id_1,
27+
SpanId: Id_3,
28+
Name: aspnet.request,
29+
Resource: GET /home/pleasesamplemeout/?,
30+
Service: sample,
31+
Type: web,
32+
Tags: {
33+
env: integration_tests,
34+
http.method: GET,
35+
http.request.headers.host: localhost:00000,
36+
http.route: {controller}/{action}/{id},
37+
http.status_code: 200,
38+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
39+
http.useragent: testhelper,
40+
language: dotnet,
41+
runtime-id: Guid_1,
42+
span.kind: server,
43+
version: 1.0.0
44+
},
45+
Metrics: {
46+
process_id: 0,
47+
_dd.rule_psr: 0.0,
48+
_dd.top_level: 1.0,
49+
_dd.tracer_kr: 1.0,
50+
_sampling_priority_v1: -1.0
51+
}
52+
}
53+
]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
[
2+
{
3+
TraceId: Id_1,
4+
SpanId: Id_2,
5+
Name: aspnet-mvc.request,
6+
Resource: GET /home/pleasesamplemeout/{id},
7+
Service: sample,
8+
Type: web,
9+
ParentId: Id_3,
10+
Tags: {
11+
aspnet.action: pleasesamplemeout,
12+
aspnet.controller: home,
13+
aspnet.route: {controller}/{action}/{id},
14+
env: integration_tests,
15+
http.method: GET,
16+
http.request.headers.host: localhost:00000,
17+
http.status_code: 200,
18+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
19+
http.useragent: testhelper,
20+
language: dotnet,
21+
span.kind: server,
22+
version: 1.0.0
23+
}
24+
},
25+
{
26+
TraceId: Id_1,
27+
SpanId: Id_3,
28+
Name: aspnet.request,
29+
Resource: GET /home/pleasesamplemeout/{id},
30+
Service: sample,
31+
Type: web,
32+
Tags: {
33+
env: integration_tests,
34+
http.method: GET,
35+
http.request.headers.host: localhost:00000,
36+
http.route: {controller}/{action}/{id},
37+
http.status_code: 200,
38+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
39+
http.useragent: testhelper,
40+
language: dotnet,
41+
runtime-id: Guid_1,
42+
span.kind: server,
43+
version: 1.0.0
44+
},
45+
Metrics: {
46+
process_id: 0,
47+
_dd.top_level: 1.0,
48+
_dd.tracer_kr: 1.0,
49+
_sampling_priority_v1: 1.0
50+
}
51+
}
52+
]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
[
2+
{
3+
TraceId: Id_1,
4+
SpanId: Id_2,
5+
Name: aspnet-mvc.request,
6+
Resource: GET /home/pleasesamplemeout/?,
7+
Service: sample,
8+
Type: web,
9+
ParentId: Id_3,
10+
Tags: {
11+
aspnet.action: pleasesamplemeout,
12+
aspnet.controller: home,
13+
aspnet.route: {controller}/{action}/{id},
14+
env: integration_tests,
15+
http.method: GET,
16+
http.request.headers.host: localhost:00000,
17+
http.status_code: 200,
18+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
19+
http.useragent: testhelper,
20+
language: dotnet,
21+
span.kind: server,
22+
version: 1.0.0
23+
}
24+
},
25+
{
26+
TraceId: Id_1,
27+
SpanId: Id_3,
28+
Name: aspnet.request,
29+
Resource: GET /home/pleasesamplemeout/?,
30+
Service: sample,
31+
Type: web,
32+
Tags: {
33+
env: integration_tests,
34+
http.method: GET,
35+
http.request.headers.host: localhost:00000,
36+
http.route: {controller}/{action}/{id},
37+
http.status_code: 200,
38+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
39+
http.useragent: testhelper,
40+
language: dotnet,
41+
runtime-id: Guid_1,
42+
span.kind: server,
43+
version: 1.0.0
44+
},
45+
Metrics: {
46+
process_id: 0,
47+
_dd.rule_psr: 0.0,
48+
_dd.top_level: 1.0,
49+
_dd.tracer_kr: 1.0,
50+
_sampling_priority_v1: -1.0
51+
}
52+
}
53+
]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
[
2+
{
3+
TraceId: Id_1,
4+
SpanId: Id_2,
5+
Name: aspnet-mvc.request,
6+
Resource: GET /home/pleasesamplemeout/{id},
7+
Service: sample,
8+
Type: web,
9+
ParentId: Id_3,
10+
Tags: {
11+
aspnet.action: pleasesamplemeout,
12+
aspnet.controller: home,
13+
aspnet.route: {controller}/{action}/{id},
14+
env: integration_tests,
15+
http.method: GET,
16+
http.request.headers.host: localhost:00000,
17+
http.status_code: 200,
18+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
19+
http.useragent: testhelper,
20+
language: dotnet,
21+
span.kind: server,
22+
version: 1.0.0
23+
}
24+
},
25+
{
26+
TraceId: Id_1,
27+
SpanId: Id_3,
28+
Name: aspnet.request,
29+
Resource: GET /home/pleasesamplemeout/{id},
30+
Service: sample,
31+
Type: web,
32+
Tags: {
33+
env: integration_tests,
34+
http.method: GET,
35+
http.request.headers.host: localhost:00000,
36+
http.route: {controller}/{action}/{id},
37+
http.status_code: 200,
38+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
39+
http.useragent: testhelper,
40+
language: dotnet,
41+
runtime-id: Guid_1,
42+
span.kind: server,
43+
version: 1.0.0
44+
},
45+
Metrics: {
46+
process_id: 0,
47+
_dd.rule_psr: 0.0,
48+
_dd.top_level: 1.0,
49+
_dd.tracer_kr: 1.0,
50+
_sampling_priority_v1: -1.0
51+
}
52+
}
53+
]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
[
2+
{
3+
TraceId: Id_1,
4+
SpanId: Id_2,
5+
Name: aspnet-mvc.request,
6+
Resource: GET /home/pleasesamplemeout/{id},
7+
Service: sample,
8+
Type: web,
9+
ParentId: Id_3,
10+
Tags: {
11+
aspnet.action: pleasesamplemeout,
12+
aspnet.controller: home,
13+
aspnet.route: {controller}/{action}/{id},
14+
env: integration_tests,
15+
http.method: GET,
16+
http.request.headers.host: localhost:00000,
17+
http.status_code: 200,
18+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
19+
http.useragent: testhelper,
20+
language: dotnet,
21+
span.kind: server,
22+
version: 1.0.0
23+
}
24+
},
25+
{
26+
TraceId: Id_1,
27+
SpanId: Id_3,
28+
Name: aspnet.request,
29+
Resource: GET /home/pleasesamplemeout/{id},
30+
Service: sample,
31+
Type: web,
32+
Tags: {
33+
env: integration_tests,
34+
http.method: GET,
35+
http.request.headers.host: localhost:00000,
36+
http.route: {controller}/{action}/{id},
37+
http.status_code: 200,
38+
http.url: http://localhost:00000/Home/PleaseSampleMeOut/5555,
39+
http.useragent: testhelper,
40+
language: dotnet,
41+
runtime-id: Guid_1,
42+
span.kind: server,
43+
version: 1.0.0
44+
},
45+
Metrics: {
46+
process_id: 0,
47+
_dd.rule_psr: 0.0,
48+
_dd.top_level: 1.0,
49+
_dd.tracer_kr: 1.0,
50+
_sampling_priority_v1: -1.0
51+
}
52+
}
53+
]

tracer/test/test-applications/aspnet/Samples.AspNetMvc4/Controllers/HomeController.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,5 +66,14 @@ public ActionResult OptionalIdentifier(int? id)
6666
ViewBag.Message = "Identifier set to " + id;
6767
return View("About");
6868
}
69+
70+
public ActionResult PleaseSampleMeOut(int? id)
71+
{
72+
// this should be sampled out - the resource name sampled is /home/pleaseSampleMeOut/?
73+
// however the one that will be shown is /home/pleaseSampleMeOut/{id} after MVC routes are replaced
74+
// but we make the sampling decision BEFORE the MVC routes are known
75+
ViewBag.Message = "This should be sampled out based on the ASP.NET Request resource name " + id;
76+
return View("About");
77+
}
6978
}
7079
}

0 commit comments

Comments
 (0)