Skip to content

[SVLS-8249] Generates UUID and passes env vars for Node#46

Open
Chronobreak wants to merge 17 commits into
mainfrom
ddk/svls-compat-named-pipes
Open

[SVLS-8249] Generates UUID and passes env vars for Node#46
Chronobreak wants to merge 17 commits into
mainfrom
ddk/svls-compat-named-pipes

Conversation

@Chronobreak

@Chronobreak Chronobreak commented Apr 24, 2026

Copy link
Copy Markdown

What does this PR do?

  • Generates UUID for Windows named pipes
  • Passes DD_APM_WINDOWS_PIPE_NAME for the binary
  • Passses DD_TRACE_AGENT_URL for the Node tracer

Motivation

https://datadoghq.atlassian.net/browse/SVLS-8249

Additional Notes

Describe how to test/QA your changes

This is running with the sm.suffix:ddk-js46

  • logs
  • traces
  • metrics - switch to metrics tabs from traces

Corresponding changes to come in serverless-compat-self-monitoring

Chronobreak and others added 3 commits April 21, 2026 12:12
Avoid pipe collisions when multiple Azure Functions share a namespace
by generating a fresh pipe name per process in start() before spawning
the compat binary.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the graceful-skip guard with a thrown error so any future
format change that violates the Windows named-pipe length limit fails
loudly instead of silently reverting to user-supplied values. Also
clarifies the call site lives inside start()'s existing try/catch so
the error is caught and spawn is aborted.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@Chronobreak Chronobreak requested a review from a team as a code owner April 24, 2026 18:25
@Chronobreak Chronobreak requested review from apiarian-datadog and removed request for a team April 24, 2026 18:25
@Chronobreak Chronobreak marked this pull request as draft April 24, 2026 18:25
@Chronobreak Chronobreak removed the request for review from apiarian-datadog April 24, 2026 18:25
Comment thread src/index.ts Outdated
Comment on lines +154 to +155
!process.env.DD_APM_WINDOWS_PIPE_NAME &&
!process.env.DD_TRACE_AGENT_URL

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current behavior is that it'll only generate the UUID only if both are not set (i.e. respecting user's env vars)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However I was curious about the default behavior when one-but-not-the-other is set and I think it results in a situation where traces are dropped

  • Only DD_APM_WINDOWS_PIPE_NAME=foo set → the spawned binary listens on \.\pipe\foo, but dd-trace-js falls back to its default agent URL (HTTP localhost:8126) and never reaches the pipe. Traces drop.
  • Only DD_TRACE_AGENT_URL=unix://./pipe/foo set → dd-trace-js sends to \.\pipe\foo, but the binary picks its own default pipe name and listens elsewhere. Traces drop.

@Chronobreak Chronobreak marked this pull request as ready for review May 18, 2026 18:04
@Chronobreak Chronobreak requested a review from Lewis-E May 19, 2026 19:52
Comment thread src/index.ts
Comment on lines +67 to +78
if (pipeName) {
process.env.DD_TRACE_AGENT_URL = `unix://./pipe/${pipeName}`;
return;
}

if (traceAgentUrl) {
const match = traceAgentUrl.match(/\/pipe\/(.+?)\/?$/);
if (match) {
process.env.DD_APM_WINDOWS_PIPE_NAME = match[1];
}
return;
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one is set but not the other, we make sure they're both set

Chronobreak and others added 6 commits May 21, 2026 17:11
dd-trace-js's agent exporter wraps DD_TRACE_AGENT_URL in `new URL()`, and its
`parseUrl` skips the unix-pathname rewrite for URL objects. For
`unix://./pipe/<n>` that yields `socketPath=/pipe/<n>` — not a valid Windows
pipe path. Emit `unix:\\.\pipe\<n>` instead so pathname already contains the
canonical form.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Without DD_DOGSTATSD_WINDOWS_PIPE_NAME, the serverless-compat Rust
binary binds UDP 8125 and panics with AddrInUse when co-located with
another function on the same Windows plan (EP2/P1V3). Mirror the APM
auto-gen path for the dogstatsd pair (DD_DOGSTATSD_WINDOWS_PIPE_NAME
server-side, DD_DOGSTATSD_PIPE_NAME client-side).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…trace span-stats

dd-trace's span-stats writer (exporters/span-stats/writer.js) builds request
options with `protocol: url.protocol` and no socketPath. With the backslash
form `unix:\\.\pipe\<name>`, url.protocol is `'unix:'`, which Node 22's
ClientRequest rejects with ERR_INVALID_PROTOCOL, crashing the worker.

Monkey-patch http(s).request/get to strip `protocol: 'unix:'` and attach
socketPath from DD_APM_WINDOWS_PIPE_NAME before forwarding to the original.
This preserves span-stats functionality without modifying dd-trace.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment thread src/index.ts
if (!pipeName) return;
const socketPath = `\\\\.\\pipe\\${pipeName}`;

for (const module of [require('http'), require('https')]) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this could impact any other features outside of the span export you're targeting? patching all of HTTP is pretty broad. Can this impact the user's app as well as the tracer?

I think talking to the tracer team about the long term fix should at least be a ticket.

@Lewis-E Lewis-E Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only fires for requests carrying protocol: 'unix:'

This could easily be an app calling redis or similar. We need to scope this to specifically http as used in the one module of the tracer we want to be patching. Y'gotta sit on Claude here; a comment is not a fix. Try making it explain all the possible risks across diverse customers with complicated apps before asking it to fix the issue as well as explaining the need for the absolute minimum blast radius.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much - took a look again and tightened to scope further to only apply to span-stats writer traffic! I'll log the additional ticket for the node tracer team and circle back here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test this with an app making http calls, so we can make sure there are no side effects.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much - tested here, getting outbound http calls

Screenshot 2026-06-17 at 1 28 06 PM

Comment thread src/index.ts Outdated
Comment thread src/index.ts
Comment thread src/index.ts Outdated

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the fix should actually be in dd-trace. I am opening a PR there. When it is fixed, we can just update dd-trace-js and that should resolve the issue, if I am not mistaken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants