[Dynamic Instrumentation] Improved instrumentation matching of symbols received through SymDb #5829
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GreenMatan
changed the title
GreenMatan
changed the title
GreenMatan
force-pushed
the
matang/symdb-fixes
branch
from
bb6bce7
to
31a1fff
Compare
GreenMatan
changed the title
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing the following branches/commits: Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5829) - mean (72ms) : 64, 81
. : milestone, 72,
master - mean (74ms) : 64, 83
. : milestone, 74,
section CallTarget+Inlining+NGEN
This PR (5829) - mean (1,060ms) : 1041, 1080
. : milestone, 1060,
master - mean (1,069ms) : 1047, 1091
. : milestone, 1069,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5829) - mean (108ms) : 104, 112
. : milestone, 108,
master - mean (109ms) : 104, 113
. : milestone, 109,
section CallTarget+Inlining+NGEN
This PR (5829) - mean (749ms) : 725, 773
. : milestone, 749,
master - mean (747ms) : 724, 771
. : milestone, 747,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5829) - mean (92ms) : 88, 97
. : milestone, 92,
master - mean (93ms) : 87, 98
. : milestone, 93,
section CallTarget+Inlining+NGEN
This PR (5829) - mean (702ms) : 685, 720
. : milestone, 702,
master - mean (704ms) : 686, 722
. : milestone, 704,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5829) - mean (192ms) : 188, 197
. : milestone, 192,
master - mean (192ms) : 189, 196
. : milestone, 192,
section CallTarget+Inlining+NGEN
This PR (5829) - mean (1,168ms) : 1140, 1197
. : milestone, 1168,
master - mean (1,165ms) : 1130, 1200
. : milestone, 1165,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5829) - mean (278ms) : 274, 283
. : milestone, 278,
master - mean (275ms) : 271, 280
. : milestone, 275,
section CallTarget+Inlining+NGEN
This PR (5829) - mean (930ms) : 892, 968
. : milestone, 930,
master - mean (917ms) : 894, 940
. : milestone, 917,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5829) - mean (267ms) : 262, 272
. : milestone, 267,
master - mean (266ms) : 261, 270
. : milestone, 266,
section CallTarget+Inlining+NGEN
This PR (5829) - mean (903ms) : 882, 925
. : milestone, 903,
master - mean (916ms) : 890, 942
. : milestone, 916,
|
Datadog ReportBranch report: ✅ 0 Failed, 348415 Passed, 2222 Skipped, 23h 3m 27.71s Total Time |
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5829) (11.544M) : 0, 11544045
master (11.678M) : 0, 11677880
benchmarks/2.9.0 (11.335M) : 0, 11335046
section Automatic
This PR (5829) (7.665M) : 0, 7664621
master (7.838M) : 0, 7838430
benchmarks/2.9.0 (8.124M) : 0, 8123605
section Trace stats
master (8.299M) : 0, 8298729
section Manual
master (11.707M) : 0, 11706878
section Manual + Automatic
This PR (5829) (7.267M) : 0, 7267474
master (7.279M) : 0, 7279436
section DD_TRACE_ENABLED=0
master (10.797M) : 0, 10797180
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5829) (9.496M) : 0, 9496159
master (9.804M) : 0, 9804149
section Automatic
This PR (5829) (6.516M) : 0, 6515891
master (6.472M) : 0, 6471648
section Trace stats
master (6.970M) : 0, 6969517
section Manual
master (9.638M) : 0, 9637761
section Manual + Automatic
This PR (5829) (6.168M) : 0, 6168482
master (6.084M) : 0, 6083512
section DD_TRACE_ENABLED=0
master (9.140M) : 0, 9139891
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5829) (9.966M) : 0, 9965927
benchmarks/2.9.0 (10.092M) : 0, 10091577
section Automatic
This PR (5829) (6.796M) : 0, 6796292
benchmarks/2.9.0 (7.470M) : 0, 7469858
section Manual + Automatic
This PR (5829) (6.362M) : 0, 6361909
|
GreenMatan
force-pushed
the
matang/symdb-fixes
branch
2 times, most recently
from
788cc9a
to
c23faf1
Compare
Benchmarks Report for tracer 🐌Benchmarks for #5829 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 | 1.140 | 396.94 | 452.46 | |
| Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 | 1.127 | 478.25 | 539.06 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 396ns | 0.64ns | 2.48ns | 0.00809 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 558ns | 0.246ns | 0.92ns | 0.00781 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 676ns | 1.02ns | 3.94ns | 0.0918 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 478ns | 0.106ns | 0.398ns | 0.00983 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 754ns | 0.343ns | 1.28ns | 0.00945 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 835ns | 0.552ns | 2.07ns | 0.104 | 0 | 0 | 658 B |
| #5829 | StartFinishSpan |
net6.0 | 453ns | 1.3ns | 4.85ns | 0.0081 | 0 | 0 | 576 B |
| #5829 | StartFinishSpan |
netcoreapp3.1 | 598ns | 0.214ns | 0.802ns | 0.00751 | 0 | 0 | 576 B |
| #5829 | StartFinishSpan |
net472 | 634ns | 0.722ns | 2.8ns | 0.0915 | 0 | 0 | 578 B |
| #5829 | StartFinishScope |
net6.0 | 539ns | 0.146ns | 0.564ns | 0.00969 | 0 | 0 | 696 B |
| #5829 | StartFinishScope |
netcoreapp3.1 | 782ns | 0.256ns | 0.99ns | 0.00962 | 0 | 0 | 696 B |
| #5829 | StartFinishScope |
net472 | 844ns | 0.475ns | 1.84ns | 0.105 | 0 | 0 | 658 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 |
net6.0 | 609ns | 0.258ns | 0.998ns | 0.00975 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 896ns | 0.455ns | 1.76ns | 0.00925 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.14μs | 0.432ns | 1.67ns | 0.105 | 0 | 0 | 658 B |
| #5829 | RunOnMethodBegin |
net6.0 | 654ns | 0.285ns | 1.1ns | 0.00956 | 0 | 0 | 696 B |
| #5829 | RunOnMethodBegin |
netcoreapp3.1 | 917ns | 1.47ns | 5.69ns | 0.00934 | 0 | 0 | 696 B |
| #5829 | RunOnMethodBegin |
net472 | 1.09μs | 0.393ns | 1.52ns | 0.105 | 0 | 0 | 658 B |
GreenMatan
force-pushed
the
matang/symdb-fixes
branch
2 times, most recently
from
f070651
to
e7cb939
Compare
GreenMatan
changed the title
GreenMatan
force-pushed
the
matang/symdb-fixes
branch
from
e7cb939
to
83fa0ae
Compare
dudikeleti
approved these changes
Jul 31, 2024
| } | ||
| else | ||
| { | ||
| if (c == '<') |
There was a problem hiding this comment.
Sometimes the type himself can contains < as part of his name, is that logic will break in this case?
There was a problem hiding this comment.
Yeah, does this need to handle unpronounceable types generated by the compiler? e.g. <PrivateImplementationDetails> and <>z__ReadOnlyArray<T>? I'd love to see unit tests for these if nothing else, to confirm that we bail-out at worst case, rather than generating something weird.
Similarly, do we need to handle open-generics?
There was a problem hiding this comment.
Open generic is handled as closed generic. E.g Foo'1<Some.Class> -> Foo'1[Some.Class] and Foo'1<!0> -> Foo'1[!0].
Refactored around + added unit tests in 4f6c7be.
| } | ||
|
|
||
| return types.ToArray(); | ||
| } |
There was a problem hiding this comment.
Do we have unit tests for that? IMO we must add some to verify the correctness because it's in a very critical path (adding probes).
Some suggestions:
P<T<V,K>> arg1, K<T<V>, P<K>>arg2<c>_someGeneratedType arg1, anotherOne<>c arg2- Also worth checking arguments with attributes and default values
There was a problem hiding this comment.
For the more esoteric signatures we shall first assess what the SymDb is uploading. I'm currently adding unit tests for the signature parser, including generics.
There was a problem hiding this comment.
Added the unit tests in 4f6c7be.
andrewlock
reviewed
Aug 1, 2024
| } | ||
| else | ||
| { | ||
| if (c == '<') |
There was a problem hiding this comment.
Yeah, does this need to handle unpronounceable types generated by the compiler? e.g. <PrivateImplementationDetails> and <>z__ReadOnlyArray<T>? I'd love to see unit tests for these if nothing else, to confirm that we bail-out at worst case, rather than generating something weird.
Similarly, do we need to handle open-generics?
GreenMatan
force-pushed
the
matang/symdb-fixes
branch
9 times, most recently
from
dc427de
to
a1b787b
Compare
andrewlock
approved these changes
Aug 5, 2024
| [InlineData("Class1<Class2<Class3<Class4>>>", new[] { "Class1[Class2[Class3[Class4]]]" })] | ||
| [InlineData("(Namespace1.Class1`1<Namespace2.Class2`1<Namespace3.Class3>)", null)] | ||
| [InlineData("Namespace1.Class1`1<Namespace2.<PrivateImplementationDetails>, Namespace3.Class3>", new[] { "Namespace1.Class1`1[Namespace2.[PrivateImplementationDetails],Namespace3.Class3]" })] | ||
| [InlineData("Namespace1.Class1`1<Namespace2.<>z__ReadOnlyArray<T>, Namespace3.Class3>", new[] { "Namespace1.Class1`1[Namespace2.<>z__ReadOnlyArray[T],Namespace3.Class3]" })] |
There was a problem hiding this comment.
I don't think this is actually a valid signature is it? 😅 Probably doesn't matter I guess (and maybe preferable in fact, as shows we're not dependent on parsing the `1
There was a problem hiding this comment.
Correct, it shall be `2 as there are two params. I don't parse out the amount of generics and verify that - not sure it's worth it. But in general you're right. If we'll improve the code to actually do that- those tests will fail 🤓
There was a problem hiding this comment.
I agree, I don't know that it's worth it either, just wanted to verify. And if we do decide to do that, this will serve as proof it works😃
| public class SignatureParserTests | ||
| { | ||
| [Theory] | ||
| [InlineData("(P<T<V,K>>, K<T<V>, P<K>>)", new[] { "P[T[V,K]]", "K[T[V],P[K]]" })] |
There was a problem hiding this comment.
Thanks for adding all these 😍
GreenMatan
force-pushed
the
matang/symdb-fixes
branch
from
a1b787b
to
1bd2d3a
Compare
GreenMatan
added a commit
that referenced
this pull request
Aug 5, 2024
…s received through SymDb (#5829) ## Summary of changes We are on the edge of advancing SymDb to Open Beta. We found a few issues that blocked us in doing so: - Generic types, methods and arguments received surrounded with `<` and `>` while the native matcher is based on `[` and `]`. - The parser of the signature in the managed side of the debugger implemented naively, mainly splitted by `,`. Generic params are comma separated too, which failed to parse it correctly. - Static methods were not handled correctly in the native side. It naively skipped the first param, as if all the methods it deals with are instance methods. - The signature received by SymDb is surrounded by `(` and `)` and has space in between every param type. ## Reason for change SymDb readiness for Open Beta. ## Implementation details - In the managed side, upon receiving a request to instrument a method with a signature that is targeting a generic method/type, the `<` and `>` characters are replaced with `[` and `]` respectively. - Parsing the signature in a more robust way that handles the generic case where comma could present in between every generic param. - Static methods are now correctly handled in the native side when a request arrives with a signature exact matching. - Correctly parsing the signature when it comes surrounded by `(` and `)`, including space in between every param type. ## Test coverage - [SignatureParserTests](https://github.com/DataDog/dd-trace-dotnet/blob/a8a2df542625b29e3f0d2ce8c745717f27abdb0a/tracer/test/Datadog.Trace.Debugger.IntegrationTests/SignatureParserTests.cs)
GreenMatan
added a commit
that referenced
this pull request
Aug 5, 2024
…s received through SymDb (#5829 -> v2) (#5847) ## Summary of changes We are on the edge of advancing SymDb to Open Beta. We found a few issues that blocked us in doing so: - Generic types, methods and arguments received surrounded with `<` and `>` while the native matcher is based on `[` and `]`. - The parser of the signature in the managed side of the debugger implemented naively, mainly splitted by `,`. Generic params are comma separated too, which failed to parse it correctly. - Static methods were not handled correctly in the native side. It naively skipped the first param, as if all the methods it deals with are instance methods. - The signature received by SymDb is surrounded by `(` and `)` and has space in between every param type. ## Reason for change SymDb readiness for Open Beta. ## Implementation details - In the managed side, upon receiving a request to instrument a method with a signature that is targeting a generic method/type, the `<` and `>` characters are replaced with `[` and `]` respectively. - Parsing the signature in a more robust way that handles the generic case where comma could present in between every generic param. - Static methods are now correctly handled in the native side when a request arrives with a signature exact matching. - Correctly parsing the signature when it comes surrounded by `(` and `)`, including space in between every param type. ## Test coverage - [SignatureParserTests](https://github.com/DataDog/dd-trace-dotnet/blob/a8a2df542625b29e3f0d2ce8c745717f27abdb0a/tracer/test/Datadog.Trace.Debugger.IntegrationTests/SignatureParserTests.cs)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
We are on the edge of advancing SymDb to Open Beta. We found a few issues that blocked us in doing so:
<and>while the native matcher is based on[and].,. Generic params are comma separated too, which failed to parse it correctly.(and)and has space in between every param type.Reason for change
SymDb readiness for Open Beta.
Implementation details
<and>characters are replaced with[and]respectively.(and), including space in between every param type.Test coverage