Skip to content

Commit

Permalink
src: add cli option to preserve env vars on dr
Browse files Browse the repository at this point in the history
PR-URL: #55697
Backport-PR-URL: #56055
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
  • Loading branch information
RafaelGSS authored and aduh95 committed Dec 10, 2024
1 parent 4f85047 commit 0aee9af
Show file tree
Hide file tree
Showing 12 changed files with 199 additions and 14 deletions.
10 changes: 10 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -2049,6 +2049,15 @@ Enables report to be generated upon receiving the specified (or predefined)
signal to the running Node.js process. The signal to trigger the report is
specified through `--report-signal`.

### `--report-exclude-env`

<!-- YAML
added: REPLACEME
-->

When `--report-exclude-env` is passed the diagnostic report generated will not
contain the `environmentVariables` data.

### `--report-signal=signal`

<!-- YAML
Expand Down Expand Up @@ -3136,6 +3145,7 @@ one is included in the list below.
* `--redirect-warnings`
* `--report-compact`
* `--report-dir`, `--report-directory`
* `--report-exclude-env`
* `--report-exclude-network`
* `--report-filename`
* `--report-on-fatalerror`
Expand Down
10 changes: 10 additions & 0 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -3478,6 +3478,16 @@ const { report } = require('node:process');
console.log(`Report on exception: ${report.reportOnUncaughtException}`);
```
### `process.report.excludeEnv`
<!-- YAML
added: REPLACEME
-->
* {boolean}
If `true`, a diagnostic report is generated without the environment variables.
### `process.report.signal`
<!-- YAML
Expand Down
7 changes: 7 additions & 0 deletions doc/api/report.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/55697
description: Added `--report-exclude-env` option for excluding environment variables from report generation.
- version: v22.0.0
pr-url: https://github.com/nodejs/node/pull/51645
description: Added `--report-exclude-network` option for excluding networking operations that can slow down report generation in some cases.
Expand Down Expand Up @@ -507,6 +510,10 @@ meaning of `SIGUSR2` for the said purposes.
in `libuv.*.(remote|local)Endpoint.host` from the diagnostic report.
By default this is not set and the network interfaces are included.

* `--report-exclude-env` Exclude `environmentVariables` from the
diagnostic report. By default this is not set and the environment
variables are included.

A report can also be triggered via an API call from a JavaScript application:

```js
Expand Down
7 changes: 7 additions & 0 deletions lib/internal/process/report.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,13 @@ const report = {

nr.setReportOnUncaughtException(trigger);
},
get excludeEnv() {
return nr.getExcludeEnv();
},
set excludeEnv(b) {
validateBoolean(b, 'excludeEnv');
nr.setExcludeEnv(b);
},
};

