Add basic PDB reading capability (and vendor-in dnlib) by OmerRaviv · Pull Request #2592 · DataDog/dd-trace-dotnet

OmerRaviv · 2022-03-23T10:51:08Z

Summary of changes

Vendor in dnlib
Introduce a very basic DatadogPdbReader class (will be extended further in debugger slydog repo)

For convenience's sake it's recommended to review this PR per commit, as some commits introduce renames which change many files.

Reason for change

Live Debugger, CI Visibility, and Continuous Profiler all require the ability to read PDB files.

Implementation details

For insight into why we chose dnlib specifically for reading PDBs, see https://github.com/OmerRaviv/PdbBenchmarks

Note that we are currently using dnlib's managed reader for Windows PDBs. A future PR will add a dependency on the Microsoft.DiaSymReader.Native nuget package, which would then be picked up automatically by dnlib and improve the performance considerably.

Test coverage

A very basic integration test was added. This should be sufficient for now as very exhaustive tests for both the Windows PDB and Portable PDB scenarios are being added in the debugger slydog repo.

tonyredondo

LGTM, just a comment about the native nuget dependency

tonyredondo · 2022-03-23T11:13:22Z


  <ItemGroup>
    <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.1.1" PrivateAssets="All" />
+    <PackageReference Include="Microsoft.DiaSymReader.Native" Version="1.7.0" />


this is not required in this PR right?

Right! Fixed in 5814bd9.

kevingosse · 2022-03-23T13:36:31Z

