Backport fixes to 1.x by andrewlock · Pull Request #2479 · DataDog/dd-trace-dotnet

andrewlock · 2022-02-21T10:54:30Z

Summary of changes

Fix missing http.status_code tag on ASP.NET Core spans with errors (Fix missing http.status_code tag on ASP.NET Core spans with errors #2458)
Support cleaning filenames in http-client + aspnet (Support cleaning of filenames in HttpClient + ASP.NET #2471)

Reason for change

#2458 is impacting customers that are currently using 1.x, and are unable to upgrade at this time.

Implementation details

Cherry-picking #2458 was slightly problematic, as there have been a number of other fixes and changes in that time (#2419). However, including those as well was causing even more difficulty.

…2458) * Add tests for case where an exception is set on a span and a non-200 status code is set Shows that the tests are failing for the "legacy" (feature-flag disabled) use case. * Fix missing HTTP status code tag when span is manually marked as an error

The HttpClient integration (and the ASP.NET/ASP.NET Core integration when the feature flag is disabled) all do "cleaning" of URLs to try and reduce the cardinality introduced by number/guid route parameters. This works by examining each "segment" in a URL, and using heuristics to see if we should replace it with `?`. This works as expected for the following paths: - `/some/value/123` => `/some/value/?` - `/some/value/123/123-456 => `/some/value/?/?` - `/some/value/123/example.png => `/some/value/?/example.png` However, it treats the whole final segment as a single unit, which means filenames with extensions will never be obfuscated, e.g. - `/some-filevalue/123.png` - `/some-filevalue/123.456` - `/some-filevalue/123.c` That can lead to high cardinality, which in turn can cause problems for customers with too many endpoints in their account. To work around it, we treat the filename as a separate segment from the file extension, and don't try and obfuscate the extension

kevingosse · 2022-02-21T10:56:56Z

-                    scope.Span.SetHttpStatusCode(httpContext.Response.StatusCode, isServer: true);
-                    scope.Span.SetHeaderTags(new HeadersCollectionAdapter(httpContext.Response.Headers), tracer.Settings.HeaderTags, defaultTagPrefix: SpanContextPropagator.HttpResponseHeadersTagPrefix);
+                    span.SetHttpStatusCode(httpContext.Response.StatusCode, isServer: true);
+                    span.SetHeaderTags(new HeadersCollectionAdapter(httpContext.Response.Headers), tracer.Settings.HeaderTags, defaultTagPrefix: SpanContextPropagator.HttpResponseHeadersTagPrefix);


There is a slight change in behavior here. Before we would be adding the header tags whenever there was no error. Now you only add them if there is no status code. It means that if there is a status code and no error, we would previously set the header tags and not anymore.

Hmmm, that's a good point. That behaviour is in master too 🤔 I wonder then if we should change it so that the headers are always added in the stop, regardless (which would supersede #2460 too)

Thes changes were necessary because of the significant differences between master and release/1.x

andrewlock · 2022-02-21T20:27:56Z

Code Coverage Report 📊

⛔ Merging #2479 into master will will decrease line coverage by 6%
⚠️ Merging #2479 into master will will decrease branch coverage by 1%
✔️ Merging #2479 into master will will decrease complexity by 2504

	master	#2479	Change
Lines	`11634` / `15979`	`10393` / `15549`
Lines %	`73%`	`67%`	`-6%` ⛔
Branches	`6258` / `9348`	`4549` / `6870`
Branches %	`67%`	`66%`	`-1%` ⚠️
Complexity	`10558`	`8054`	`-2504` ✔️

View the full report for further details:

Datadog.Trace Breakdown ⛔

	master	#2479	Change
Lines %	`73%`	`67%`	`-6%` ⛔
Branches %	`67%`	`66%`	`-1%` ⚠️
Complexity	`10558`	`8054`	`-2504` ✔️

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.ClrProfiler.AutoInstrumentation.AdoNet.CommandExecuteScalarIntegration	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.ClrProfiler.AutoInstrumentation.MongoDb.IWireProtocol_Execute_Integration	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.ClrProfiler.AutoInstrumentation.MongoDb.IWireProtocol_ExecuteAsync_Integration	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.ClrProfiler.AutoInstrumentation.MongoDb.IWireProtocol_Generic_Execute_Integration	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.ClrProfiler.CallTarget.CallTargetInvokerException	`-100%` ⛔	`0%` ✔️	`0` ✔️
Datadog.Trace.Agent.Transports.ApiWebRequest	`-66%` ⛔	`-88%` ⛔	`16` ⛔
Datadog.Trace.ClrProfiler.CallTarget.Handlers.IntegrationOptions`2	`-50%` ⛔	`-75%` ⛔	`-6` ✔️
Datadog.Trace.Agent.Transports.HttpStreamRequest	`-49%` ⛔	`-33%` ⛔	`5` ⛔
Datadog.Trace.Configuration.IntegrationSettingsCollection	`-33%` ⛔	`-60%` ⛔	`7` ⛔
Datadog.Trace.ClrProfiler.AutoInstrumentation.Http.WebRequest.HttpWebRequest_EndGetResponse_Integration	`-28%` ⛔	`-24%` ⛔	`-4` ✔️
...And 55 more

The following classes were added in #2479:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.Agent.MessagePack.FormatterResolverWrapper	`100%`	`100%`	`2`
Datadog.Trace.Agent.TransportStrategy	`71%`	`83%`	`7`
Datadog.Trace.AppSec.Transport.Http.HttpTransport	`100%`	`100%`	`10`
Datadog.Trace.AppSec.Transport.Http.HttpTransport	`0%`	`100%`	`10`
Datadog.Trace.AsyncLocalCompat`1	`100%`	`100%`	`8`
...And 107 more

180 classes were removed from Datadog.Trace in #2479

View the full reports for further details:

andrewlock · 2022-02-21T20:53:00Z

Benchmarks Report 🐌

Benchmarks for #2479 compared to master:

2 benchmarks are faster, with geometric mean 1.169
9 benchmarks are slower, with geometric mean 1.324
All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

Mann–Whitney U test with statistical test for significance of 5%
Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	880.5 μs	17.10 μs	21.63 μs	3.09 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	676.2 μs	11.76 μs	11.00 μs	2.51 KB
#2479	`WriteAndFlushEnrichedTraces`	net472	956.4 μs	18.64 μs	26.74 μs	3.09 KB
#2479	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	741.7 μs	14.69 μs	22.43 μs	2.51 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0.0000 ns	0.0000 ns	0.0000 ns	0.0000	0 B
master	`SendRequest`	netcoreapp3.1	301,104.4055 ns	4,432.8329 ns	3,929.5909 ns	0.1524	19714 B
#2479	`SendRequest`	net472	0.0000 ns	0.0000 ns	0.0000 ns	0.0000	0 B
#2479	`SendRequest`	netcoreapp3.1	313,079.3782 ns	4,471.1496 ns	4,182.3161 ns	0.1623	19847 B

Benchmarks.Trace.DbCommandBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2479

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery‑netcoreapp3.1	1.260	1,514.63	1,908.45

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.831 μs	0.0222 μs	0.0218 μs	0.0927	0.0009	586 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.533 μs	0.0300 μs	0.0485 μs	0.0087	0.0000	624 B
#2479	`ExecuteNonQuery`	net472	2.023 μs	0.0402 μs	0.0847 μs	0.1083	0.0010	682 B
#2479	`ExecuteNonQuery`	netcoreapp3.1	1.912 μs	0.0197 μs	0.0175 μs	0.0095	0.0000	712 B

Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2479

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net472	1.218	2,555.59	3,113.86
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync‑net472	1.130	2,912.39	3,290.06

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.552 μs	0.0340 μs	0.0266 μs	0.1250	794 B
master	`CallElasticsearch`	netcoreapp3.1	1.723 μs	0.0251 μs	0.0223 μs	0.0104	784 B
master	`CallElasticsearchAsync`	net472	2.937 μs	0.0584 μs	0.1069 μs	0.1452	931 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.891 μs	0.0359 μs	0.0318 μs	0.0125	904 B
#2479	`CallElasticsearch`	net472	3.116 μs	0.0587 μs	0.0577 μs	0.1284	827 B
#2479	`CallElasticsearch`	netcoreapp3.1	1.651 μs	0.0313 μs	0.0292 μs	0.0104	768 B
#2479	`CallElasticsearchAsync`	net472	3.300 μs	0.0506 μs	0.0474 μs	0.1501	963 B
#2479	`CallElasticsearchAsync`	netcoreapp3.1	1.772 μs	0.0351 μs	0.0345 μs	0.0125	888 B

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	3.032 μs	0.0604 μs	0.0741 μs	0.1662	1.03 KB
master	`ExecuteAsync`	netcoreapp3.1	2.028 μs	0.0159 μs	0.0124 μs	0.0146	1 KB
#2479	`ExecuteAsync`	net472	3.368 μs	0.0362 μs	0.0303 μs	0.1696	1083 B
#2479	`ExecuteAsync`	netcoreapp3.1	1.953 μs	0.0389 μs	0.0606 μs	0.0140	1008 B

Benchmarks.Trace.HttpClientBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #2479

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.HttpClientBenchmark.SendAsync‑net472	1.182	7,020.33	5,940.22
Benchmarks.Trace.HttpClientBenchmark.SendAsync‑netcoreapp3.1	1.156	4,934.83	4,270.55

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	7.053 μs	0.1405 μs	0.1618 μs	0.3638	2.25 KB
master	`SendAsync`	netcoreapp3.1	4.941 μs	0.0986 μs	0.1415 μs	0.0316	2.15 KB
#2479	`SendAsync`	net472	5.950 μs	0.1147 μs	0.1853 μs	0.3505	2.19 KB
#2479	`SendAsync`	netcoreapp3.1	4.317 μs	0.0841 μs	0.1152 μs	0.0291	2.09 KB

Benchmarks.Trace.ILoggerBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2479

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ILoggerBenchmark.EnrichedLog‑netcoreapp3.1	1.185	3,109.61	3,684.70
Benchmarks.Trace.ILoggerBenchmark.EnrichedLog‑net472	1.156	3,288.92	3,803.59

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.274 μs	0.0608 μs	0.0508 μs	0.2247	1.4 KB
master	`EnrichedLog`	netcoreapp3.1	3.115 μs	0.0600 μs	0.0561 μs	0.0201	1.48 KB
#2479	`EnrichedLog`	net472	3.814 μs	0.0736 μs	0.0956 μs	0.2872	1.79 KB
#2479	`EnrichedLog`	netcoreapp3.1	3.690 μs	0.0335 μs	0.0297 μs	0.0274	1.94 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	291.5 μs	3.40 μs	3.18 μs	0.4376	0.1459	4.21 KB
master	`EnrichedLog`	netcoreapp3.1	237.2 μs	3.42 μs	5.11 μs	0.0000	0.0000	4.09 KB
#2479	`EnrichedLog`	net472	296.2 μs	3.05 μs	2.71 μs	0.5938	0.1485	5.23 KB
#2479	`EnrichedLog`	netcoreapp3.1	233.9 μs	2.21 μs	1.85 μs	0.0000	0.0000	5.05 KB

Benchmarks.Trace.NLogBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2479

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.NLogBenchmark.EnrichedLog‑netcoreapp3.1	1.824	6,526.85	11,903.07
Benchmarks.Trace.NLogBenchmark.EnrichedLog‑net472	1.516	7,631.82	11,571.45

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	7.663 μs	0.1288 μs	0.1142 μs	0.5019	3.13 KB
master	`EnrichedLog`	netcoreapp3.1	6.522 μs	0.1064 μs	0.1182 μs	0.0505	3.5 KB
#2479	`EnrichedLog`	net472	11.57 μs	0.149 μs	0.139 μs	0.8553	5.31 KB
#2479	`EnrichedLog`	netcoreapp3.1	11.92 μs	0.213 μs	0.199 μs	0.0885	6.28 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.378 μs	0.0473 μs	0.0736 μs	0.1567	1003 B
master	`SendReceive`	netcoreapp3.1	2.022 μs	0.0285 μs	0.0267 μs	0.0139	1000 B
#2479	`SendReceive`	net472	2.106 μs	0.0224 μs	0.0187 μs	0.1543	987 B
#2479	`SendReceive`	netcoreapp3.1	2.078 μs	0.0413 μs	0.0825 μs	0.0130	984 B

Benchmarks.Trace.SerilogBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #2479

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SerilogBenchmark.EnrichedLog‑netcoreapp3.1	1.424	5,285.28	7,524.55
Benchmarks.Trace.SerilogBenchmark.EnrichedLog‑net472	1.335	5,948.89	7,941.38

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.965 μs	0.1033 μs	0.0916 μs	0.2899	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	5.280 μs	0.0990 μs	0.0926 μs	0.0183	1.44 KB
#2479	`EnrichedLog`	net472	8.005 μs	0.1590 μs	0.1487 μs	0.4464	2.8 KB
#2479	`EnrichedLog`	netcoreapp3.1	7.509 μs	0.1484 μs	0.2600 μs	0.0352	2.61 KB

Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	Error	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	905.4 ns	12.64 ns	11.21 ns	0.0706	449 B
master	`StartFinishSpan`	netcoreapp3.1	897.1 ns	12.61 ns	10.53 ns	0.0063	448 B
master	`StartFinishScope`	net472	1,090.9 ns	21.42 ns	20.03 ns	0.0832	530 B
master	`StartFinishScope`	netcoreapp3.1	1,061.1 ns	19.71 ns	17.47 ns	0.0078	568 B
#2479	`StartFinishSpan`	net472	903.7 ns	17.64 ns	17.33 ns	0.0678	433 B
#2479	`StartFinishSpan`	netcoreapp3.1	868.6 ns	13.85 ns	12.96 ns	0.0061	432 B
#2479	`StartFinishScope`	net472	1,059.2 ns	21.02 ns	27.33 ns	0.0806	514 B
#2479	`StartFinishScope`	netcoreapp3.1	1,011.7 ns	8.57 ns	6.69 ns	0.0077	552 B

andrewlock added 2 commits February 21, 2022 10:51

andrewlock added the type:bug label Feb 21, 2022

andrewlock requested a review from a team as a code owner February 21, 2022 10:54

kevingosse reviewed Feb 21, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Always set response header tags in ASP.NET Core (#2480)

ed1d1e6

andrewlock force-pushed the andrew/aspnet-hotfixes branch from 8acf930 to ed1d1e6 Compare February 21, 2022 15:05

Fix sample helpers and update snapshots

38a0eed

Thes changes were necessary because of the significant differences between master and release/1.x

This comment has been minimized.

Sign in to view

kevingosse approved these changes Feb 21, 2022

View reviewed changes

andrewlock merged commit 9b0aa7e into release/1.x Feb 21, 2022

andrewlock deleted the andrew/aspnet-hotfixes branch February 21, 2022 22:54

andrewlock added this to the vNext-v1 milestone Feb 22, 2022

Uh oh!

Conversation

andrewlock commented Feb 21, 2022

Summary of changes

Reason for change

Implementation details

Uh oh!

kevingosse Feb 21, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Feb 21, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock Feb 21, 2022

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

andrewlock commented Feb 21, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ⛔

Uh oh!

andrewlock commented Feb 21, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Slower ⚠️ in #2479

Raw results

Slower ⚠️ in #2479

Raw results

Raw results

Faster 🎉 in #2479

Raw results

Slower ⚠️ in #2479

Raw results

Raw results

Slower ⚠️ in #2479

Raw results

Raw results

Slower ⚠️ in #2479

Raw results

Raw results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants