Use dd-dotnet in dd-trace #4732
Merged
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
Datadog ReportBranch report: ✅ |
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 (4732) - mean (71ms) : 64, 78
. : milestone, 71,
master - mean (69ms) : 63, 76
. : milestone, 69,
section CallTarget+Inlining+NGEN
This PR (4732) - mean (1,023ms) : 1001, 1045
. : milestone, 1023,
master - mean (1,013ms) : 994, 1032
. : milestone, 1013,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4732) - mean (106ms) : 104, 108
. : milestone, 106,
master - mean (107ms) : 96, 118
. : milestone, 107,
section CallTarget+Inlining+NGEN
This PR (4732) - mean (716ms) : 700, 732
. : milestone, 716,
master - mean (716ms) : 695, 737
. : milestone, 716,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4732) - mean (91ms) : 88, 93
. : milestone, 91,
master - mean (90ms) : 84, 96
. : milestone, 90,
section CallTarget+Inlining+NGEN
This PR (4732) - mean (681ms) : 669, 693
. : milestone, 681,
master - mean (682ms) : 653, 711
. : milestone, 682,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4732) - mean (188ms) : 185, 190
. : milestone, 188,
master - mean (188ms) : 185, 192
. : milestone, 188,
section CallTarget+Inlining+NGEN
This PR (4732) - mean (1,128ms) : 1109, 1147
. : milestone, 1128,
master - mean (1,127ms) : 1109, 1146
. : milestone, 1127,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4732) - mean (271ms) : 268, 274
. : milestone, 271,
master - mean (272ms) : 269, 274
. : milestone, 272,
section CallTarget+Inlining+NGEN
This PR (4732) - mean (1,091ms) : 1060, 1123
. : milestone, 1091,
master - mean (1,090ms) : 1069, 1110
. : milestone, 1090,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4732) - mean (260ms) : 256, 265
. : milestone, 260,
master - mean (261ms) : 255, 267
. : milestone, 261,
section CallTarget+Inlining+NGEN
This PR (4732) - mean (1,053ms) : 1028, 1079
. : milestone, 1053,
master - mean (1,055ms) : 1022, 1087
. : milestone, 1055,
|
Benchmarks Report 🐌Benchmarks for #4732 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.Asm.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecWafBenchmark - 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 - Faster 🎉 Same allocations ✔️
|
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 | 1.156 | 471.05 | 407.62 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 471ns | 0.189ns | 0.732ns | 0.00755 | 0 | 0 | 536 B |
| master | StartFinishSpan |
netcoreapp3.1 | 580ns | 0.271ns | 1.05ns | 0.00728 | 0 | 0 | 536 B |
| master | StartFinishSpan |
net472 | 684ns | 0.117ns | 0.451ns | 0.0854 | 0 | 0 | 538 B |
| master | StartFinishScope |
net6.0 | 535ns | 0.154ns | 0.577ns | 0.00903 | 0 | 0 | 656 B |
| master | StartFinishScope |
netcoreapp3.1 | 754ns | 0.158ns | 0.59ns | 0.00866 | 0 | 0 | 656 B |
| master | StartFinishScope |
net472 | 881ns | 0.359ns | 1.34ns | 0.098 | 0 | 0 | 618 B |
| #4732 | StartFinishSpan |
net6.0 | 407ns | 0.479ns | 1.86ns | 0.00747 | 0 | 0 | 536 B |
| #4732 | StartFinishSpan |
netcoreapp3.1 | 524ns | 1.97ns | 7.65ns | 0.00727 | 0 | 0 | 536 B |
| #4732 | StartFinishSpan |
net472 | 688ns | 0.103ns | 0.399ns | 0.0852 | 0 | 0 | 538 B |
| #4732 | StartFinishScope |
net6.0 | 567ns | 0.143ns | 0.555ns | 0.00913 | 0 | 0 | 656 B |
| #4732 | StartFinishScope |
netcoreapp3.1 | 736ns | 0.22ns | 0.853ns | 0.00892 | 0 | 0 | 656 B |
| #4732 | StartFinishScope |
net472 | 857ns | 1.19ns | 4.6ns | 0.098 | 0 | 0 | 618 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 | 586ns | 0.177ns | 0.686ns | 0.00908 | 0 | 0 | 656 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 812ns | 2.49ns | 9.66ns | 0.00872 | 0 | 0 | 656 B |
| master | RunOnMethodBegin |
net472 | 917ns | 0.284ns | 1.1ns | 0.098 | 0 | 0 | 618 B |
| #4732 | RunOnMethodBegin |
net6.0 | 579ns | 0.194ns | 0.753ns | 0.00927 | 0 | 0 | 656 B |
| #4732 | RunOnMethodBegin |
netcoreapp3.1 | 798ns | 0.189ns | 0.731ns | 0.00889 | 0 | 0 | 656 B |
| #4732 | RunOnMethodBegin |
net472 | 1.02μs | 0.347ns | 1.34ns | 0.098 | 0 | 0 | 618 B |
kevingosse
added
the
status:work-in-progress
kevingosse
removed
the
status:work-in-progress
kevingosse
force-pushed
the
kevin/merge_tools
branch
from
0c7592f to
cc1f9f7
Compare
andrewlock
approved these changes
Oct 20, 2023
Comment on lines
+549
to
+550
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Datadog.Trace.Tools.Shared", "tracer\src\Datadog.Trace.Tools.Shared\Datadog.Trace.Tools.Shared.csproj", "{7A1B9BFE-052D-435E-B27F-72AF3DDCC0A0}" | ||
| EndProject |
There was a problem hiding this comment.
Don't know if it's any easier to use a shared project rather than a normal project here? You wouldn't need to specify target frameworks, no need for "internals visible to", changing visibility etc🤷♂️
There was a problem hiding this comment.
Dunno. If I did that right away then maybe, but now I don't really see a significant benefit to changing it.
tracer/src/Datadog.Trace.Tools.Shared/System.Diagnostics.CodeAnalysis.Attributes.cs
Outdated
Show resolved
Hide resolved
| { | ||
| internal static class NativeMethods | ||
| public static class NativeMethods |
There was a problem hiding this comment.
You wouldn't need to change these if you used a shared library instead of a normal project 😉
| { | ||
| baseDirectory ??= Path.GetDirectoryName(process.MainModule); | ||
|
|
||
| var configurationSource = new CompositeConfigurationSource(); |
There was a problem hiding this comment.
Don't you want to be using CompositeConfigurationSourceInternal here? 🤔 Because this is a public API, and will try to record telemetry for it
There was a problem hiding this comment.
That's the dd-dotnet CompositeConfigurationSource, not the tracer one. It has no telemetry. I can rename it if it's too confusing, but I don't have great ideas for the new name.
tracer/src/Datadog.Trace.Tools.dd_dotnet/CommandWithExamples.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I believe we're deleting these because we already have equivalent tests for dd-dotnet, but is there any value in making sure that these tests do still run correctly when going via dd-dotnet or is that more hassle to implement than it's worth?
From my Pov, I'd have more confidence of "we haven't broken something" if we didn't also delete all the tests for that functionality at the same time 😛 🙈
There was a problem hiding this comment.
dd-trace cannot invoke dd-dotnet in the integration tests (only in the artifact tests).
I could duplicate the dd-dotnet artifact tests to run with dd-trace, but I'm not sure it's worth the hassle 🤔
kevingosse
force-pushed
the
kevin/merge_tools
branch
from
c7a1b7c to
da5cbfb
Compare
link04
approved these changes
Oct 24, 2023
andrewlock
added
type:enhancement
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
Remove the diagnostic part of dd-trace and have it rely on dd-dotnet instead.
Reason for change
The diagnostic code is currently duplicated between dd-trace and dd-dotnet, and can't be easily shared (because dd-trace targets all .net core versions back to 2.1, whereas dd-dotnet relies on more recent features for some diagnostics).
Since dd-dotnet is bundled anyway, having dd-trace call it seems like a good middle ground.
Implementation details
The AnalyzeInstrumentationErrors command relies on ProcessInfo, so I had to introduce a shared project for this class and its immediate dependencies.
When calling dd-dotnet, dd-trace sets the
DD_OVERRIDE_COMMANDenvironment variable to change the name of the executable in the help messagesTest coverage
The coverage should be the same as before. A unit test projects has been added for dd-dotnet.
One caveat is that the ProcessInfo tests were moved to the dd-dotnet test project, which means that they will run only for .NET 7. That's because they depend on
ExtractConfigurationSourcewhich was moved to dd-dotnet. In theory it could go to the shared project, but it's fairly complex because of the configuration sources that also exist in the tracer (so we have namespace conflicts).Other details
Depends on #4725