function addSignalHandler(sig) {
Expand Down
4 changes: 4 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -915,6 +915,10 @@ inline bool Environment::is_in_heapsnapshot_heap_limit_callback() const {
return is_in_heapsnapshot_heap_limit_callback_;
}

inline bool Environment::report_exclude_env() const {
return options_->report_exclude_env;
}

inline void Environment::AddHeapSnapshotNearHeapLimitCallback() {
DCHECK(!heapsnapshot_near_heap_limit_callback_added_);
heapsnapshot_near_heap_limit_callback_added_ = true;
Expand Down
2 changes: 2 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -1048,6 +1048,8 @@ class Environment final : public MemoryRetainer {
inline void set_heap_snapshot_near_heap_limit(uint32_t limit);
inline bool is_in_heapsnapshot_heap_limit_callback() const;

inline bool report_exclude_env() const;

inline void AddHeapSnapshotNearHeapLimitCallback();

inline void RemoveHeapSnapshotNearHeapLimitCallback(size_t heap_limit);
Expand Down
5 changes: 5 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -895,6 +895,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
&EnvironmentOptions::tls_max_v1_3,
kAllowedInEnvvar);

AddOption("--report-exclude-env",
"Exclude environment variables when generating report"
" (default: false)",
&EnvironmentOptions::report_exclude_env,
kAllowedInEnvvar);
AddOption("--report-exclude-network",
"exclude network interface diagnostics."
" (default: false)",
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ class EnvironmentOptions : public Options {

std::vector<std::string> user_argv;

bool report_exclude_env = false;
bool report_exclude_network = false;

inline DebugOptions* get_debug_options() { return &debug_options_; }
Expand Down
55 changes: 46 additions & 9 deletions src/node_report.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ static void WriteNodeReport(Isolate* isolate,
std::ostream& out,
Local<Value> error,
bool compact,
bool exclude_network = false);
bool exclude_network = false,
bool exclude_env = false);
static void PrintVersionInformation(JSONWriter* writer,
bool exclude_network = false);
static void PrintJavaScriptErrorStack(JSONWriter* writer,
Expand All @@ -78,6 +79,7 @@ static void PrintJavaScriptErrorProperties(JSONWriter* writer,
static void PrintNativeStack(JSONWriter* writer);
static void PrintResourceUsage(JSONWriter* writer);
static void PrintGCStatistics(JSONWriter* writer, Isolate* isolate);
static void PrintEnvironmentVariables(JSONWriter* writer);
static void PrintSystemInformation(JSONWriter* writer);
static void PrintLoadedLibraries(JSONWriter* writer);
static void PrintComponentVersions(JSONWriter* writer);
Expand All @@ -95,7 +97,8 @@ static void WriteNodeReport(Isolate* isolate,
std::ostream& out,
Local<Value> error,
bool compact,
bool exclude_network) {
bool exclude_network,
bool exclude_env) {
// Obtain the current time and the pid.
TIME_TYPE tm_struct;
DiagnosticFilename::LocalTime(&tm_struct);
Expand Down Expand Up @@ -251,6 +254,9 @@ static void WriteNodeReport(Isolate* isolate,
writer.json_arrayend();

// Report operating system information
if (exclude_env == false) {
PrintEnvironmentVariables(&writer);
}
PrintSystemInformation(&writer);

writer.json_objectend();
Expand Down Expand Up @@ -696,8 +702,7 @@ static void PrintResourceUsage(JSONWriter* writer) {
#endif // RUSAGE_THREAD
}

// Report operating system information.
static void PrintSystemInformation(JSONWriter* writer) {
static void PrintEnvironmentVariables(JSONWriter* writer) {
uv_env_item_t* envitems;
int envcount;
int r;
Expand All @@ -717,7 +722,10 @@ static void PrintSystemInformation(JSONWriter* writer) {
}

writer->json_objectend();
}

// Report operating system information.
static void PrintSystemInformation(JSONWriter* writer) {
#ifndef _WIN32
static struct {
const char* description;
Expand Down Expand Up @@ -917,6 +925,10 @@ std::string TriggerNodeReport(Isolate* isolate,
bool exclude_network = env != nullptr ? env->options()->report_exclude_network
: per_process::cli_options->per_isolate
->per_env->report_exclude_network;
bool exclude_env =
env != nullptr
? env->report_exclude_env()
: per_process::cli_options->per_isolate->per_env->report_exclude_env;

report::WriteNodeReport(isolate,
env,
Expand All @@ -926,7 +938,8 @@ std::string TriggerNodeReport(Isolate* isolate,
*outstream,
error,
compact,
exclude_network);
exclude_network,
exclude_env);

// Do not close stdout/stderr, only close files we opened.
if (outfile.is_open()) {
Expand Down Expand Up @@ -980,8 +993,20 @@ void GetNodeReport(Isolate* isolate,
bool exclude_network = env != nullptr ? env->options()->report_exclude_network
: per_process::cli_options->per_isolate
->per_env->report_exclude_network;
report::WriteNodeReport(
isolate, env, message, trigger, "", out, error, false, exclude_network);
bool exclude_env =
env != nullptr
? env->report_exclude_env()
: per_process::cli_options->per_isolate->per_env->report_exclude_env;
report::WriteNodeReport(isolate,
env,
message,
trigger,
"",
out,
error,
false,
exclude_network,
exclude_env);
}

// External function to trigger a report, writing to a supplied stream.
Expand All @@ -997,8 +1022,20 @@ void GetNodeReport(Environment* env,
bool exclude_network = env != nullptr ? env->options()->report_exclude_network
: per_process::cli_options->per_isolate
->per_env->report_exclude_network;
report::WriteNodeReport(
isolate, env, message, trigger, "", out, error, false, exclude_network);
bool exclude_env =
env != nullptr
? env->report_exclude_env()
: per_process::cli_options->per_isolate->per_env->report_exclude_env;
report::WriteNodeReport(isolate,
env,
message,
trigger,
"",
out,
error,
false,
exclude_network,
exclude_env);
}

} // namespace node
15 changes: 15 additions & 0 deletions src/node_report_module.cc
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,17 @@ static void SetExcludeNetwork(const FunctionCallbackInfo<Value>& info) {
env->options()->report_exclude_network = info[0]->IsTrue();
}

static void GetExcludeEnv(const FunctionCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
info.GetReturnValue().Set(env->report_exclude_env());
}

static void SetExcludeEnv(const FunctionCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
CHECK(info[0]->IsBoolean());
env->options()->report_exclude_env = info[0]->IsTrue();
}

static void GetDirectory(const FunctionCallbackInfo<Value>& info) {
Mutex::ScopedLock lock(per_process::cli_options_mutex);
Environment* env = Environment::GetCurrent(info);
Expand Down Expand Up @@ -187,6 +198,8 @@ static void Initialize(Local<Object> exports,
SetMethod(context, exports, "setCompact", SetCompact);
SetMethod(context, exports, "getExcludeNetwork", GetExcludeNetwork);
SetMethod(context, exports, "setExcludeNetwork", SetExcludeNetwork);
SetMethod(context, exports, "getExcludeEnv", GetExcludeEnv);
SetMethod(context, exports, "setExcludeEnv", SetExcludeEnv);
SetMethod(context, exports, "getDirectory", GetDirectory);
SetMethod(context, exports, "setDirectory", SetDirectory);
SetMethod(context, exports, "getFilename", GetFilename);
Expand Down Expand Up @@ -215,6 +228,8 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(SetCompact);
registry->Register(GetExcludeNetwork);
registry->Register(SetExcludeNetwork);
registry->Register(GetExcludeEnv);
registry->Register(SetExcludeEnv);
registry->Register(GetDirectory);
registry->Register(SetDirectory);
registry->Register(GetFilename);
Expand Down
17 changes: 12 additions & 5 deletions test/common/report.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,12 @@ function _validateContent(report, fields = []) {

// Verify that all sections are present as own properties of the report.
const sections = ['header', 'nativeStack', 'javascriptStack', 'libuv',
'environmentVariables', 'sharedObjects', 'resourceUsage', 'workers'];
'sharedObjects', 'resourceUsage', 'workers'];

if (!process.report.excludeEnv) {
sections.push('environmentVariables');
}

if (!isWindows)
sections.push('userLimits');

Expand Down Expand Up @@ -294,10 +299,12 @@ function _validateContent(report, fields = []) {
resource.type === 'loop' ? 'undefined' : 'boolean');
});

// Verify the format of the environmentVariables section.
for (const [key, value] of Object.entries(report.environmentVariables)) {
assert.strictEqual(typeof key, 'string');
assert.strictEqual(typeof value, 'string');
if (!process.report.excludeEnv) {
// Verify the format of the environmentVariables section.
for (const [key, value] of Object.entries(report.environmentVariables)) {
assert.strictEqual(typeof key, 'string');
assert.strictEqual(typeof value, 'string');
}
}

// Verify the format of the userLimits section on non-Windows platforms.
Expand Down
80 changes: 80 additions & 0 deletions test/report/test-report-writereport-exclude-env.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Flags: --report-exclude-env
'use strict';

// Test producing a report via API call, using the no-hooks/no-signal interface.
require('../common');
const assert = require('assert');
const fs = require('fs');
const path = require('path');
const helper = require('../common/report');
const tmpdir = require('../common/tmpdir');

tmpdir.refresh();
process.report.directory = tmpdir.path;

function validate() {
const reports = helper.findReports(process.pid, tmpdir.path);
assert.strictEqual(reports.length, 1);
helper.validate(reports[0], arguments[0]);
fs.unlinkSync(reports[0]);
return reports[0];
}

{
// Test with no arguments.
process.report.writeReport();
validate();
}

{
// Test with an error argument.
process.report.writeReport(new Error('test error'));
validate();
}

{
// Test with an error with one line stack
const error = new Error();
error.stack = 'only one line';
process.report.writeReport(error);
validate();
}

{
const error = new Error();
error.foo = 'goo';
process.report.writeReport(error);
validate([['javascriptStack.errorProperties.foo', 'goo']]);
}

{
// Test with a file argument.
const file = process.report.writeReport('custom-name-1.json');
const absolutePath = tmpdir.resolve(file);
assert.strictEqual(helper.findReports(process.pid, tmpdir.path).length, 0);
assert.strictEqual(file, 'custom-name-1.json');
helper.validate(absolutePath);
fs.unlinkSync(absolutePath);
}

{
// Test with file and error arguments.
const file = process.report.writeReport('custom-name-2.json',
new Error('test error'));
const absolutePath = tmpdir.resolve(file);
assert.strictEqual(helper.findReports(process.pid, tmpdir.path).length, 0);
assert.strictEqual(file, 'custom-name-2.json');
helper.validate(absolutePath);
fs.unlinkSync(absolutePath);
}

{
// Test with a filename option.
process.report.filename = 'custom-name-3.json';
const file = process.report.writeReport();
assert.strictEqual(helper.findReports(process.pid, tmpdir.path).length, 0);
const filename = path.join(process.report.directory, 'custom-name-3.json');
assert.strictEqual(file, process.report.filename);
helper.validate(filename);
fs.unlinkSync(filename);
}

0 comments on commit 0aee9af

Please sign in to comment.