-
Notifications
You must be signed in to change notification settings - Fork 403
fix(config): extract relevant env vars from pm2_env JSON before tracer init #8863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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) | ||
| for (var i = 0; i < pm2Keys.length; i++) { | ||
| var k = pm2Keys[i] | ||
| var v = pm2Config[k] | ||
| if (v != null) { | ||
| process.env[k] = String(v) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When SSI puts Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
| } catch (e) {} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 () { | ||
|
|
||
| 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() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 allowObject.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.