fix(config): extract relevant env vars from pm2_env JSON before tracer init#8863
Conversation
Overall package sizeSelf size: 6.77 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.1 | 122.62 kB | 438.86 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8863 +/- ##
========================================
Coverage 98.34% 98.34%
========================================
Files 924 924
Lines 123056 123074 +18
Branches 10924 10481 -443
========================================
+ Hits 121016 121042 +26
+ Misses 2040 2032 -8 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 74f2de8 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-07-17 23:33:16 Comparing candidate commit 74f2de8 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2322 metrics, 36 unstable metrics.
|
![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: 41e1117416
ℹ️ 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".
|
The codecov check is buggy. It passes and fails between different runs even when code hasn't changed. |
tlhunter
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83370aa0cf
ℹ️ 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".
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
This will likely fix the overall problem for most users, while our guardrails will not know about DD_INJECT_FORCE and DD_INJECTION_ENABLED.
I am unsure if that is an issue. Next to that: we currently also access process.env using webpack, esbuild, and cypress and I guess they could also be called, while unlikely?
| /** | ||
| * In PM2 cluster mode, per-app env vars (DD_SERVICE, DD_ENV, etc.) are not | ||
| * passed as individual environment variables. Instead PM2 serializes the entire | ||
| * process config into a single `pm2_env` JSON string and passes only that to |
There was a problem hiding this comment.
😢
Should we potentially open a PR against PM2 to stop this and instead just sets the envs when starting the child accordingly?
|
I'm converting this into a draft while we wait on PM2 to reply to the PR. |
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
Looking at this again: I believe we have to land something like it because the upstream fix will likely take long.
To fix it, I think we should expand the envs inside of init.js before the guardrails and just unconditially accept the values and override any other ones.
83370aa to
4107ce2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95a38a180a
ℹ️ 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".
| if (typeof pm2EnvStr === 'string') { | ||
| try { | ||
| var pm2Config = JSON.parse(pm2EnvStr) | ||
| var pm2Keys = Object.keys(pm2Config) |
There was a problem hiding this comment.
It would be nicer to ignore the Object.keys warning locally instead of globally, while it likely does not matter much
There was a problem hiding this comment.
If the linter complains that Object.keys() doesn't exist in Node.js v0.8.0, but it actually does (I confirmed on v0.8.6), then wouldn't it be more correct to globally configure the linter to always allow Object.keys() everywhere (specifically when evaluating from a v0.8.0 perspective) instead of in a single location?
Adding a single eslint ignore line feel wrong since it's superfluous at best and at worst masks (admittedly unlikely) issues where Object.keys() becomes deprecated in the future.
fac5c7d to
d926665
Compare
| }, | ||
| }, | ||
| { | ||
| name: 'dd-trace/defaults/v0.8-oldest/init', |
There was a problem hiding this comment.
That... is not what I had in mind 😅
I meant a local eslint disable next line comment to deactivate a single rule.
1ec3cd9 to
653441a
Compare
| process.env[k] = String(v) | ||
| } | ||
| } | ||
| } catch (e) {} |
There was a problem hiding this comment.
Should this log an error? It's so early in the bootstrapping process that I don't think we can do so cleanly.
I could cache the error and then log it later once the logger is ready. There is some precedent for that.
There was a problem hiding this comment.
I think it is fine as a best effort approach here. Throwing would be very confusing anyway (none of the operations should be possible to throw)
653441a to
74f2de8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74f2de857b
ℹ️ 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".
| var k = pm2Keys[i] | ||
| var v = pm2Config[k] | ||
| if (v != null) { | ||
| process.env[k] = String(v) |
There was a problem hiding this comment.
When pm2_env contains a relative file-backed config such as DD_SPAN_SAMPLING_RULES_FILE or the AppSec blocked-template paths and pm2_env.pm_cwd differs from the PM2 daemon cwd, this assignment makes dd-trace consume the value during require('.').init() before PM2's wrapper later runs process.chdir(pm2_env.pm_cwd || ...) (PM2 ProcessContainer). The config readFilePath transformer reads fs.readFileSync(raw) relative to the current cwd, so the tracer drops or misreads a valid per-app PM2 config that would work once PM2 starts the app.
Useful? React with 👍 / 👎.
| var k = pm2Keys[i] | ||
| var v = pm2Config[k] | ||
| if (v != null) { | ||
| process.env[k] = String(v) |
There was a problem hiding this comment.
When SSI puts DD_INJECTION_ENABLED in the PM2 app env and that app also has its own dd-trace, copying the PM2 blob here makes the guardrails run the app-dir clobber check immediately, but at this point process.argv[1] is still PM2's wrapper while the real script is only in pm2Config.pm_exec_path. The guard in packages/dd-trace/src/guardrails/index.js resolves dd-trace from process.argv[1], so PM2 cluster workers won't detect the app-local tracer and will initialize the injected tracer anyway.
Useful? React with 👍 / 👎.
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
No matter that the AI is complaining: this is improving our current status quo a lot, so we should include this as is in my opinion. Thank you for the fix!
2 participants
What does this PR do?
This allows the tracer to run in pm2 cluster mode. Notably pm2 passes in a JSON-encoded env var that is merges into
process.envafter--requireruns, so this change extracts any tracer-relevant entries first.Motivation
This is from a user-reported error.