Skip to content

[CI Visibility] - Add a final try to load the pdb from a module.#3350

Merged
tonyredondo merged 2 commits into
masterfrom
tony/get-module-def-fix
Oct 14, 2022
Merged

[CI Visibility] - Add a final try to load the pdb from a module.#3350
tonyredondo merged 2 commits into
masterfrom
tony/get-module-def-fix

Conversation

@tonyredondo

@tonyredondo tonyredondo commented Oct 13, 2022

Copy link
Copy Markdown
Member

Summary of changes

This PR adds a fallback when the MethodSymbolResolver cannot open the PDB file automatically, by manually trying to find the pdb as a last resort.

Reason for change

When working in the test logger, this scenario appears to happen frequently, this workaround fixes the issue.

@tonyredondo tonyredondo self-assigned this Oct 13, 2022
@tonyredondo
tonyredondo requested review from a team as code owners October 13, 2022 13:27
@github-actions github-actions Bot added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Oct 13, 2022
@andrewlock

andrewlock commented Oct 13, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3350 compared to master:

  • 1 benchmarks are slower, with geometric mean 1.130
  • 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 Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 782μs 903ns 3.38μs 0.388 0 0 3.22 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 511μs 214ns 770ns 0 0 0 2.62 KB
#3350 WriteAndFlushEnrichedTraces net472 784μs 1.48μs 5.73μs 0.393 0 0 3.22 KB
#3350 WriteAndFlushEnrichedTraces netcoreapp3.1 525μs 131ns 454ns 0 0 0 2.62 KB
Benchmarks.Trace.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #3350

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleMoreComplexBody‑net472 1.130 194.25 219.44

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 196ns 0.0987ns 0.382ns 0.0727 0 0 457 B
master AllCycleSimpleBody netcoreapp3.1 256ns 0.257ns 0.994ns 0.00635 0 0 456 B
master AllCycleMoreComplexBody net472 194ns 0.0439ns 0.158ns 0.0688 0 0 433 B
master AllCycleMoreComplexBody netcoreapp3.1 253ns 0.22ns 0.852ns 0.00594 0 0 432 B
master BodyExtractorSimpleBody net472 295ns 0.157ns 0.609ns 0.0573 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 239ns 0.554ns 2ns 0.00378 0 0 272 B
master BodyExtractorMoreComplexBody net472 16.2μs 7.76ns 29ns 1.2 0.0163 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.9μs 5.6ns 19.4ns 0.0903 0 0 6.75 KB
#3350 AllCycleSimpleBody net472 198ns 0.102ns 0.395ns 0.0727 0 0 457 B
#3350 AllCycleSimpleBody netcoreapp3.1 256ns 0.117ns 0.451ns 0.00618 0 0 456 B
#3350 AllCycleMoreComplexBody net472 220ns 0.439ns 1.7ns 0.0689 0 0 433 B
#3350 AllCycleMoreComplexBody netcoreapp3.1 257ns 0.332ns 1.29ns 0.00596 0 0 432 B
#3350 BodyExtractorSimpleBody net472 277ns 0.257ns 0.994ns 0.0573 0 0 361 B
#3350 BodyExtractorSimpleBody netcoreapp3.1 238ns 0.128ns 0.497ns 0.00372 0 0 272 B
#3350 BodyExtractorMoreComplexBody net472 15.7μs 15.2ns 58.8ns 1.21 0.0158 0 7.62 KB
#3350 BodyExtractorMoreComplexBody netcoreapp3.1 13.1μs 4.35ns 15.7ns 0.0918 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
master SendRequest netcoreapp3.1 182μs 281ns 1.09μs 0.271 0 0 20.68 KB
#3350 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3350 SendRequest netcoreapp3.1 180μs 267ns 1.03μs 0.18 0 0 20.68 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net472 1.85μs 0.549ns 2.05ns 0.155 0.000925 0 979 B
master ExecuteNonQuery netcoreapp3.1 1.36μs 0.65ns 2.52ns 0.0129 0 0 968 B
#3350 ExecuteNonQuery net472 1.84μs 0.928ns 3.6ns 0.155 0.000921 0 979 B
#3350 ExecuteNonQuery netcoreapp3.1 1.42μs 0.685ns 2.65ns 0.0129 0 0 968 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net472 2.54μs 0.926ns 3.46ns 0.189 0 0 1.19 KB
master CallElasticsearch netcoreapp3.1 1.52μs 0.838ns 3.25ns 0.0152 0 0 1.13 KB
master CallElasticsearchAsync net472 2.64μs 0.838ns 3.14ns 0.21 0 0 1.32 KB
master CallElasticsearchAsync netcoreapp3.1 1.63μs 0.749ns 2.9ns 0.017 0 0 1.25 KB
#3350 CallElasticsearch net472 2.66μs 9.43ns 36.5ns 0.188 0 0 1.19 KB
#3350 CallElasticsearch netcoreapp3.1 1.61μs 0.597ns 2.31ns 0.0154 0 0 1.13 KB
#3350 CallElasticsearchAsync net472 2.78μs 1.82ns 7.05ns 0.21 0 0 1.32 KB
#3350 CallElasticsearchAsync netcoreapp3.1 1.57μs 0.501ns 1.87ns 0.0164 0 0 1.25 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net472 2.73μs 1.49ns 5.77ns 0.23 0 0 1.45 KB
master ExecuteAsync netcoreapp3.1 1.8μs 0.466ns 1.74ns 0.019 0 0 1.38 KB
#3350 ExecuteAsync net472 2.77μs 0.673ns 2.61ns 0.231 0 0 1.45 KB
#3350 ExecuteAsync netcoreapp3.1 1.76μs 0.656ns 2.45ns 0.0194 0 0 1.38 KB
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net472 5.91μs 2.14ns 8.29ns 0.445 0 0 2.8 KB
master SendAsync netcoreapp3.1 3.78μs 2.17ns 8.42ns 0.0359 0 0 2.63 KB
#3350 SendAsync net472 5.87μs 1.59ns 5.93ns 0.443 0 0 2.8 KB
#3350 SendAsync netcoreapp3.1 3.69μs 1.28ns 4.78ns 0.0351 0 0 2.63 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 3.13μs 3.3ns 12.8ns 0.298 0 0 1.88 KB
master EnrichedLog netcoreapp3.1 2.59μs 1.41ns 5.47ns 0.0259 0 0 1.91 KB
#3350 EnrichedLog net472 3.25μs 3.21ns 12ns 0.297 0 0 1.88 KB
#3350 EnrichedLog netcoreapp3.1 2.51μs 1.51ns 5.44ns 0.0257 0 0 1.91 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 151μs 161ns 624ns 0.679 0.226 0 4.72 KB
master EnrichedLog netcoreapp3.1 120μs 160ns 620ns 0.0598 0 0 4.55 KB
#3350 EnrichedLog net472 155μs 254ns 983ns 0.689 0.23 0 4.72 KB
#3350 EnrichedLog netcoreapp3.1 121μs 187ns 724ns 0.0604 0 0 4.55 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 5.79μs 3.05ns 11ns 0.578 0.00289 0 3.65 KB
master EnrichedLog netcoreapp3.1 4.66μs 2.04ns 7.88ns 0.053 0 0 3.98 KB
#3350 EnrichedLog net472 5.64μs 2.34ns 9.04ns 0.578 0.00282 0 3.65 KB
#3350 EnrichedLog netcoreapp3.1 4.38μs 1.41ns 5.45ns 0.055 0 0 3.98 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net472 2.34μs 3.17ns 12.3ns 0.222 0 0 1.4 KB
master SendReceive netcoreapp3.1 1.88μs 0.453ns 1.63ns 0.0188 0 0 1.35 KB
#3350 SendReceive net472 2.36μs 2.17ns 8.41ns 0.223 0 0 1.4 KB
#3350 SendReceive netcoreapp3.1 1.84μs 0.922ns 3.57ns 0.0185 0 0 1.35 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 5.07μs 2.69ns 10.4ns 0.364 0 0 2.3 KB
master EnrichedLog netcoreapp3.1 4.37μs 1.2ns 4.66ns 0.024 0 0 1.86 KB
#3350 EnrichedLog net472 5.03μs 4.01ns 15.5ns 0.362 0 0 2.3 KB
#3350 EnrichedLog netcoreapp3.1 4.44μs 2.02ns 7.27ns 0.0245 0 0 1.86 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net472 1.16μs 0.296ns 1.11ns 0.133 0 0 842 B
master StartFinishSpan netcoreapp3.1 960ns 0.283ns 1.06ns 0.0106 0 0 792 B
master StartFinishScope net472 1.37μs 0.393ns 1.47ns 0.146 0 0 923 B
master StartFinishScope netcoreapp3.1 1.09μs 1.38ns 5.14ns 0.0124 0 0 912 B
#3350 StartFinishSpan net472 1.18μs 0.574ns 2.07ns 0.133 0 0 842 B
#3350 StartFinishSpan netcoreapp3.1 988ns 0.784ns 2.83ns 0.0108 0 0 792 B
#3350 StartFinishScope net472 1.47μs 0.645ns 2.41ns 0.146 0 0 923 B
#3350 StartFinishScope netcoreapp3.1 1.08μs 0.795ns 3.08ns 0.0122 0 0 912 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net472 1.55μs 0.474ns 1.84ns 0.147 0 0 923 B
master RunOnMethodBegin netcoreapp3.1 1.16μs 0.222ns 0.83ns 0.0122 0 0 912 B
#3350 RunOnMethodBegin net472 1.53μs 0.671ns 2.6ns 0.147 0 0 923 B
#3350 RunOnMethodBegin netcoreapp3.1 1.21μs 0.319ns 1.24ns 0.0121 0 0 912 B

