Skip to content

[Debugger] Fixed serialization issues in producing snapshots#3064

Merged
GreenMatan merged 2 commits into
masterfrom
matang/fixing-snapshot-serialization
Aug 23, 2022
Merged

[Debugger] Fixed serialization issues in producing snapshots#3064
GreenMatan merged 2 commits into
masterfrom
matang/fixing-snapshot-serialization

Conversation

@GreenMatan

@GreenMatan GreenMatan commented Aug 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary of changes

Fixed various snapshot serialization issues that resulted in failure of producing snapshots.

Test coverage

Added a test MultidimensionalArrayTest that reproduced an issue seen in a method that resides in the Newtonsoft.Json package.

@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from 293b3ff to 674d746 Compare August 9, 2022 08:48
@GreenMatan
GreenMatan marked this pull request as ready for review August 9, 2022 08:49
@GreenMatan
GreenMatan requested review from a team as code owners August 9, 2022 08:49
@GreenMatan GreenMatan changed the title [Debugger] Fixed serialization issues [Debugger] Fixed serialization issues in producing snapshots Aug 9, 2022
@GreenMatan
GreenMatan requested a review from a team as a code owner August 9, 2022 08:56
@andrewlock

This comment has been minimized.

Sign in to view
OmerRaviv
OmerRaviv approved these changes Aug 9, 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

@andrewlock

This comment has been minimized.

Sign in to view
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from c7ecea7 to fac7b7d Compare August 10, 2022 11:59
@andrewlock

This comment has been minimized.

Sign in to view
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from fac7b7d to cab4701 Compare August 10, 2022 14:31
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from cab4701 to f424a12 Compare August 10, 2022 14:54
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from f424a12 to 2dfee63 Compare August 14, 2022 08:32
@andrewlock

This comment has been minimized.

Sign in to view
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch 3 times, most recently from 71871ea to 79126d6 Compare August 16, 2022 14:52
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from 79126d6 to a5bb5b7 Compare August 17, 2022 08:30
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from a5bb5b7 to f3899cd Compare August 17, 2022 12:36
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from f3899cd to d31256f Compare August 18, 2022 14:01
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from d31256f to 732c609 Compare August 22, 2022 12:09
gleocadie
gleocadie approved these changes Aug 22, 2022
View reviewed changes

@gleocadie gleocadie 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

@andrewlock

This comment has been minimized.

Sign in to view
@GreenMatan
GreenMatan force-pushed the matang/fixing-snapshot-serialization branch from 732c609 to 6c82da9 Compare August 23, 2022 09:08
@andrewlock