+
+        public static DatadogPdbReader CreatePdbReader(string assemblyFullPath)
+        {
+            var module = ModuleDefMD.Load(File.OpenRead(assemblyFullPath));


According to the documentation of ModuleDefMD, the stream is owned by the caller, so you need to dispose it.

Great catch, thank you! Fixed in 303ff50

andrewlock · 2022-03-23T18:19:53Z

Code Coverage Report 📊

✔️ Merging #2592 into master will not change line coverage
✔️ Merging #2592 into master will not change branch coverage
⛔ Merging #2592 into master will will increase complexity by 14

	master	#2592	Change
Lines	`12404` / `17361`	`12483` / `17439`
Lines %	`71%`	`72%`	`0%` ✔️
Branches	`7015` / `10189`	`7031` / `10195`
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`11413`	`11427`	`14` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#2592	Change
Lines %	`71%`	`72%`	`0%` ✔️
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`11413`	`11427`	`14` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.ClrProfiler.InstrumentationDefinitions	`-17%` ⛔	`0%` ✔️	`2` ⛔
Datadog.Trace.DiagnosticListeners.AspNetCoreDiagnosticObserver	`3%` ✔️	`7%` ✔️	`0` ✔️
Datadog.Trace.ClrProfiler.NativeMethods	`3%` ✔️	`8%` ✔️	`0` ✔️
Datadog.Trace.Agent.TracesTransportStrategy	`15%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.Telemetry.JsonHttpClientTelemetryTransport	`26%` ✔️	`38%` ✔️	`0` ✔️
Datadog.Trace.Agent.StreamFactories.TcpStreamFactory	`90%` ✔️	`0%` ✔️	`0` ✔️

The following classes were added in #2592:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.ClrProfiler.DefinitionsIdAttribute	`0%`	`100%`	`1`
Datadog.Trace.PDBs.DatadogPdbReader	`95%`	`50%`	`5`

View the full reports for further details:

andrewlock · 2022-03-23T18:30:02Z

Benchmarks Report 🐌

Benchmarks for #2592 compared to master:

All benchmarks have the same speed
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	StdError	StdDev	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	900μs	4.46μs	20μs	3.16 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	697μs	2.17μs	8.11μs	2.57 KB
#2592	`WriteAndFlushEnrichedTraces`	net472	905μs	2.73μs	9.84μs	3.16 KB
#2592	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	690μs	3.18μs	12.3μs	2.57 KB

Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	285μs	1.33μs	5.33μs	0.283	19.74 KB
#2592	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#2592	`SendRequest`	netcoreapp3.1	294μs	1.31μs	5.09μs	0.148	19.74 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.75μs	9.05ns	41.5ns	0.0943	0.000865	594 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.45μs	5.67ns	21.9ns	0.00866	0	632 B
#2592	`ExecuteNonQuery`	net472	1.82μs	9.6ns	49.9ns	0.0941	0	594 B
#2592	`ExecuteNonQuery`	netcoreapp3.1	1.45μs	5.28ns	19ns	0.00881	0	632 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.6μs	5.98ns	26.1ns	0.127	803 B
master	`CallElasticsearch`	netcoreapp3.1	1.66μs	7.81ns	31.2ns	0.0107	792 B
master	`CallElasticsearchAsync`	net472	2.84μs	7.49ns	26ns	0.149	939 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.99μs	10.5ns	50.4ns	0.0129	912 B
#2592	`CallElasticsearch`	net472	2.71μs	14.4ns	70.4ns	0.127	802 B
#2592	`CallElasticsearch`	netcoreapp3.1	1.71μs	7.1ns	27.5ns	0.0108	792 B
#2592	`CallElasticsearchAsync`	net472	2.91μs	14.1ns	56.3ns	0.149	939 B
#2592	`CallElasticsearchAsync`	netcoreapp3.1	1.78μs	7.28ns	27.2ns	0.0128	912 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteAsync`	net472	3.12μs	16.7ns	98.7ns	0.168	0.00153	1.06 KB
master	`ExecuteAsync`	netcoreapp3.1	1.96μs	10.1ns	46.4ns	0.0148	0	1.03 KB
#2592	`ExecuteAsync`	net472	3.2μs	18ns	124ns	0.166	0	1.06 KB
#2592	`ExecuteAsync`	netcoreapp3.1	2.03μs	10.9ns	76.1ns	0.0148	0	1.03 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	6.85μs	25.7ns	99.5ns	0.36	2.28 KB
master	`SendAsync`	netcoreapp3.1	4.77μs	21.6ns	83.6ns	0.031	2.21 KB
#2592	`SendAsync`	net472	6.95μs	34.1ns	136ns	0.361	2.28 KB
#2592	`SendAsync`	netcoreapp3.1	4.72μs	23.2ns	101ns	0.0298	2.21 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.28μs	15ns	56.1ns	0.228	1.45 KB
master	`EnrichedLog`	netcoreapp3.1	3.12μs	10.7ns	38.4ns	0.0203	1.53 KB
#2592	`EnrichedLog`	net472	3.29μs	14.1ns	54.7ns	0.227	1.45 KB
#2592	`EnrichedLog`	netcoreapp3.1	3.2μs	9.78ns	37.9ns	0.0207	1.53 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	294μs	1.65μs	10.5μs	0.428	0.143	4.33 KB
master	`EnrichedLog`	netcoreapp3.1	228μs	643ns	2.49μs	0	0	4.21 KB
#2592	`EnrichedLog`	net472	286μs	1.25μs	4.84μs	0.429	0.143	4.33 KB
#2592	`EnrichedLog`	netcoreapp3.1	229μs	927ns	3.59μs	0	0	4.21 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	7.35μs	15.5ns	53.7ns	0.505	3.23 KB
master	`EnrichedLog`	netcoreapp3.1	6.24μs	25ns	93.4ns	0.0499	3.6 KB
#2592	`EnrichedLog`	net472	8μs	44.8ns	350ns	0.505	3.23 KB
#2592	`EnrichedLog`	netcoreapp3.1	6.34μs	20.4ns	73.4ns	0.0504	3.6 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`SendReceive`	net472	2.62μs	13.8ns	74.3ns	0.16	0.00131	1.01 KB
master	`SendReceive`	netcoreapp3.1	2.1μs	10.5ns	44.5ns	0.0143	0	1.01 KB
#2592	`SendReceive`	net472	2.33μs	6.86ns	24.7ns	0.16	0.00115	1.01 KB
#2592	`SendReceive`	netcoreapp3.1	2.22μs	12.2ns	72.3ns	0.0141	0	1.01 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	6.07μs	33.5ns	195ns	0.292	1.87 KB
master	`EnrichedLog`	netcoreapp3.1	4.98μs	13.2ns	51.2ns	0.0198	1.49 KB
#2592	`EnrichedLog`	net472	6.06μs	33ns	190ns	0.292	1.87 KB
#2592	`EnrichedLog`	netcoreapp3.1	4.98μs	12.9ns	49.8ns	0.021	1.49 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	876ns	3.33ns	12ns	0.0719	457 B
master	`StartFinishSpan`	netcoreapp3.1	871ns	2.8ns	10.9ns	0.00604	456 B
master	`StartFinishScope`	net472	1.09μs	3.58ns	13.8ns	0.0841	538 B
master	`StartFinishScope`	netcoreapp3.1	1.03μs	5.27ns	23ns	0.00802	576 B
#2592	`StartFinishSpan`	net472	896ns	3.79ns	14.7ns	0.0719	457 B
#2592	`StartFinishSpan`	netcoreapp3.1	850ns	2.56ns	9.92ns	0.00606	456 B
#2592	`StartFinishScope`	net472	1.11μs	5.19ns	20.7ns	0.0844	538 B
#2592	`StartFinishScope`	netcoreapp3.1	1.02μs	4.58ns	17.8ns	0.00803	576 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.29μs	7.31ns	50.6ns	0.0843	538 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.15μs	2.86ns	11.4ns	0.00769	576 B
#2592	`RunOnMethodBegin`	net472	1.29μs	5.79ns	21.7ns	0.0843	538 B
#2592	`RunOnMethodBegin`	netcoreapp3.1	1.14μs	4.11ns	15.9ns	0.00786	576 B

OmerRaviv added 4 commits March 23, 2022 12:40

Vendored in dnlib.

92ee1b0

Fix ambiguity by renaming OSPlatform to OSPlatformName

33bb6bf

Fix ambiguity by renaming AppDomain.Instance to AppDomainInstance

4a90ac9

Update PublicApiTests.AssemblyReferencesHaveNotChanged verified snapshot

ed9c89f

OmerRaviv requested a review from a team as a code owner March 23, 2022 10:51

OmerRaviv mentioned this pull request Mar 23, 2022

Add basic PDB reading capability (and vendor-in dnlib) #2550

Closed

OmerRaviv force-pushed the omerr/pdb-reading-with-dnlib-take2 branch from 89fd9fe to 9391b09 Compare March 23, 2022 11:03

Add basic DatadogPdbReader and a very basic test.

0bf3b1e

OmerRaviv force-pushed the omerr/pdb-reading-with-dnlib-take2 branch from 9391b09 to 0bf3b1e Compare March 23, 2022 11:04

tonyredondo approved these changes Mar 23, 2022

View reviewed changes

tonyredondo requested a review from a team March 23, 2022 11:15

Remove dependency on Microsoft.DiaSymReader.Native

5814bd9

This comment has been minimized.

Sign in to view

kevingosse suggested changes Mar 23, 2022

View reviewed changes

Properly Dipose() the ModuleDefMD when we dispose DatadogPdbReader

303ff50

This comment has been minimized.

Sign in to view

Remove stream that was not getting properly Disposed

7eec8b7

kevingosse approved these changes Mar 23, 2022

View reviewed changes

OmerRaviv merged commit 5050d2a into master Mar 23, 2022

OmerRaviv deleted the omerr/pdb-reading-with-dnlib-take2 branch March 23, 2022 18:46

github-actions Bot added this to the vNext milestone Mar 23, 2022

OmerRaviv mentioned this pull request Apr 12, 2022

Disable warnings in vendored code #2672

Merged

Uh oh!

Conversation

OmerRaviv commented Mar 23, 2022

Summary of changes

Reason for change

Implementation details

Test coverage

Uh oh!

tonyredondo left a comment

Choose a reason for hiding this comment

Uh oh!

tonyredondo Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

OmerRaviv Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

kevingosse Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

OmerRaviv Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

andrewlock commented Mar 23, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

andrewlock commented Mar 23, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants