[Tool] update continuous profiler diagnostics #6014
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
andrewlock
added
area:tools
area:profiler
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 (6014) - mean (70ms) : 67, 72
. : milestone, 70,
master - mean (69ms) : 67, 72
. : milestone, 69,
section CallTarget+Inlining+NGEN
This PR (6014) - mean (1,114ms) : 1095, 1133
. : milestone, 1114,
master - mean (1,116ms) : 1094, 1137
. : milestone, 1116,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6014) - mean (108ms) : 105, 111
. : milestone, 108,
master - mean (109ms) : 103, 114
. : milestone, 109,
section CallTarget+Inlining+NGEN
This PR (6014) - mean (814ms) : 801, 827
. : milestone, 814,
master - mean (816ms) : 797, 834
. : milestone, 816,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6014) - mean (93ms) : 90, 95
. : milestone, 93,
master - mean (92ms) : 89, 94
. : milestone, 92,
section CallTarget+Inlining+NGEN
This PR (6014) - mean (770ms) : 755, 785
. : milestone, 770,
master - mean (768ms) : 753, 784
. : milestone, 768,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6014) - mean (190ms) : 188, 193
. : milestone, 190,
master - mean (191ms) : 188, 193
. : milestone, 191,
section CallTarget+Inlining+NGEN
This PR (6014) - mean (1,194ms) : 1171, 1218
. : milestone, 1194,
master - mean (1,194ms) : 1171, 1217
. : milestone, 1194,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6014) - mean (276ms) : 272, 281
. : milestone, 276,
master - mean (276ms) : 271, 280
. : milestone, 276,
section CallTarget+Inlining+NGEN
This PR (6014) - mean (972ms) : 955, 988
. : milestone, 972,
master - mean (974ms) : 955, 993
. : milestone, 974,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6014) - mean (265ms) : 260, 270
. : milestone, 265,
master - mean (265ms) : 260, 269
. : milestone, 265,
section CallTarget+Inlining+NGEN
This PR (6014) - mean (955ms) : 927, 982
. : milestone, 955,
master - mean (959ms) : 940, 978
. : milestone, 959,
|
Datadog ReportBranch report: ✅ 0 Failed, 359812 Passed, 2052 Skipped, 15h 10m 54.39s 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 (6014) (11.361M) : 0, 11361016
master (11.283M) : 0, 11283455
benchmarks/2.9.0 (11.185M) : 0, 11185492
section Automatic
This PR (6014) (7.462M) : 0, 7461794
master (7.348M) : 0, 7348323
benchmarks/2.9.0 (7.935M) : 0, 7934520
section Trace stats
master (7.775M) : 0, 7774621
section Manual
master (10.877M) : 0, 10876544
section Manual + Automatic
This PR (6014) (6.893M) : 0, 6892787
master (6.702M) : 0, 6702266
section DD_TRACE_ENABLED=0
master (10.066M) : 0, 10066409
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6014) (9.465M) : 0, 9464744
master (9.597M) : 0, 9597221
benchmarks/2.9.0 (9.560M) : 0, 9559569
section Automatic
This PR (6014) (6.583M) : 0, 6582760
master (6.662M) : 0, 6661866
section Trace stats
master (6.698M) : 0, 6698477
section Manual
master (9.563M) : 0, 9563004
section Manual + Automatic
This PR (6014) (6.273M) : 0, 6272810
master (6.210M) : 0, 6210292
section DD_TRACE_ENABLED=0
master (8.980M) : 0, 8979687
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6014) (10.272M) : 0, 10272477
master (10.014M) : 0, 10013811
benchmarks/2.9.0 (10.118M) : 0, 10117626
section Automatic
This PR (6014) (6.805M) : 0, 6804899
master (6.639M) : 0, 6638596
benchmarks/2.9.0 (7.416M) : 0, 7415715
section Trace stats
master (7.359M) : 0, 7358547
section Manual
master (10.064M) : 0, 10063800
section Manual + Automatic
This PR (6014) (6.208M) : 0, 6207629
master (6.067M) : 0, 6066682
section DD_TRACE_ENABLED=0
master (9.407M) : 0, 9406887
|
andrewlock
force-pushed
the
andrew/profiler_ssi
branch
from
750cf67
to
8bed0e8
Compare
chrisnas
approved these changes
Sep 10, 2024
| ? string.Empty | ||
| : ssiInjectionEnabled == true ? "tracer,profiler" : "tracer"; | ||
|
|
||
| var expected = (enabled, ssiInjectionEnabled) switch |
There was a problem hiding this comment.
Maybe extract this code as a helper method and call it also from WorkingWithContinuousProfiler() instead of duplicating the code
There was a problem hiding this comment.
Unfortunately easier said than done, as these are in completely different projects etc. We could pull it into a separate file that's linked in both, I'll have a look, thanks 👍 Worst case I'll add a comment
Benchmarks Report for tracer 🐌Benchmarks for #6014 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 - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️Raw results
|
Co-authored-by: chrisnas <chrisnas@users.noreply.github.com>
andrewlock
force-pushed
the
andrew/profiler_ssi
branch
from
c06b6db
to
1c46986
Compare
andrewlock
added a commit
that referenced
this pull request
Sep 11, 2024
## Summary of changes Fix profiler integration tests in the SSI run ## Reason for change #5240 changed the behaviour of the profiler in the presence of SSI variables. We have a scheduled run on master that specifically sets the variables. This breaks the profiler integration tests that depend on those variables. ## Implementation details Add an "environment restorer" attribute, which mirrors the one we use in the tracing integration tests. Ensures that the SSI tests are working in their expected environment ## Test coverage This is the test, but did [a dedicated run too](https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=163728&view=results), and it works ## Other details Related to #6014
bouwkast
approved these changes
Sep 11, 2024
| @@ -46,9 +46,11 @@ internal static class Resources | |||
| public const string DdAgentChecks = "---- DATADOG AGENT CHECKS -----"; | |||
|
|
|||
| public const string ContinuousProfilerEnabled = "DD_PROFILING_ENABLED is set."; | |||
| public const string ContinuousProfilerSsiDeployed = "DD_INJECTION_ENABLED is set. Profiler is deployed through SSI."; | |||
| public const string ContinuousProfilerEnabledWithHeuristics = "DD_PROFILING_ENABLED is set to 'auto'. The continuous profiler is enabled and may begin profiling based on heuristics."; | |||
There was a problem hiding this comment.
Does may indicate that it also may not? Essentially wondering if may == will here.
There was a problem hiding this comment.
Correct, it might not start profiling, depending on the heuristics. e.g. if it's a short-lived process, or a process where there's no tracing happening
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
Fix the diagnostics associated with the expected state of the continuous profiler
Reason for change
In #5240, the continuous profiler gained two new "modes" of operation: delayed enablement based on heuristics, and "monitoring" mode. The tool should be updated to understand those possibilities + handle it in the tests
Implementation details
DD_INJECTION_ENABLEDvariable before running the tests - the presence/absence of the variable changes the expectations, so we need to account for itDD_INJECTION_ENABLEDandDD_PROFILING_ENABLED.Test coverage
Added additional integration and artifact tests
Other details
This should address most of the issues in the scheduled SSI run. I suspect the windows integration tests may need additional work, as currently the profiler library isn't copied to those tests. Will fix in this PR if that ends up being the case
Supersedes