andrewlock commented Aug 23, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3064 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 Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 732μs 393ns 1.52μs 0.361 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 466μs 152ns 569ns 0 0 0 2.58 KB
#3064 WriteAndFlushEnrichedTraces net472 721μs 278ns 1.08μs 0.361 0 0 3.18 KB
#3064 WriteAndFlushEnrichedTraces netcoreapp3.1 471μs 340ns 1.27μs 0 0 0 2.58 KB
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 183ns 0.203ns 0.788ns 0.0675 9.17E-05 0 425 B
master AllCycleSimpleBody netcoreapp3.1 237ns 0.244ns 0.945ns 0.00583 0 0 424 B
master AllCycleMoreComplexBody net472 198ns 0.974ns 4.02ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 233ns 0.294ns 1.14ns 0.00551 0 0 400 B
master BodyExtractorSimpleBody net472 257ns 0.252ns 0.974ns 0.0574 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 231ns 0.401ns 1.44ns 0.0037 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.6μs 16ns 59.9ns 1.21 0.0218 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 11.8μs 14.8ns 57.4ns 0.0883 0 0 6.75 KB
#3064 AllCycleSimpleBody net472 199ns 0.225ns 0.871ns 0.0675 0 0 425 B
#3064 AllCycleSimpleBody netcoreapp3.1 236ns 0.398ns 1.54ns 0.00577 0 0 424 B
#3064 AllCycleMoreComplexBody net472 200ns 0.676ns 2.62ns 0.0637 0 0 401 B
#3064 AllCycleMoreComplexBody netcoreapp3.1 234ns 0.246ns 0.919ns 0.00538 0 0 400 B
#3064 BodyExtractorSimpleBody net472 262ns 0.158ns 0.591ns 0.0573 0 0 361 B
#3064 BodyExtractorSimpleBody netcoreapp3.1 233ns 0.266ns 0.996ns 0.00374 0 0 272 B
#3064 BodyExtractorMoreComplexBody net472 14.3μs 10.4ns 38.9ns 1.21 0.0214 0 7.62 KB
#3064 BodyExtractorMoreComplexBody netcoreapp3.1 12μs 19.8ns 76.7ns 0.09 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 179μs 111ns 431ns 0.267 0 0 20.33 KB
#3064 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3064 SendRequest netcoreapp3.1 177μs 84ns 314ns 0.265 0 0 20.33 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.59μs 1.2ns 4.64ns 0.126 0.000794 0 794 B
master ExecuteNonQuery netcoreapp3.1 1.24μs 0.678ns 2.54ns 0.0112 0 0 824 B
#3064 ExecuteNonQuery net472 1.6μs 0.586ns 2.19ns 0.126 0.000801 0 794 B
#3064 ExecuteNonQuery netcoreapp3.1 1.21μs 0.497ns 1.86ns 0.0109 0 0 824 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.23μs 0.764ns 2.96ns 0.159 0 0 1 KB
master CallElasticsearch netcoreapp3.1 1.41μs 0.971ns 3.76ns 0.0132 0 0 984 B
master CallElasticsearchAsync net472 2.3μs 0.515ns 2ns 0.181 0 0 1.14 KB
master CallElasticsearchAsync netcoreapp3.1 1.46μs 1.29ns 4.84ns 0.0148 0 0 1.1 KB
#3064 CallElasticsearch net472 2.26μs 0.505ns 1.95ns 0.158 0 0 1 KB
#3064 CallElasticsearch netcoreapp3.1 1.39μs 0.771ns 2.89ns 0.0132 0 0 984 B
#3064 CallElasticsearchAsync net472 2.26μs 0.907ns 3.14ns 0.18 0 0 1.14 KB
#3064 CallElasticsearchAsync netcoreapp3.1 1.45μs 1.32ns 4.77ns 0.0148 0 0 1.1 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.3μs 6.78ns 26.3ns 0.2 0 0 1.26 KB
master ExecuteAsync netcoreapp3.1 1.55μs 3.47ns 13ns 0.0162 0 0 1.22 KB
#3064 ExecuteAsync net472 2.47μs 5.03ns 19.5ns 0.199 0 0 1.26 KB
#3064 ExecuteAsync netcoreapp3.1 1.56μs 3.42ns 12.3ns 0.0163 0 0 1.22 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 4.84μs 11.2ns 41.8ns 0.394 0 0 2.48 KB
master SendAsync netcoreapp3.1 3.09μs 7.4ns 27.7ns 0.0325 0 0 2.36 KB
#3064 SendAsync net472 4.96μs 13.6ns 51ns 0.393 0 0 2.48 KB
#3064 SendAsync netcoreapp3.1 3.11μs 7.4ns 27.7ns 0.0322 0 0 2.36 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 2.86μs 0.665ns 2.49ns 0.263 0 0 1.66 KB
master EnrichedLog netcoreapp3.1 2.38μs 1.09ns 4.07ns 0.0238 0 0 1.73 KB
#3064 EnrichedLog net472 2.73μs 0.352ns 1.27ns 0.264 0 0 1.66 KB
#3064 EnrichedLog netcoreapp3.1 2.4μs 1.38ns 5.17ns 0.024 0 0 1.73 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 148μs 258ns 999ns 0.668 0.223 0 4.5 KB
master EnrichedLog netcoreapp3.1 113μs 145ns 563ns 0 0 0 4.38 KB
#3064 EnrichedLog net472 146μs 91.4ns 354ns 0.662 0.221 0 4.5 KB
#3064 EnrichedLog netcoreapp3.1 113μs 231ns 895ns 0.0568 0 0 4.38 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.17μs 12.7ns 49.2ns 0.546 0.00256 0 3.43 KB
master EnrichedLog netcoreapp3.1 4.27μs 7.04ns 26.3ns 0.0513 0 0 3.8 KB
#3064 EnrichedLog net472 5.24μs 19.3ns 74.8ns 0.544 0.00259 0 3.43 KB
#3064 EnrichedLog netcoreapp3.1 4.23μs 12.5ns 48.2ns 0.0519 0 0 3.8 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.04μs 0.789ns 3.06ns 0.193 0 0 1.22 KB
master SendReceive netcoreapp3.1 1.64μs 0.878ns 3.17ns 0.0164 0 0 1.21 KB
#3064 SendReceive net472 1.99μs 0.662ns 2.56ns 0.194 0 0 1.22 KB
#3064 SendReceive netcoreapp3.1 1.72μs 1.17ns 4.52ns 0.0164 0 0 1.21 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 4.69μs 1.88ns 7.3ns 0.329 0 0 2.08 KB
master EnrichedLog netcoreapp3.1 4.21μs 1.46ns 5.46ns 0.023 0 0 1.69 KB
#3064 EnrichedLog net472 4.75μs 1.1ns 3.97ns 0.329 0 0 2.08 KB
#3064 EnrichedLog netcoreapp3.1 4.16μs 1.39ns 5.18ns 0.0228 0 0 1.69 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 859ns 0.545ns 2.11ns 0.104 0 0 658 B
master StartFinishSpan netcoreapp3.1 767ns 0.252ns 0.909ns 0.00892 0 0 648 B
master StartFinishScope net472 1.08μs 0.523ns 1.96ns 0.117 0 0 738 B
master StartFinishScope netcoreapp3.1 918ns 0.242ns 0.939ns 0.0105 0 0 768 B
#3064 StartFinishSpan net472 847ns 0.176ns 0.634ns 0.105 0 0 658 B
#3064 StartFinishSpan netcoreapp3.1 736ns 0.296ns 1.07ns 0.00883 0 0 648 B
#3064 StartFinishScope net472 1.13μs 0.397ns 1.54ns 0.117 0 0 738 B
#3064 StartFinishScope netcoreapp3.1 958ns 0.42ns 1.57ns 0.0105 0 0 768 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.21μs 0.459ns 1.78ns 0.117 0 0 738 B
master RunOnMethodBegin netcoreapp3.1 1.07μs 0.22ns 0.762ns 0.0107 0 0 768 B
#3064 RunOnMethodBegin net472 1.26μs 1.03ns 3.84ns 0.117 0 0 738 B
#3064 RunOnMethodBegin netcoreapp3.1 1.02μs 0.473ns 1.77ns 0.0102 0 0 768 B

