Repository navigation
Conversation
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
When the StatsD connection ran out of connect retries it logged at debug and stopped for the life of the JVM, so default customer logs showed no sign that tracer metrics were never sent. Warn instead, with the StatsD address, the number of attempts, and the last error.
When the DogStatsD pipe or port was missing at startup, the StatsD connection retried 20 times, 10 seconds apart, then gave up for the life of the JVM, so a DogStatsD that came up later was never used. In Azure App Service this happens when the tracer starts before DogStatsD listens. - Retry forever, doubling the delay from 10 seconds to a cap of 60 seconds. Drop the jitter on retries so the delay stays within the cap. - Warn on the first failure, and again at most every 5 minutes once the delay reaches the cap, so a lasting failure stays visible without debug logs. Log other attempts at debug, and log once at info when a connection succeeds after failures.
4767300 to
1de72a7
Compare
The StatsD client opens its named pipe once and never reopens it. After the DogStatsD it was writing to went away, every write failed for the life of the JVM, even once a new DogStatsD served the same pipe name. In an Azure App Service overlapping restart, tracer metrics were still missing 50 minutes after the old worker's DogStatsD exited. - Track write errors in handle(), for named pipes only. The UDP and Unix datagram clients send to their address on every write, and the Unix stream client reconnects itself, so recreating them would only drop queued metrics, for example while an overloaded agent fills the socket buffer. - Errors more than 60 seconds apart start a new streak; a streak of at least 3 errors spanning at least 30 seconds triggers a recreate. The 60 second gap is longer than the 30 second tracer health metrics flush, so a dead pipe keeps one streak going, while brief bursts and isolated timeouts under load do not trigger it. - Recreate on the task scheduler, not the client's own thread: close the client and reconnect through the existing connect path, which retries with backoff if the pipe is still missing. - Warn on recreate, rate limited to once every 5 minutes.
NamedPipeSocketFactory kept one NamedPipeSocket per pipe and returned it to every OkHttp connection (#3493). NamedPipeSocket ignores read timeouts, so when OkHttp checks a pooled connection's health it blocks on an unbounded pipe read, and every other agent request (traces, stats, remote config, telemetry) waits behind it on the shared handle. On Azure Functions with DD_TRACE_PIPE_NAME set, the tracer queued spans but never sent or failed a trace upload until the JVM restarted. Open a new pipe handle for each socket, as the .NET tracer does for each request. #3493 shared the handle because a named pipe was thought to allow only one client at a time; that change was untested on Windows, and the .NET tracer's per-request pipe connections show the agent's pipe server accepts several clients. The agent's pipe servers create their next pipe instance only after a client connects, so a request that opens the pipe in between fails with "All pipe instances are busy". Like .NET's NamedPipeClientStream.Connect, retry opening the pipe for up to 500 ms, the .NET tracer's default DD_TRACE_PIPE_TIMEOUT_MS. Java reports a busy pipe, a missing pipe, and other open failures with the same exception, so all of them are retried.
NamedPipeSocket can't time out a read, so when OkHttp checks a pooled connection's health before reusing it, the check can block forever on the pipe. Give named-pipe clients a connection pool that keeps no idle connections, so each request opens and closes its own pipe connection, as the .NET tracer does. For named pipes this replaces the maxRunningRequests connection pool; the dispatcher limits are unchanged.
When a DogStatsD named pipe was set and no port was configured, DDAgentStatsDConnection left the port null, and building the client threw NullPointerException on .port(port). Tracer health metrics and JMXFetch JVM metrics were never sent over a pipe. Use port 0 when a named pipe is set and no port is configured. The client connects to the pipe and ignores the port; port 0 also selects the same packet size and socket settings the tracer uses for Unix domain sockets.
1de72a7 to
2254f7d
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
The pipe-recovery trigger also counts generic StatsD reporting exceptions, so recurring non-transport errors can repeatedly tear down a healthy shared connection. Recovery should be driven by pipe write failures.
🤖 Bits Code Review · Commit 2254f7d
| // only the named pipe client never reconnects; the UDP and Unix socket clients send to their | ||
| // address again or reconnect themselves, so recreating them would only drop queued metrics | ||
| if (null != namedPipe | ||
| && writeFailureStreak.recordError(NANOSECONDS.toMillis(System.nanoTime()))) { |
There was a problem hiding this comment.
Restrict pipe recreation to transport failures
For named-pipe connections, this generic error hook also receives externally reported exceptions through DDAgentStatsDClient.error(), including errors forwarded by AgentStatsdReporter.ErrorHandler. Three non-transport errors spanning 30 seconds, with gaps below 60 seconds, therefore trigger recreation of a healthy shared client. This unnecessarily interrupts unrelated metrics without fixing the reporting failure. Keep externally reported errors on a logging/counting path and advance the recovery streak only for pipe write failures; separating those paths requires changes beyond this diff hunk.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
What Does This Do
This fixes the tracer's connections to the Datadog Agent over Windows named pipes. Currently, Java apps on Azure Functions will fight over a shared UDP connection, leading to missing traces and metrics (SVLS-8247). Additionally, we're covering edge cases around Named Pipe connections, as they behave differently than UDP. During Azure Functions restarts, the underlying infrastructure (IIS) can have overlapping instances, which can cause incorrect connections and long failure states.
Traces, stats, remote config, and telemetry (HTTP over the trace agent pipe), commits 4 and 5:
NamedPipeSocketFactoryopens a new pipe handle for every socket instead of returning one sharedNamedPipeSocketper pipe. Like .NET'sNamedPipeClientStream.Connect, it retries opening a busy or missing pipe for up to 500 ms, the .NET defaultDD_TRACE_PIPE_TIMEOUT_MS.maxRunningRequestsconnection pool, while the dispatcher limits stay as they are.Tracer metrics (StatsD), commits 1 to 3 and 6:
DDAgentStatsDConnectionuses port 0 when the config sets a named pipe and no port. Before, it threwNullPointerException.Motivation
Customers on two Azure products hit these bugs.
Azure Functions with named pipes (SVLS-8247): Java tracing over a named pipe for Functions on a shared Windows plan need per-function pipes.
Azure App Service overlapping restarts (SVLS-9057, escalation SLES-2826): during an overlapped IIS recycle, the old and new worker share the DogStatsD pipe
Additional Notes
NamedPipeSocketFactoryhas a package-privateFileconstructor so that its test can use a regular file as the pipe on any OS.Tests:
NamedPipeSocketFactoryTestcovers a new socket per connectionDDAgentStatsDConnectionTestcovers the retry delay, the write-failure streak, and the named-pipe port#12899 (App Service agent supervision and logging) is stacked on this branch. datadog-serverless-compat-java#43 gives each Windows Azure Function its own named pipes, and it requires the first dd-trace-java release with this PR.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: SVLS-8247 [SVLS-9057]