Repository navigation
SVLS-9418: check that the dogstatsd pipe is actually bound - #8874
apiarian-datadog wants to merge 9 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77fa65884f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8874) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-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 highlighted 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). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8874) - mean (72ms) : 69, 74
master - mean (71ms) : 69, 73
section Bailout
This PR (8874) - mean (79ms) : 75, 83
master - mean (75ms) : 73, 77
section CallTarget+Inlining+NGEN
This PR (8874) - mean (1,093ms) : 1038, 1147
master - mean (1,096ms) : 1034, 1159
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8874) - mean (114ms) : 108, 120
master - mean (110ms) : 107, 113
section Bailout
This PR (8874) - mean (114ms) : 108, 120
master - mean (111ms) : 108, 114
section CallTarget+Inlining+NGEN
This PR (8874) - mean (784ms) : 760, 808
master - mean (783ms) : 755, 810
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8874) - mean (102ms) : 96, 109
master - mean (101ms) : 95, 107
section Bailout
This PR (8874) - mean (99ms) : 97, 102
master - mean (99ms) : 97, 100
section CallTarget+Inlining+NGEN
This PR (8874) - mean (952ms) : 913, 992
master - mean (951ms) : 910, 991
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8874) - mean (98ms) : 92, 105
master - mean (97ms) : 93, 100
section Bailout
This PR (8874) - mean (102ms) : 95, 109
master - mean (102ms) : 97, 107
section CallTarget+Inlining+NGEN
This PR (8874) - mean (821ms) : 785, 856
master - mean (820ms) : 783, 856
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8874) - mean (213ms) : 208, 217
master - mean (209ms) : 204, 215
section Bailout
This PR (8874) - mean (216ms) : 211, 220
master - mean (212ms) : 207, 216
section CallTarget+Inlining+NGEN
This PR (8874) - mean (1,266ms) : 1227, 1305
master - mean (1,248ms) : 1208, 1288
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8874) - mean (303ms) : 296, 311
master - mean (297ms) : 289, 304
section Bailout
This PR (8874) - mean (305ms) : 298, 311
master - mean (297ms) : 292, 301
section CallTarget+Inlining+NGEN
This PR (8874) - mean (1,006ms) : 982, 1030
master - mean (997ms) : 977, 1017
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8874) - mean (297ms) : 291, 304
master - mean (289ms) : 282, 295
section Bailout
This PR (8874) - mean (299ms) : 291, 308
master - mean (290ms) : 285, 295
section CallTarget+Inlining+NGEN
This PR (8874) - mean (1,200ms) : 1156, 1244
master - mean (1,191ms) : 1140, 1243
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8874) - mean (296ms) : 288, 304
master - mean (293ms) : 287, 300
section Bailout
This PR (8874) - mean (295ms) : 291, 300
master - mean (293ms) : 286, 300
section CallTarget+Inlining+NGEN
This PR (8874) - mean (1,089ms) : 994, 1184
master - mean (1,073ms) : 999, 1147
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-07-24 17:38:04 Comparing candidate commit d51fa95 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 70 metrics, 0 unstable metrics, 60 known flaky benchmarks, 66 flaky benchmarks without significant changes.
|
|
validated with https://github.com/DataDog/serverless-examples/pull/205 . works in isolation and in combination. |
apiarian-datadog
added
os:windows
area:azure-site-extension
There was a problem hiding this comment.
Pull request overview
Updates the Azure App Service child-process management logic (AgentProcessManager) to treat DogStatsD as healthy only when its named pipe is actually bound (for pipe-only configurations), helping recovery from overlapped restarts where a “running-but-broken” dogstatsd.exe can persist.
Changes:
- Add a pipe-bound–aware (and debounced) health check path for pipe-only DogStatsD scenarios.
- Add unit tests for the new health evaluation/debounce behavior and for
DD_DOGSTATSD_PORT=0+ pipe-name config resolving to Named Pipe. - Add a manual verification guide for reproducing/validating recovery in Azure App Service.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tracer/src/Datadog.Trace/AgentProcessManager.cs | Adds pipe-only “require bound pipe” health semantics, debounced health checks, and stale-process kill-before-restart logic. |
| tracer/test/Datadog.Trace.Tests/Agent/AgentProcessManagerTests.cs | Unit tests for new health evaluation and debounce arithmetic/threshold behavior. |
| tracer/test/Datadog.Trace.Tests/Configuration/ExporterSettingsTests.cs | Ensures DD_DOGSTATSD_PORT=0 + pipe name resolves to Named Pipe transport (enabling pipe-only behavior). |
| docs/development/SLES-2913-AzureAppServiceDogStatsdPipeRecovery.md | Manual validation steps for Azure App Service recovery behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2043056 to
79d561f
Compare
apiarian-datadog
changed the title
fc34b90 to
aaccf99
Compare
| await Task.Delay((int)delay).ConfigureAwait(false); | ||
|
|
||
| if (metadata.ProcessIsHealthy()) | ||
| if (metadata.IsHealthyDebounced()) |
There was a problem hiding this comment.
Could you help me understand the timing here?
If I understand correctly the intention here is basically answering whether this AppDomain where this is running should start the DogStatsD process.
If DogStatsD is running but its pipe is not yet bound line 209 will return true
Waits 50ms
Checks here and if it is not bound again, it returns false
So I think this is saying that we only will ever need 50ms to bind a pipe otherwise we will start multiple DogStatsD processes across multiple AppDomains?
There was a problem hiding this comment.
the check here is in case some other app domain owns a dogstatsd process. if that domain owns it, it's either a peer that is free to do so or a previous instance that is about to shut down anyway.
if it is a healthy peer, we give it a little over 3 seconds (in steps of 50ms, 87ms, 153ms, 267ms, etc) to prove that it's still healthy. if so, the process state is kept as healthy and we move on.
if it is a dogstatsd that gets cleared out in those 3 seconds, we assume that we are recovering from some kind of previous shutdown and the dogstatsd we saw was a blip and we should be starting a new one.
There was a problem hiding this comment.
(actually, it's not 3 seconds, it's more like 1.8 since the last delay never hits. off by one here, may tweak the looping condition later.)
There was a problem hiding this comment.
I think we may be discussing different cases?
My thought process here is that we have a multi AppDomain startup race with that 50ms pipe check now.
If AppDomain A's DogStatsD process is running but takes longer than 50ms to bind its pipe AppDomain B can classify it as unhealthy and start a second process. One of those processes will likely get the pipe ownership.
Is this correct? If so is this a known limitation
I guess if so, there should follow up with a fix, but I'll approve to unblock the customer fix you are trying to get out. I would recommend a second review as this
There was a problem hiding this comment.
ah, ok, i think i understand now. there is in fact a race here, but it's a pretty low-risk one, i think.
there was no real cross-runner coordination, it was always based on os observations (one for checking the process by name and the other for checking if the named pipe is bound). our change prefers the pipe check over the process check, so it is possible for two process managers to both see that nothing is bound to the pipe and then try to launch the process.
however, this manager only runs in windows azure app service site extensions, and this new behavior only activates when we expect that the metrics transport is the named pipe. in all other scenarios we go back to the original pipe-bound || program-running behavior.
in azure app service extensions each app gets its own unique named pipe at install time (some day soon it will be a unique named pipe at runtime, too, i think). so there's no cross-app contention here, only limited within a single app.
and as you say, the change is self-correcting since only one of those dogstatsd processes will be able to bind to the unique named pipe. the others will fail to bind, and stop mattering since they don't bind to the pipe. we've got a change in the dogstatsd agent that will actually cause them to shut down when it cannot bind to a pipe, but that's a month and a half out at least in the agent release cycle.
we could explore some kind of system-wide lock on the dogstatsd, but that seems like too much extra complexity.
There was a problem hiding this comment.
and this new behavior only activates when we expect that the metrics transport is the named pipe
But the metrics transport is always named pipes in the AAS extension? 🤔
we've got a change in the dogstatsd agent that will actually cause them to shut down when it cannot bind to a pipe, but that's a month and a half out at least in the agent release cycle.
I mean, my inclination is to drop all the complexity here and just wait for that fix tbh, given (AFAICT) the only impact is leaking zombie dogstatsd instances, which seems like a low impact? 🤷♂️
There was a problem hiding this comment.
unfortunately the zombie dogstatsd instances currently result in memory leaks in csharp: DataDog/dogstatsd-csharp-client#231 and it's going to take a month and a half for the agent changes to be released. we were hoping to get this patch out sooner so that we can get the extension in a better state.
bouwkast
left a comment
bouwkast
left a comment
There was a problem hiding this comment.
I think there is a potential startup race condition that this creates where multiple DogStatsD processes can be started.
Given that this seems better than just having the process be broken maybe this is the more desirable state?
Regardless I would recommend a second approval before merging as this changes some long standing process management behavior.
| await Task.Delay((int)delay).ConfigureAwait(false); | ||
|
|
||
| if (metadata.ProcessIsHealthy()) | ||
| if (metadata.IsHealthyDebounced()) |
There was a problem hiding this comment.
I think we may be discussing different cases?
My thought process here is that we have a multi AppDomain startup race with that 50ms pipe check now.
If AppDomain A's DogStatsD process is running but takes longer than 50ms to bind its pipe AppDomain B can classify it as unhealthy and start a second process. One of those processes will likely get the pipe ownership.
Is this correct? If so is this a known limitation
I guess if so, there should follow up with a fix, but I'll approve to unblock the customer fix you are trying to get out. I would recommend a second review as this
andrewlock
left a comment
andrewlock
left a comment
There was a problem hiding this comment.
Tbh, I'm not sure it's worth doing this work, adding a bunch of circuitous retrying when the root cause is being fixed upstream 🤷♂️ Either way, we can simplify the logic, because the AgentProcessManager is only called with named pipes AFAIK?
| // A pipe-only dogstatsd (named pipe transport selected) can stay alive but fully | ||
| // non-functional after failing to bind its pipe, so process liveness is not a |
There was a problem hiding this comment.
This seems like a bug in dogstatsd? 🤔 Given we ship the tracer and dogstatsd together, can we make the fix in dogstatsd instead, which would remove all this complexity and heuristics?
There was a problem hiding this comment.
we are doing that, but it's going to take a month and a half to get that change released. this lets us cover the issue from both sides (there's a third side, actually: DataDog/dogstatsd-csharp-client#231 )
| // non-functional after failing to bind its pipe, so process liveness is not a | ||
| // reliable health signal. Resolve the transport from configuration rather than | ||
| // re-deriving it, so this matches whatever the metrics client will actually use. | ||
| var exporterSettings = new ExporterSettings(GlobalConfigurationSource.Instance, File.Exists, NullConfigurationTelemetry.Instance); |
There was a problem hiding this comment.
This is a whole bunch of duplicated work to be doing in the startup path - is it really necessary? Because AFAIK, AgentProcessManager is only used in the AAS extension, and we only support named pipes there? Could we not simplify a whole lot of this logic?
There was a problem hiding this comment.
i suppose we could add an assertion that we're running with a named pipe dogstatsd and then simplify the rest of the logic.
There was a problem hiding this comment.
out of the box the extension is configured to run with named pipes and disable the udp port. but it seems like there's no guarantee that a customer won't override it. so i think we do still need to err on the side of being ready for named pipes not to be required.
There was a problem hiding this comment.
I would strongly prefer that tbh, seeing as far as I know, only named pipes is supported
There was a problem hiding this comment.
wait, prefer which? not allow customers to override the configuration at all?
There was a problem hiding this comment.
Just don't support it - if customers override the config they'll be broken anyway, so we're doing a bunch of extra work in the startup hotpath here to try to work around a scenario that's already broken 🤔
Tbh, my preference is probably just to wait for the dogstatsd fix because it seems like that avoids all the extra work here?
There was a problem hiding this comment.
not supporting it would be a breaking change, wouldn't it? and it seems possible that a customer would disable named pipes and, say, manually manage unique ports for their colocated dogstatsd instances.
and a wait for the dogstatsd puts us back a month waiting for the agent release cycle. maybe that's okay given the csharp client updates, but we'd need to check with support.
| await Task.Delay((int)delay).ConfigureAwait(false); | ||
|
|
||
| if (metadata.ProcessIsHealthy()) | ||
| if (metadata.IsHealthyDebounced()) |
There was a problem hiding this comment.
and this new behavior only activates when we expect that the metrics transport is the named pipe
But the metrics transport is always named pipes in the AAS extension? 🤔
we've got a change in the dogstatsd agent that will actually cause them to shut down when it cannot bind to a pipe, but that's a month and a half out at least in the agent release cycle.
I mean, my inclination is to drop all the complexity here and just wait for that fix tbh, given (AFAICT) the only impact is leaking zombie dogstatsd instances, which seems like a low impact? 🤷♂️
…ool (#231) `dogstatsd` in Azure App Services with the Windows Extension sometimes fail to correctly get a named pipe. This can happen when the app is hot-reloaded and the previous incarnation of `dogstatsd` hasn't released the named pipe when the new `dogstatsd` is starting up. This causes the named pipe writes from the dogstatsd client to fail (permanently, until the app is restarted again). These failures cause a steady growth of threads as the telemetry timer spawns one every 10 seconds to try to write client telemetry, and the 2 second timeout for each of the 11 stats that the telemetry wants to send compounds to 22 seconds of work. Forever. This change turns the telemetry timer into one that reschedules itself after every round, instead of spawning one every 10 seconds. It also adds a cooldown for sending telemetry, so that if there's a failure to send, we back off for a bit before trying to send any more. This is one of a few different patches to address this issue. We also have DataDog/dd-trace-dotnet#8874 for the app service extension process manager and DataDog/datadog-agent#53328 for dogstatsd.
aaccf99 to
b2f2881
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tracer/test/Datadog.Trace.Tests/Agent/AgentProcessManagerTests.cs:7
AgentProcessManageris declared in theDatadog.Tracenamespace, but this test file doesn't import it or fully-qualify the type, so it won't compile unless a project-wide global using exists (none found intracer/test/Datadog.Trace.Tests/GlobalUsings.cs). Addusing Datadog.Trace;(or fully-qualifyDatadog.Trace.AgentProcessManager).
using FluentAssertions;
using Xunit;
|
looks like this isn't so urgent that we cannot wait for the dogstatsd change to be released. going to close this pr for now. |
Summary of changes
Change the dogstatsd process management to require that the pipe is bound and the process is running.
Reason for change
In some Azure App Service restarts, the old app and its dogstatsd server is still running, with the dogstatsd holding on to the named pipe. The new
dogstatsdfails to bind to the pipe and just stalls, doing nothing (datadog-agent patch, but we'd like to have coverage here in the tracer in addition to thedogstatsdpatch).Implementation details
Add a debounced process and pipe check.
Test coverage
Unit test. Will also test manually in an actual azure app service environment.