@andrewlock

andrewlock commented Aug 23, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

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

master #3064 Change
Lines 16940 / 23281 16918 / 23281
Lines % 73% 73% 0% ✔️
Branches 10106 / 14362 10108 / 14374
Branches % 70% 70% 0% ✔️
Complexity 15458 15470 12 ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3064 Change
Lines % 73% 73% 0% ✔️
Branches % 70% 70% 0% ✔️
Complexity 15458 15470 12 ⛔

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Debugger.CapturedContext -12% ⛔ -17% ⛔ 2 ⛔
Datadog.Trace.Activity.DiagnosticSourceEventListener -6% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.CapturedValue -6% ⛔ 7% ✔️ -4 ✔️
Datadog.Trace.Pdb.DatadogPdbReader 1% ✔️ 12% ✔️ 2 ⛔
Datadog.Trace.Ci.CIVisibility 5% ✔️ 6% ✔️ 0 ✔️

The following classes were added in #3064:

File Line coverage Branch coverage Complexity
Datadog.Trace.Debugger.SnapshotSerializationHelpers 88% 50% 4

View the full reports for further details:

@GreenMatan
GreenMatan merged commit afc5944 into master Aug 23, 2022
@GreenMatan
GreenMatan deleted the matang/fixing-snapshot-serialization branch August 23, 2022 13:59
@github-actions github-actions Bot added this to the vNext milestone Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anna-git anna-git anna-git left review comments
@OmerRaviv OmerRaviv OmerRaviv approved these changes
@dudikeleti dudikeleti dudikeleti approved these changes
@gleocadie gleocadie gleocadie approved these changes

Assignees

No one assigned

Labels

area:debugger

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

6 participants

@GreenMatan @andrewlock @OmerRaviv @anna-git @dudikeleti @gleocadie