-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't instrument most dotnet
SDK calls
#5564
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 340013 Passed, 2330 Skipped, 22h 44m 59.3s Total Time |
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 (5564) - mean (74ms) : 63, 85
. : milestone, 74,
master - mean (75ms) : 67, 83
. : milestone, 75,
section CallTarget+Inlining+NGEN
This PR (5564) - mean (1,012ms) : 977, 1047
. : milestone, 1012,
master - mean (1,013ms) : 987, 1039
. : milestone, 1013,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5564) - mean (110ms) : 106, 114
. : milestone, 110,
master - mean (111ms) : 108, 115
. : milestone, 111,
section CallTarget+Inlining+NGEN
This PR (5564) - mean (722ms) : 696, 748
. : milestone, 722,
master - mean (729ms) : 708, 750
. : milestone, 729,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5564) - mean (93ms) : 90, 96
. : milestone, 93,
master - mean (94ms) : 90, 97
. : milestone, 94,
section CallTarget+Inlining+NGEN
This PR (5564) - mean (684ms) : 663, 705
. : milestone, 684,
master - mean (679ms) : 655, 703
. : milestone, 679,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5564) - mean (193ms) : 188, 199
. : milestone, 193,
master - mean (193ms) : 189, 196
. : milestone, 193,
section CallTarget+Inlining+NGEN
This PR (5564) - mean (1,113ms) : 1092, 1135
. : milestone, 1113,
master - mean (1,115ms) : 1091, 1139
. : milestone, 1115,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5564) - mean (275ms) : 270, 281
. : milestone, 275,
master - mean (276ms) : 270, 281
. : milestone, 276,
section CallTarget+Inlining+NGEN
This PR (5564) - mean (910ms) : 893, 928
. : milestone, 910,
master - mean (913ms) : 891, 935
. : milestone, 913,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5564) - mean (265ms) : 261, 270
. : milestone, 265,
master - mean (265ms) : 261, 269
. : milestone, 265,
section CallTarget+Inlining+NGEN
This PR (5564) - mean (894ms) : 871, 917
. : milestone, 894,
master - mean (895ms) : 873, 917
. : milestone, 895,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things. Good startup to move from C# to C++ 😆
Benchmarks Report for tracer 🐌Benchmarks for #5564 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.StartFinishScope‑netcoreapp3.1 | 1.152 | 835.23 | 962.50 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StartFinishSpan |
net6.0 | 522ns | 0.111ns | 0.415ns | 0.00805 | 0 | 0 | 576 B |
master | StartFinishSpan |
netcoreapp3.1 | 727ns | 1ns | 3.75ns | 0.00786 | 0 | 0 | 576 B |
master | StartFinishSpan |
net472 | 760ns | 0.269ns | 1.04ns | 0.0914 | 0 | 0 | 578 B |
master | StartFinishScope |
net6.0 | 564ns | 0.213ns | 0.739ns | 0.00974 | 0 | 0 | 696 B |
master | StartFinishScope |
netcoreapp3.1 | 835ns | 0.203ns | 0.732ns | 0.00962 | 0 | 0 | 696 B |
master | StartFinishScope |
net472 | 1.09μs | 1.01ns | 3.63ns | 0.104 | 0 | 0 | 658 B |
#5564 | StartFinishSpan |
net6.0 | 477ns | 0.306ns | 1.18ns | 0.00796 | 0 | 0 | 576 B |
#5564 | StartFinishSpan |
netcoreapp3.1 | 729ns | 0.776ns | 3.01ns | 0.0077 | 0 | 0 | 576 B |
#5564 | StartFinishSpan |
net472 | 798ns | 0.64ns | 2.22ns | 0.0917 | 0 | 0 | 578 B |
#5564 | StartFinishScope |
net6.0 | 617ns | 0.363ns | 1.36ns | 0.00982 | 0 | 0 | 696 B |
#5564 | StartFinishScope |
netcoreapp3.1 | 962ns | 0.564ns | 2.11ns | 0.00914 | 0 | 0 | 696 B |
#5564 | StartFinishScope |
net472 | 1.03μs | 0.88ns | 3.41ns | 0.104 | 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 | 690ns | 0.356ns | 1.38ns | 0.00997 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
netcoreapp3.1 | 873ns | 0.393ns | 1.52ns | 0.00953 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
net472 | 1.22μs | 0.522ns | 2.02ns | 0.104 | 0 | 0 | 658 B |
#5564 | RunOnMethodBegin |
net6.0 | 639ns | 0.144ns | 0.519ns | 0.00965 | 0 | 0 | 696 B |
#5564 | RunOnMethodBegin |
netcoreapp3.1 | 909ns | 0.441ns | 1.65ns | 0.00907 | 0 | 0 | 696 B |
#5564 | RunOnMethodBegin |
net472 | 1.17μs | 0.761ns | 2.95ns | 0.104 | 0 | 0 | 658 B |
Throughput/Crank Report:zap: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 (5564) (11.791M) : 0, 11790681
master (11.879M) : 0, 11879110
benchmarks/2.9.0 (11.983M) : 0, 11982622
section Automatic
This PR (5564) (8.007M) : 0, 8007275
master (8.054M) : 0, 8053530
benchmarks/2.9.0 (8.324M) : 0, 8324349
section Trace stats
master (8.362M) : 0, 8361687
section Manual
This PR (5564) (10.251M) : 0, 10251409
master (10.229M) : 0, 10229080
section Manual + Automatic
This PR (5564) (7.648M) : 0, 7648286
master (7.641M) : 0, 7641349
section Version Conflict
master (6.825M) : 0, 6824800
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5564) (9.666M) : 0, 9665885
master (9.599M) : 0, 9598656
benchmarks/2.9.0 (9.587M) : 0, 9587201
section Automatic
This PR (5564) (6.638M) : 0, 6638144
master (6.588M) : 0, 6588326
section Trace stats
master (6.893M) : 0, 6892670
section Manual
This PR (5564) (8.228M) : 0, 8227740
master (8.131M) : 0, 8130516
section Manual + Automatic
This PR (5564) (6.252M) : 0, 6252495
master (6.167M) : 0, 6167127
section Version Conflict
master (5.576M) : 0, 5575919
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5564) (9.809M) : 0, 9809076
master (9.987M) : 0, 9986822
benchmarks/2.9.0 (9.913M) : 0, 9912627
section Automatic
This PR (5564) (6.961M) : 0, 6961022
master (6.986M) : 0, 6985911
benchmarks/2.9.0 (7.312M) : 0, 7312269
section Trace stats
master (7.286M) : 0, 7285868
section Manual
This PR (5564) (8.550M) : 0, 8549982
master (8.789M) : 0, 8788786
section Manual + Automatic
This PR (5564) (6.701M) : 0, 6700898
master (6.745M) : 0, 6745109
section Version Conflict
master (6.204M) : 0, 6204499
|
tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/InstrumentationTests.cs
Outdated
Show resolved
Hide resolved
…command we don't want to instrument
Co-authored-by: Gregory LEOCADIE <gregory.leocadie@datadoghq.com>
a770a25
to
c2025ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM tested in macOS 👍🏻 all good.
Co-authored-by: Kevin Gosse <kevin.gosse@datadoghq.com>
Summary of changes
If we detect that we're running a .NET SDK command, bail-out of instrumentation
Reason for change
There are bugs in the runtime, particularly around event pipes, which can cause crashes on exit. Instrumenting .NET SDK commands (like
build
orrestore
) is unlikely to be of benefit to customers, will increase latency, and comes at the risk of stability issues, so makes sense to bail out. This is particularly important in auto-instrumentation/single step scenarios.Note that there are obviously some
dotnet
commands we do want to instrument:dotnet test
andexec testhost
dotnet run
as instrumented, although AFAICT, it always callsexec MyApp.exe
or similar so we theoretically wouldn't need to? Seemed safer to keep it thoughdotnet exec
for known commands like csc.dll and VBCSCompiler.exeImplementation details
GetCurrentProcessCommandLine()
function to return both the "full" command line as well as the individual argumentsTest coverage
Added integration checks to confirm that we don't instrument the following:
But that we do instrument
All our existing tests cover the standard mode of
dotnet MyApp.dll
Other details
Hopefully resolves internal ticket: https://datadoghq.atlassian.net/browse/AIT-9034
These are example logs from the native loader for a
dotnet build
command: