-
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
Fix scenario where dogstatsd tries to send to the wrong hostname #5222
Conversation
/// <summary> | ||
/// Gets or sets the agent host to use when <see cref="MetricsTransport"/> is <see cref="TransportType.UDP"/> | ||
/// </summary> | ||
[IgnoreForSnapshot] // We don't record this in telemetry currently, but if we do, we'll record it when we set its |
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.
Its what? the suspense is killing me
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.
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.
FWIW, fixed the comment typo in both in the stacked PR
Datadog ReportBranch report: ✅ 0 Failed, 307234 Passed, 1578 Skipped, 40m 36.6s Wall 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 (5222) - mean (76ms) : 70, 83
. : milestone, 76,
master - mean (74ms) : 65, 82
. : milestone, 74,
section CallTarget+Inlining+NGEN
This PR (5222) - mean (987ms) : 969, 1005
. : milestone, 987,
master - mean (979ms) : 952, 1006
. : milestone, 979,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5222) - mean (113ms) : 109, 116
. : milestone, 113,
master - mean (111ms) : 107, 115
. : milestone, 111,
section CallTarget+Inlining+NGEN
This PR (5222) - mean (722ms) : 697, 747
. : milestone, 722,
master - mean (712ms) : 691, 732
. : milestone, 712,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5222) - mean (96ms) : 92, 99
. : milestone, 96,
master - mean (94ms) : 90, 97
. : milestone, 94,
section CallTarget+Inlining+NGEN
This PR (5222) - mean (675ms) : 654, 696
. : milestone, 675,
master - mean (664ms) : 644, 684
. : milestone, 664,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5222) - mean (187ms) : 184, 190
. : milestone, 187,
master - mean (187ms) : 183, 191
. : milestone, 187,
section CallTarget+Inlining+NGEN
This PR (5222) - mean (1,061ms) : 1027, 1095
. : milestone, 1061,
master - mean (1,055ms) : 1028, 1082
. : milestone, 1055,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5222) - mean (270ms) : 265, 276
. : milestone, 270,
master - mean (269ms) : 264, 275
. : milestone, 269,
section CallTarget+Inlining+NGEN
This PR (5222) - mean (1,055ms) : 1037, 1074
. : milestone, 1055,
master - mean (1,053ms) : 1028, 1077
. : milestone, 1053,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5222) - mean (258ms) : 252, 264
. : milestone, 258,
master - mean (258ms) : 254, 262
. : milestone, 258,
section CallTarget+Inlining+NGEN
This PR (5222) - mean (990ms) : 961, 1020
. : milestone, 990,
master - mean (988ms) : 962, 1014
. : milestone, 988,
|
67f4b1f
to
8eecfa6
Compare
Benchmarks Report 🐌Benchmarks for #5222 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.AppSecEncoderBenchmark - 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.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations
|
Benchmark | Base Allocated | Diff Allocated | Change | Change % |
---|---|---|---|---|
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net472 | 221.18 KB | 229.38 KB | 8.19 KB | 3.70% |
Benchmark | Base Allocated | Diff Allocated | Change | Change % |
---|---|---|---|---|
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 | 203.4 KB | 202.37 KB | -1.03 KB | -0.51% |
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 | 60.95 KB | 60.53 KB | -424 B | -0.70% |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StringConcatBenchmark |
net6.0 | 64.6μs | 973ns | 9.68μs | 0 | 0 | 0 | 43.44 KB |
master | StringConcatBenchmark |
netcoreapp3.1 | 52.7μs | 238ns | 857ns | 0 | 0 | 0 | 42.64 KB |
master | StringConcatBenchmark |
net472 | 37.5μs | 69.7ns | 261ns | 0 | 0 | 0 | 60.95 KB |
master | StringConcatAspectBenchmark |
net6.0 | 267μs | 1.24μs | 8.03μs | 0 | 0 | 0 | 204.12 KB |
master | StringConcatAspectBenchmark |
netcoreapp3.1 | 286μs | 1.5μs | 7.78μs | 0 | 0 | 0 | 203.4 KB |
master | StringConcatAspectBenchmark |
net472 | 239μs | 2.96μs | 28.1μs | 0 | 0 | 0 | 221.18 KB |
#5222 | StringConcatBenchmark |
net6.0 | 56.8μs | 588ns | 5.85μs | 0 | 0 | 0 | 43.44 KB |
#5222 | StringConcatBenchmark |
netcoreapp3.1 | 58.9μs | 804ns | 7.83μs | 0 | 0 | 0 | 42.64 KB |
#5222 | StringConcatBenchmark |
net472 | 37.3μs | 91.9ns | 331ns | 0 | 0 | 0 | 60.53 KB |
#5222 | StringConcatAspectBenchmark |
net6.0 | 278μs | 1.48μs | 9.34μs | 0 | 0 | 0 | 204.88 KB |
#5222 | StringConcatAspectBenchmark |
netcoreapp3.1 | 283μs | 1.15μs | 3.98μs | 0 | 0 | 0 | 202.37 KB |
#5222 | StringConcatAspectBenchmark |
net472 | 227μs | 1.22μs | 6.35μs | 0 | 0 | 0 | 229.38 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.5μs | 0.622ns | 2.33ns | 0.0225 | 0 | 0 | 1.58 KB |
master | EnrichedLog |
netcoreapp3.1 | 2.18μs | 5.22ns | 20.2ns | 0.0218 | 0 | 0 | 1.58 KB |
master | EnrichedLog |
net472 | 2.59μs | 1.6ns | 5.98ns | 0.239 | 0 | 0 | 1.51 KB |
#5222 | EnrichedLog |
net6.0 | 1.5μs | 0.882ns | 3.42ns | 0.0219 | 0 | 0 | 1.58 KB |
#5222 | EnrichedLog |
netcoreapp3.1 | 2.32μs | 1.48ns | 5.74ns | 0.0207 | 0 | 0 | 1.58 KB |
#5222 | EnrichedLog |
net472 | 2.57μs | 8.01ns | 31ns | 0.24 | 0 | 0 | 1.51 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 | 115μs | 125ns | 485ns | 0.0571 | 0 | 0 | 4.22 KB |
master | EnrichedLog |
netcoreapp3.1 | 117μs | 90.3ns | 350ns | 0.0579 | 0 | 0 | 4.22 KB |
master | EnrichedLog |
net472 | 147μs | 101ns | 376ns | 0.658 | 0.219 | 0 | 4.4 KB |
#5222 | EnrichedLog |
net6.0 | 113μs | 62.7ns | 217ns | 0.0564 | 0 | 0 | 4.22 KB |
#5222 | EnrichedLog |
netcoreapp3.1 | 117μs | 111ns | 428ns | 0.0584 | 0 | 0 | 4.22 KB |
#5222 | EnrichedLog |
net472 | 147μs | 65.9ns | 247ns | 0.661 | 0.22 | 0 | 4.4 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.91μs | 1.89ns | 7.33ns | 0.0291 | 0 | 0 | 2.14 KB |
master | EnrichedLog |
netcoreapp3.1 | 4.19μs | 2.03ns | 7.87ns | 0.0273 | 0 | 0 | 2.14 KB |
master | EnrichedLog |
net472 | 4.83μs | 1.91ns | 7.14ns | 0.309 | 0 | 0 | 1.95 KB |
#5222 | EnrichedLog |
net6.0 | 2.89μs | 1.36ns | 5.28ns | 0.0304 | 0 | 0 | 2.14 KB |
#5222 | EnrichedLog |
netcoreapp3.1 | 4.22μs | 1.11ns | 3.99ns | 0.0295 | 0 | 0 | 2.14 KB |
#5222 | EnrichedLog |
net472 | 4.76μs | 1.38ns | 5.17ns | 0.309 | 0 | 0 | 1.95 KB |
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | SendReceive |
net6.0 | 1.32μs | 2.17ns | 8.41ns | 0.0154 | 0 | 0 | 1.11 KB |
master | SendReceive |
netcoreapp3.1 | 1.72μs | 0.806ns | 3.02ns | 0.0145 | 0 | 0 | 1.11 KB |
master | SendReceive |
net472 | 2.19μs | 2.68ns | 10.4ns | 0.178 | 0 | 0 | 1.12 KB |
#5222 | SendReceive |
net6.0 | 1.42μs | 0.706ns | 2.64ns | 0.0157 | 0 | 0 | 1.11 KB |
#5222 | SendReceive |
netcoreapp3.1 | 1.76μs | 1.64ns | 6.13ns | 0.015 | 0 | 0 | 1.11 KB |
#5222 | SendReceive |
net472 | 1.99μs | 0.716ns | 2.77ns | 0.178 | 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.73μs | 0.705ns | 2.64ns | 0.0219 | 0 | 0 | 1.54 KB |
master | EnrichedLog |
netcoreapp3.1 | 3.88μs | 0.787ns | 3.05ns | 0.0213 | 0 | 0 | 1.58 KB |
master | EnrichedLog |
net472 | 4.32μs | 2.39ns | 9.26ns | 0.313 | 0 | 0 | 1.97 KB |
#5222 | EnrichedLog |
net6.0 | 2.69μs | 1.05ns | 3.94ns | 0.0216 | 0 | 0 | 1.54 KB |
#5222 | EnrichedLog |
netcoreapp3.1 | 3.91μs | 3.75ns | 14.5ns | 0.0212 | 0 | 0 | 1.58 KB |
#5222 | EnrichedLog |
net472 | 4.42μs | 3.03ns | 11.7ns | 0.312 | 0 | 0 | 1.97 KB |
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #5222
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0
1.121
533.29
597.63
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 | 1.121 | 533.29 | 597.63 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StartFinishSpan |
net6.0 | 590ns | 0.848ns | 3.28ns | 0.00749 | 0 | 0 | 544 B |
master | StartFinishSpan |
netcoreapp3.1 | 803ns | 1.17ns | 4.54ns | 0.00725 | 0 | 0 | 544 B |
master | StartFinishSpan |
net472 | 764ns | 2.17ns | 8.39ns | 0.0864 | 0 | 0 | 546 B |
master | StartFinishScope |
net6.0 | 533ns | 0.882ns | 3.41ns | 0.0093 | 0 | 0 | 664 B |
master | StartFinishScope |
netcoreapp3.1 | 827ns | 1.5ns | 5.82ns | 0.00866 | 0 | 0 | 664 B |
master | StartFinishScope |
net472 | 985ns | 2.43ns | 9.42ns | 0.0994 | 0 | 0 | 626 B |
#5222 | StartFinishSpan |
net6.0 | 534ns | 0.993ns | 3.84ns | 0.00753 | 0 | 0 | 544 B |
#5222 | StartFinishSpan |
netcoreapp3.1 | 744ns | 2.03ns | 7.85ns | 0.00737 | 0 | 0 | 544 B |
#5222 | StartFinishSpan |
net472 | 772ns | 1.86ns | 7.22ns | 0.0865 | 0 | 0 | 546 B |
#5222 | StartFinishScope |
net6.0 | 598ns | 1.01ns | 3.92ns | 0.00922 | 0 | 0 | 664 B |
#5222 | StartFinishScope |
netcoreapp3.1 | 852ns | 1.96ns | 7.61ns | 0.00897 | 0 | 0 | 664 B |
#5222 | StartFinishScope |
net472 | 996ns | 2.35ns | 9.09ns | 0.0993 | 0 | 0 | 626 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Slower ⚠️ Same allocations ✔️
Slower ⚠️ in #5222
Benchmark
diff/base
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0
1.183
601.81
712.11
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 | 1.183 | 601.81 | 712.11 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | RunOnMethodBegin |
net6.0 | 602ns | 1.45ns | 5.62ns | 0.00943 | 0 | 0 | 664 B |
master | RunOnMethodBegin |
netcoreapp3.1 | 910ns | 1.78ns | 6.89ns | 0.00909 | 0 | 0 | 664 B |
master | RunOnMethodBegin |
net472 | 1.08μs | 3.27ns | 12.7ns | 0.0994 | 0 | 0 | 626 B |
#5222 | RunOnMethodBegin |
net6.0 | 711ns | 0.873ns | 3.38ns | 0.00939 | 0 | 0 | 664 B |
#5222 | RunOnMethodBegin |
netcoreapp3.1 | 931ns | 2.15ns | 8.32ns | 0.009 | 0 | 0 | 664 B |
#5222 | RunOnMethodBegin |
net472 | 1.04μs | 2.04ns | 7.91ns | 0.0992 | 0 | 0 | 626 B |
8eecfa6
to
98e405c
Compare
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 (5222) (11.381M) : 0, 11381193
master (11.338M) : 0, 11337631
benchmarks/2.9.0 (11.425M) : 0, 11424847
section Automatic
This PR (5222) (7.849M) : 0, 7848740
master (7.650M) : 0, 7650211
benchmarks/2.9.0 (8.301M) : 0, 8300659
section Trace stats
This PR (5222) (8.115M) : 0, 8114860
master (7.922M) : 0, 7922321
section Manual
This PR (5222) (9.635M) : 0, 9634926
master (9.709M) : 0, 9709219
section Manual + Automatic
This PR (5222) (7.353M) : 0, 7353409
master (7.392M) : 0, 7392190
section Version Conflict
This PR (5222) (6.654M) : 0, 6654264
master (6.646M) : 0, 6646482
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5222) (9.419M) : 0, 9419046
master (9.487M) : 0, 9487398
benchmarks/2.9.0 (9.641M) : 0, 9641077
section Automatic
This PR (5222) (6.460M) : 0, 6459518
master (6.605M) : 0, 6605442
section Trace stats
This PR (5222) (6.966M) : 0, 6966224
master (6.935M) : 0, 6935276
section Manual
This PR (5222) (7.967M) : 0, 7966793
master (8.153M) : 0, 8152949
section Manual + Automatic
This PR (5222) (6.191M) : 0, 6191334
master (6.217M) : 0, 6217066
section Version Conflict
This PR (5222) (5.713M) : 0, 5713061
master (5.420M) : 0, 5419699
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5222) (10.056M) : 0, 10055759
master (9.981M) : 0, 9981056
benchmarks/2.9.0 (9.812M) : 0, 9811640
section Automatic
This PR (5222) (7.304M) : 0, 7304242
master (7.169M) : 0, 7169119
benchmarks/2.9.0 (7.410M) : 0, 7410467
section Trace stats
This PR (5222) (7.510M) : 0, 7510324
master (7.448M) : 0, 7447972
section Manual
This PR (5222) (9.111M) : 0, 9110912
master (8.860M) : 0, 8859810
section Manual + Automatic
This PR (5222) (7.021M) : 0, 7020799
master (6.977M) : 0, 6977328
section Version Conflict
This PR (5222) (6.255M) : 0, 6254825
master (6.377M) : 0, 6377341
gantt
title Throughput Linux x64 (ASM) (Total requests)
dateFormat X
axisFormat %s
section Baseline
master (7.452M) : 0, 7452193
benchmarks/2.9.0 (7.805M) : 0, 7805433
section No attack
master (1.888M) : 0, 1887644
benchmarks/2.9.0 (3.184M) : 0, 3183762
section Attack
master (1.505M) : 0, 1505387
benchmarks/2.9.0 (2.497M) : 0, 2497002
section Blocking
master (3.200M) : 0, 3199507
section IAST default
master (6.679M) : 0, 6678778
section IAST full
master (5.786M) : 0, 5786316
section Base vuln
master (0.982M) : 0, 981556
section IAST vuln
master (0.917M) : 0, 917350
|
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, would love to go over it in a meeting!
98e405c
to
680621a
Compare
Summary of changes
ExporterSettingsTests
Reason for change
We recently had an escalation where the customer had set three properties (via single-step instrumentation)
DD_TRACE_AGENT_URL=unix:///var/run/datadog/apm.socket
DD_AGENT_HOST=1.2.3.4
DD_DOGSTATSD_URL=unix:///var/run/datadog/dsd.socket
(not currently supported)The resulting configuration was:
/var/run/datadog/apm.socket
✔️127.0.01:8125
❌ Should be UDP:1.2.3.4:8125
The big problem in the the above is that the metrics is sending to localhost, which is incorrect.
Implementation details
The problem was that we had this code to determine the host to use for metrics:
However, when we're using UDS traces,
AgentUriInternal
isunix:///var/run/datadog/apm.socket
andAgentUriInternal.DnsSafeHost
returns""
so we fallback to"127.0.0.1"
. In the current scenario, we don't actually storeDD_AGENT_HOST
anywhere.The implementation changes that to explicitly record the metrics host in
ExporterSettings
when it's being configured. That means we can use the value ofDD_AGENT_HOST
if it's set and fixes the bug.Test coverage
Updated
ExporterSettingsTests
to assert that we are setting the hostname correctly inExporterSettings
I haven't tested thatTracerManagerFactory
uses it correctly when creating the dogstatsd client, as that's a bit of a PITA and I think the change is simple enough to not worry about (famous last words?)Other details
I'll do a PR to add support for
DD_DOGSTATSD_URL
stacked on this