@andrewlock

andrewlock commented Oct 13, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

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

master #3350 Change
Lines 19534 / 26924 19521 / 26929
Lines % 73% 72% 0% ✔️
Branches 11607 / 16747 11601 / 16753
Branches % 69% 69% 0% ✔️
Complexity 18113 18119 6 ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3350 Change
Lines % 73% 72% 0% ✔️
Branches % 69% 69% 0% ✔️
Complexity 18113 18119 6 ⛔

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Ci.GitInfo 5% ✔️ 8% ✔️ 0 ✔️
Datadog.Trace.Propagators.SpanContextPropagator 8% ✔️ 6% ✔️ 0 ✔️

View the full reports for further details:

lucaspimentel
lucaspimentel reviewed Oct 14, 2022
View reviewed changes
Comment thread tracer/src/Datadog.Trace/PDBs/MethodSymbolResolver.cs
if (File.Exists(pdbFile))
{
options.PdbFileOrData = pdbFile;
goto loadModule;

@lucaspimentel lucaspimentel Oct 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

goto 👀

@tonyredondo tonyredondo Oct 14, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It’s time to embrace it 🤣

lucaspimentel
lucaspimentel reviewed Oct 14, 2022
View reviewed changes
Comment thread tracer/src/Datadog.Trace/PDBs/MethodSymbolResolver.cs
else if (options.PdbFileOrData is null && !string.IsNullOrEmpty(module.Assembly.Location))
{
// If the PDB cannot be loaded automatically we do a final try to manually find it.
var pdbFile = Path.ChangeExtension(module.Assembly.Location, "pdb");

@lucaspimentel lucaspimentel Oct 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From the docs for Assembly.Location:

.NET Framework only: If the loaded file was shadow-copied, the location is that of the file after being shadow-copied. To get the location before the file has been shadow-copied, use the CodeBase property.

@lucaspimentel lucaspimentel Oct 14, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But also from the docs for Assembly.CodeBase:

Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility. Use Assembly.Location.

😅

@tonyredondo tonyredondo Oct 14, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

image

@tonyredondo
tonyredondo merged commit 595d2b1 into master Oct 14, 2022
@tonyredondo
tonyredondo deleted the tony/get-module-def-fix branch October 14, 2022 18:29
@github-actions github-actions Bot added this to the vNext milestone Oct 14, 2022
OmerRaviv
OmerRaviv reviewed Oct 14, 2022
View reviewed changes

@OmerRaviv OmerRaviv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OmerRaviv OmerRaviv OmerRaviv left review comments
@lucaspimentel lucaspimentel lucaspimentel approved these changes
@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes
@andrewlock andrewlock andrewlock approved these changes

Assignees

@tonyredondo tonyredondo

Labels

area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations)

Projects

None yet

Milestone

2.18.0

Development

Successfully merging this pull request may close these issues.

5 participants

@tonyredondo @andrewlock @OmerRaviv @lucaspimentel @zacharycmontoya