-
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
Tiny fix for crashing if DD_PROFILER_ENABLED
is not set
#4849
Conversation
@@ -33,7 +33,7 @@ internal static IConfig WithDatadog(this IConfig config, bool? enableProfiler) | |||
{ | |||
var cfg = config.AddLogger(DatadogSessionLogger.Default); | |||
|
|||
enableProfiler ??= (EnvironmentHelpers.GetEnvironmentVariable(ConfigurationKeys.ProfilingEnabled) ?? string.Empty).ToBoolean(); | |||
enableProfiler ??= (EnvironmentHelpers.GetEnvironmentVariable(ConfigurationKeys.ProfilingEnabled) ?? string.Empty).ToBoolean() ?? false; |
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.
If enableProfiler
is null
, and the ConfigurationKeys.ProfilingEnabled
key isn't set, ToBoolean()
returns a nullable, which we then try to unwrap on Line 47 and 💥
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.
Then we probably don't need the !
at line 47 anymore?
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 (4849) - mean (72ms) : 63, 81
. : milestone, 72,
master - mean (70ms) : 62, 79
. : milestone, 70,
section CallTarget+Inlining+NGEN
This PR (4849) - mean (1,003ms) : 980, 1025
. : milestone, 1003,
master - mean (993ms) : 970, 1015
. : milestone, 993,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4849) - mean (106ms) : 104, 109
. : milestone, 106,
master - mean (106ms) : 102, 110
. : milestone, 106,
section CallTarget+Inlining+NGEN
This PR (4849) - mean (689ms) : 671, 707
. : milestone, 689,
master - mean (689ms) : 673, 706
. : milestone, 689,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4849) - mean (90ms) : 87, 93
. : milestone, 90,
master - mean (89ms) : 85, 93
. : milestone, 89,
section CallTarget+Inlining+NGEN
This PR (4849) - mean (660ms) : 635, 686
. : milestone, 660,
master - mean (654ms) : 628, 681
. : milestone, 654,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4849) - mean (187ms) : 185, 190
. : milestone, 187,
master - mean (187ms) : 184, 190
. : milestone, 187,
section CallTarget+Inlining+NGEN
This PR (4849) - mean (1,097ms) : 1078, 1116
. : milestone, 1097,
master - mean (1,097ms) : 1078, 1116
. : milestone, 1097,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4849) - mean (272ms) : 267, 276
. : milestone, 272,
master - mean (272ms) : 268, 277
. : milestone, 272,
section CallTarget+Inlining+NGEN
This PR (4849) - mean (1,051ms) : 1035, 1067
. : milestone, 1051,
master - mean (1,054ms) : 1027, 1081
. : milestone, 1054,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4849) - mean (263ms) : 259, 266
. : milestone, 263,
master - mean (262ms) : 259, 265
. : milestone, 262,
section CallTarget+Inlining+NGEN
This PR (4849) - mean (1,031ms) : 1009, 1053
. : milestone, 1031,
master - mean (1,029ms) : 1002, 1057
. : milestone, 1029,
|
@@ -33,7 +33,7 @@ internal static IConfig WithDatadog(this IConfig config, bool? enableProfiler) | |||
{ | |||
var cfg = config.AddLogger(DatadogSessionLogger.Default); | |||
|
|||
enableProfiler ??= (EnvironmentHelpers.GetEnvironmentVariable(ConfigurationKeys.ProfilingEnabled) ?? string.Empty).ToBoolean(); | |||
enableProfiler ??= (EnvironmentHelpers.GetEnvironmentVariable(ConfigurationKeys.ProfilingEnabled) ?? string.Empty).ToBoolean() ?? false; |
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.
Then we probably don't need the !
at line 47 anymore?
Benchmarks Report 🐌Benchmarks for #4849 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 - Slower
|
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync‑netcoreapp3.1 | 1.143 | 1,377.26 | 1,573.66 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | ExecuteAsync |
net6.0 | 1.19μs | 0.534ns | 2ns | 0.0126 | 0 | 0 | 912 B |
master | ExecuteAsync |
netcoreapp3.1 | 1.38μs | 0.94ns | 3.64ns | 0.0124 | 0 | 0 | 912 B |
master | ExecuteAsync |
net472 | 1.61μs | 0.901ns | 3.49ns | 0.139 | 0 | 0 | 875 B |
#4849 | ExecuteAsync |
net6.0 | 1.31μs | 0.646ns | 2.42ns | 0.0125 | 0 | 0 | 912 B |
#4849 | ExecuteAsync |
netcoreapp3.1 | 1.57μs | 0.86ns | 3.33ns | 0.0126 | 0 | 0 | 912 B |
#4849 | ExecuteAsync |
net472 | 1.75μs | 0.777ns | 2.8ns | 0.139 | 0 | 0 | 875 B |
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | SendAsync |
net6.0 | 3.88μs | 1.66ns | 6.2ns | 0.0253 | 0 | 0 | 1.9 KB |
master | SendAsync |
netcoreapp3.1 | 4.52μs | 6.73ns | 26.1ns | 0.0314 | 0 | 0 | 2.43 KB |
master | SendAsync |
net472 | 6.96μs | 2.7ns | 10.5ns | 0.475 | 0 | 0 | 2.99 KB |
#4849 | SendAsync |
net6.0 | 3.81μs | 1.24ns | 4.8ns | 0.0268 | 0 | 0 | 1.9 KB |
#4849 | SendAsync |
netcoreapp3.1 | 4.38μs | 1.65ns | 5.95ns | 0.0331 | 0 | 0 | 2.43 KB |
#4849 | SendAsync |
net472 | 7.07μs | 4.8ns | 18.6ns | 0.475 | 0 | 0 | 2.99 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 |
net6.0 | 1.34μs | 0.422ns | 1.58ns | 0.0222 | 0 | 0 | 1.57 KB |
master | EnrichedLog |
netcoreapp3.1 | 1.94μs | 2.81ns | 10.1ns | 0.0213 | 0 | 0 | 1.57 KB |
master | EnrichedLog |
net472 | 2.24μs | 1.29ns | 4.99ns | 0.236 | 0 | 0 | 1.49 KB |
#4849 | EnrichedLog |
net6.0 | 1.42μs | 0.717ns | 2.78ns | 0.022 | 0 | 0 | 1.57 KB |
#4849 | EnrichedLog |
netcoreapp3.1 | 1.93μs | 0.997ns | 3.73ns | 0.0209 | 0 | 0 | 1.57 KB |
#4849 | EnrichedLog |
net472 | 2.36μs | 0.833ns | 3.23ns | 0.237 | 0 | 0 | 1.49 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 |
net6.0 | 113μs | 80.9ns | 303ns | 0 | 0 | 0 | 4.21 KB |
master | EnrichedLog |
netcoreapp3.1 | 118μs | 157ns | 607ns | 0 | 0 | 0 | 4.21 KB |
master | EnrichedLog |
net472 | 149μs | 322ns | 1.25μs | 0.662 | 0.221 | 0 | 4.38 KB |
#4849 | EnrichedLog |
net6.0 | 112μs | 150ns | 582ns | 0.0558 | 0 | 0 | 4.21 KB |
#4849 | EnrichedLog |
netcoreapp3.1 | 116μs | 82.8ns | 299ns | 0.0579 | 0 | 0 | 4.21 KB |
#4849 | EnrichedLog |
net472 | 149μs | 94.2ns | 340ns | 0.669 | 0.223 | 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 |
net6.0 | 2.74μs | 1.24ns | 4.64ns | 0.029 | 0 | 0 | 2.13 KB |
master | EnrichedLog |
netcoreapp3.1 | 3.84μs | 2.14ns | 8.01ns | 0.0288 | 0 | 0 | 2.13 KB |
master | EnrichedLog |
net472 | 4.59μs | 1.37ns | 5.11ns | 0.306 | 0 | 0 | 1.93 KB |
#4849 | EnrichedLog |
net6.0 | 2.99μs | 1.17ns | 4.54ns | 0.0298 | 0 | 0 | 2.13 KB |
#4849 | EnrichedLog |
netcoreapp3.1 | 3.72μs | 1.12ns | 4.04ns | 0.0279 | 0 | 0 | 2.13 KB |
#4849 | EnrichedLog |
net472 | 4.5μs | 1.46ns | 5.48ns | 0.307 | 0 | 0 | 1.93 KB |
Benchmarks.Trace.RedisBenchmark - Faster 🎉 Same allocations ✔️
Faster 🎉 in #4849
Benchmark
base/diff
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.RedisBenchmark.SendReceive‑net6.0
1.183
1,449.67
1,225.51
Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.RedisBenchmark.SendReceive‑net6.0 | 1.183 | 1,449.67 | 1,225.51 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | SendReceive |
net6.0 | 1.45μs | 0.962ns | 3.73ns | 0.0152 | 0 | 0 | 1.1 KB |
master | SendReceive |
netcoreapp3.1 | 1.67μs | 7.89ns | 30.6ns | 0.0143 | 0 | 0 | 1.1 KB |
master | SendReceive |
net472 | 1.93μs | 2.85ns | 11.1ns | 0.177 | 0 | 0 | 1.12 KB |
#4849 | SendReceive |
net6.0 | 1.23μs | 0.462ns | 1.73ns | 0.0153 | 0 | 0 | 1.1 KB |
#4849 | SendReceive |
netcoreapp3.1 | 1.59μs | 2.35ns | 8.79ns | 0.0152 | 0 | 0 | 1.1 KB |
#4849 | SendReceive |
net472 | 1.8μs | 1.03ns | 3.97ns | 0.176 | 0 | 0 | 1.12 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 |
net6.0 | 2.63μs | 0.951ns | 3.56ns | 0.0209 | 0 | 0 | 1.53 KB |
master | EnrichedLog |
netcoreapp3.1 | 3.65μs | 1.14ns | 4.41ns | 0.0201 | 0 | 0 | 1.58 KB |
master | EnrichedLog |
net472 | 4.13μs | 1.96ns | 7.61ns | 0.31 | 0 | 0 | 1.96 KB |
#4849 | EnrichedLog |
net6.0 | 2.63μs | 1.31ns | 5.07ns | 0.0211 | 0 | 0 | 1.53 KB |
#4849 | EnrichedLog |
netcoreapp3.1 | 3.51μs | 0.662ns | 2.48ns | 0.0211 | 0 | 0 | 1.58 KB |
#4849 | EnrichedLog |
net472 | 3.93μs | 1.28ns | 4.97ns | 0.31 | 0 | 0 | 1.96 KB |
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #4849
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472
1.133
609.30
690.56
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0
1.124
516.74
580.84
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472 | 1.133 | 609.30 | 690.56 | |
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 | 1.124 | 516.74 | 580.84 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StartFinishSpan |
net6.0 | 411ns | 0.323ns | 1.25ns | 0.00757 | 0 | 0 | 536 B |
master | StartFinishSpan |
netcoreapp3.1 | 496ns | 0.141ns | 0.529ns | 0.00739 | 0 | 0 | 536 B |
master | StartFinishSpan |
net472 | 609ns | 0.193ns | 0.748ns | 0.0853 | 0 | 0 | 538 B |
master | StartFinishScope |
net6.0 | 517ns | 0.188ns | 0.728ns | 0.00935 | 0 | 0 | 656 B |
master | StartFinishScope |
netcoreapp3.1 | 710ns | 0.248ns | 0.929ns | 0.00885 | 0 | 0 | 656 B |
master | StartFinishScope |
net472 | 860ns | 0.512ns | 1.98ns | 0.0981 | 0 | 0 | 618 B |
#4849 | StartFinishSpan |
net6.0 | 454ns | 0.196ns | 0.758ns | 0.00754 | 0 | 0 | 536 B |
#4849 | StartFinishSpan |
netcoreapp3.1 | 517ns | 0.337ns | 1.22ns | 0.00739 | 0 | 0 | 536 B |
#4849 | StartFinishSpan |
net472 | 691ns | 0.221ns | 0.856ns | 0.0851 | 0 | 0 | 538 B |
#4849 | StartFinishScope |
net6.0 | 581ns | 0.224ns | 0.868ns | 0.00914 | 0 | 0 | 656 B |
#4849 | StartFinishScope |
netcoreapp3.1 | 780ns | 0.885ns | 3.43ns | 0.00885 | 0 | 0 | 656 B |
#4849 | StartFinishScope |
net472 | 839ns | 0.662ns | 2.56ns | 0.0979 | 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 | 615ns | 0.306ns | 1.18ns | 0.00946 | 0 | 0 | 656 B |
master | RunOnMethodBegin |
netcoreapp3.1 | 834ns | 0.227ns | 0.88ns | 0.00876 | 0 | 0 | 656 B |
master | RunOnMethodBegin |
net472 | 1μs | 0.426ns | 1.65ns | 0.0979 | 0 | 0 | 618 B |
#4849 | RunOnMethodBegin |
net6.0 | 622ns | 0.273ns | 0.945ns | 0.00909 | 0 | 0 | 656 B |
#4849 | RunOnMethodBegin |
netcoreapp3.1 | 779ns | 0.279ns | 1.04ns | 0.00884 | 0 | 0 | 656 B |
#4849 | RunOnMethodBegin |
net472 | 999ns | 0.407ns | 1.58ns | 0.0981 | 0 | 0 | 618 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 (4849) (11.075M) : 0, 11074747
master (11.365M) : 0, 11364660
benchmarks/2.9.0 (11.650M) : 0, 11650156
section Automatic
This PR (4849) (7.665M) : 0, 7665312
master (7.952M) : 0, 7952033
benchmarks/2.9.0 (8.463M) : 0, 8463497
section Trace stats
This PR (4849) (7.983M) : 0, 7982719
master (8.125M) : 0, 8124896
section Manual
This PR (4849) (9.618M) : 0, 9618377
master (9.996M) : 0, 9996189
section Manual + Automatic
This PR (4849) (7.059M) : crit ,0, 7058592
master (7.477M) : 0, 7476818
section Version Conflict
This PR (4849) (6.625M) : 0, 6625225
master (6.908M) : 0, 6907880
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (4849) (9.544M) : 0, 9544411
master (9.555M) : 0, 9554556
benchmarks/2.9.0 (9.454M) : 0, 9454103
section Automatic
This PR (4849) (6.611M) : 0, 6610609
master (6.532M) : 0, 6531809
section Trace stats
This PR (4849) (7.059M) : 0, 7059411
master (6.948M) : 0, 6948235
section Manual
This PR (4849) (8.480M) : 0, 8480163
master (8.325M) : 0, 8325246
section Manual + Automatic
This PR (4849) (6.228M) : 0, 6228348
master (6.277M) : 0, 6277324
section Version Conflict
This PR (4849) (5.742M) : 0, 5741768
master (5.924M) : 0, 5924344
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (4849) (8.942M) : 0, 8941980
master (10.211M) : 0, 10211482
benchmarks/2.9.0 (9.974M) : 0, 9974322
section Automatic
This PR (4849) (7.226M) : 0, 7225749
master (7.507M) : 0, 7507323
benchmarks/2.9.0 (7.408M) : 0, 7408367
section Trace stats
This PR (4849) (7.661M) : 0, 7660979
master (7.883M) : 0, 7882755
section Manual
This PR (4849) (9.371M) : 0, 9370552
master (9.456M) : 0, 9456127
section Manual + Automatic
This PR (4849) (6.903M) : crit ,0, 6903364
master (7.282M) : 0, 7282409
section Version Conflict
This PR (4849) (6.403M) : 0, 6402676
master (6.615M) : 0, 6615388
gantt
title Throughput Linux x64 (ASM) (Total requests)
dateFormat X
axisFormat %s
section Baseline
master (7.687M) : 0, 7687290
benchmarks/2.9.0 (7.940M) : 0, 7940428
section No attack
master (2.179M) : 0, 2179090
benchmarks/2.9.0 (3.229M) : 0, 3229263
section Attack
master (1.706M) : 0, 1706183
benchmarks/2.9.0 (2.541M) : 0, 2540724
section Blocking
master (3.530M) : 0, 3530105
section IAST default
master (6.727M) : 0, 6726784
section IAST full
master (6.129M) : 0, 6128654
section Base vuln
master (0.990M) : 0, 990312
section IAST vuln
master (0.867M) : 0, 867410
|
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
Summary of changes
Fix crash when trying to run Benchmarks.Trace locally
Reason for change
Was trying to run benchmarks locally, and getting crashes
Implementation details
Add a fallback default for
enableProfiler
so that we don't getNullReferenceExceptions
Test coverage
Ran it locally, it works
Other details