Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,9 @@ export default [
// The following are false positives that are supported in Node.js 0.8.0
ignores: [
'JSON',
'JSON.parse',
'JSON.stringify',
'Object.keys',
'parseInt',
'String',
],
Expand All @@ -700,6 +702,7 @@ export default [
ignores: [
'array-prototype-indexof',
'json',
'object-keys',
],
}],
'no-var': 'off', // Only supported in Node.js 6+
Expand Down
18 changes: 18 additions & 0 deletions init.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,23 @@
'use strict'

// In PM2 cluster mode, per-app env vars arrive as a `pm2_env`
// JSON string after --require has already run so we extract them
// manually if present.
var pm2EnvStr = process.env.pm2_env
if (typeof pm2EnvStr === 'string') {
try {
var pm2Config = JSON.parse(pm2EnvStr)
var pm2Keys = Object.keys(pm2Config)

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.

It would be nicer to ignore the Object.keys warning locally instead of globally, while it likely does not matter much

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

for (var i = 0; i < pm2Keys.length; i++) {
var k = pm2Keys[i]
var v = pm2Config[k]
if (v != null) {
process.env[k] = String(v)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Handle PM2 file configs relative to the app cwd

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 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Resolve the PM2 app before the clobber guard

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 👍 / 👎.

}
}
} catch (e) {}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

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 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)

}

var guard = require('./packages/dd-trace/src/guardrails')

module.exports = guard(function () {
Expand Down
50 changes: 50 additions & 0 deletions integration-tests/init.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,56 @@ describe('init.js', () => {

testInjectionScenarios('require', 'init.js', false)
testRuntimeVersionChecks('require', 'init.js')

describe('PM2 cluster mode', () => {
useEnv({ NODE_OPTIONS: '--require dd-trace/init' })

afterEach(() => {
delete process.env.pm2_env
})

function checkEnv (expectedValues) {
return testFile('init/pm2-env.js', out => {
const env = JSON.parse(out.trim())
for (const [key, value] of Object.entries(expectedValues)) {
assert.strictEqual(env[key], value, `expected env.${key} to equal ${value}`)
}
}, [], '')
}

it('applies all env vars from pm2_env blob to process.env', () => {
process.env.pm2_env = JSON.stringify({ DD_SERVICE: 'pm2-svc', DD_ENV: 'pm2-env', MY_APP_VAR: 'hello' })
return checkEnv({ DD_SERVICE: 'pm2-svc', DD_ENV: 'pm2-env', MY_APP_VAR: 'hello' })
})

it('coerces non-string values to strings', () => {
process.env.pm2_env = JSON.stringify({ DD_TRACE_SAMPLE_RATE: 0.5 })
return checkEnv({ DD_TRACE_SAMPLE_RATE: '0.5' })
})

it('skips keys with null values', () => {
process.env.pm2_env = JSON.stringify({ DD_SERVICE: null })
return checkEnv({ DD_SERVICE: undefined })
})

it('does not crash on malformed pm2_env JSON', () => {
process.env.pm2_env = 'not-valid-json'
return checkEnv({ DD_SERVICE: undefined })
})

it('does nothing when pm2_env is absent', () => {
return checkEnv({ DD_SERVICE: undefined })
})

describe('when env vars are already set', () => {
useEnv({ DD_SERVICE: 'original-service', MY_APP_VAR: 'original' })

it('overwrites existing env vars with pm2_env values', () => {
process.env.pm2_env = JSON.stringify({ DD_SERVICE: 'pm2-service', MY_APP_VAR: 'pm2-value' })
return checkEnv({ DD_SERVICE: 'pm2-service', MY_APP_VAR: 'pm2-value' })
})
})
})
})

// ESM is not supportable prior to Node.js 14.13.1 on the 14.x line,
Expand Down
10 changes: 10 additions & 0 deletions integration-tests/init/pm2-env.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
'use strict'

// eslint-disable-next-line no-console
console.log(JSON.stringify({
DD_SERVICE: process.env.DD_SERVICE,
DD_ENV: process.env.DD_ENV,
DD_TRACE_SAMPLE_RATE: process.env.DD_TRACE_SAMPLE_RATE,
MY_APP_VAR: process.env.MY_APP_VAR,
}))
process.exit()
Loading