report: fix typos in memory limit units

[technic960183]

Replace typos "kbytes" with "bytes" in PrintSystemInformation() in src/node_report.cc for "data_seg_size_kbytes", "max_memory_size_kbytes" and "virtual_memory_kbytes", as RLIMIT_DATA, RLIMIT_RSS and RLIMIT_AS from <sys/resource.h> are given in bytes. (ref)

I found this problem when I was testing the limit of virtual memory. When I set it to 32GB, the report showed

"virtual_memory_kbytes": {
  "soft": 34359738368,
  "hard": 34359738368
}

After checking the source codes, I think that it should be in bytes, not kbytes.

Refs: https://www.ibm.com/docs/en/aix/7.3?topic=k-kgetrlimit64-kernel-service

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.00%. Comparing base (9c046ea) to head (eebdcdd). Report is 56 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #56068   +/-   ##
=======================================
  Coverage   88.00%   88.00%           
=======================================
  Files         656      656           
  Lines      189131   189136    +5     
  Branches    36009    36007    -2     
=======================================
+ Hits       166439   166447    +8     
+ Misses      15853    15850    -3     
  Partials     6839     6839           

Files with missing lines	Coverage Δ
src/node_report.cc	93.53% <ø> (ø)

... and 29 files with indirect coverage changes

[legendecas]

Nice spot! Thank you for the contribution, I think this could be a breaking change in removing keys in the diagnostic report, even their values are incorrect. I think an alternative could be adding both the new keys and keeping the old ones, but with correct values.

[technic960183]

Hi @legendecas,

Thank you so much for your thoughtful reply and for spotting the potential compatibility issue! This is my first contribution to a public, well-known repository, so I greatly appreciate your guidance and encouragement.

Coming from the Python community, I am aware of deprecation mechanisms (like deprecation warnings) that allow for gradual transitions when updating APIs or key names. Does Node have a similar mechanism to flag keys like *_kbytes as deprecated while providing their updated counterparts (*_bytes)? If so, this might help in balancing backward compatibility and correctness.

Alternatively, I’m also fine with keeping the same keys and ensuring they provide the correct values in bytes. That said, I realize there’s an edge case to consider: the RLIMIT values might not always be divisible by 1024, which could result in rounded values when using the *_kbytes keys. I’m not sure how rare such cases are or how likely this breaking change would cause issues for users. If someone is relying on these keys, they might have already noticed and addressed this bug in their own code. Would providing the correct values (and lead to an incorrect result in their pipe) in bytes potentially be better than users encountering a missing key error?

Since I’m still a student with a background mainly in science and very new to software development, I’ll gladly adjust my PR based on your guidance. Would it be necessary for me to write tests for the changes if I provide the correct values for the original keys? I’m unsure how to write unit tests that require system-level settings as input, so any advice or pointers would be really helpful.

Thank you again for guiding me through this process!

[legendecas]

IIRC, this is the first time that a key is been removed from the report. Referencing the version number explainer:

Diagnostic report has an associated single-digit version number (report.header.reportVersion), uniquely representing the report format. The version number is bumped when new key is added or removed, or the data type of a value is changed. Report version definitions are consistent across LTS releases. https://github.com/nodejs/node/blob/main/doc/api/report.md

Given that either adding or removing keys needs a version bump, the keys *_kbytes can be removed at the same time when new keys *_bytes are added. The report version number can be bumped at https://github.com/nodejs/node/blob/main/src/node_report.cc#L26.

I filed https://github.com/nodejs/node/pull/56130 to trace the changes in the diagnostic report. And the change in this PR should be recorded in the same way.

/cc @nodejs/diagnostics

[technic960183]

To prevent a merge conflict, should I wait until #56130 landing then rebase and force push to this branch? Or should I update this now, so you can review it sooner? (Then resolved the conflict later.)

[legendecas]

I would suggest waiting a bit to avoid conflicts and unnecessary labor.

[legendecas]

@technic960183 would you mind rebaseing on top of the main branch and bumping the version/adding the change doc? Thank you!

[technic960183]

@legendecas Thanks for your guidance.

Should the Node version be v23.4.0 in the version 5 report version history?

By the way, I notice that the example report in doc/api/report.md, for core_file_size_blocks, the value of soft is "". And also hard is smaller than soft in the fields max_locked_memory_bytes, open_files, and max_user_processes. Will these ever happened in a real report? If it will not happen, could I fix it in this PR or I should open another?

  "userLimits": {
    "core_file_size_blocks": {
      "soft": "",
      "hard": "unlimited"
    },
    "max_locked_memory_bytes": {
      "soft": "unlimited",
      "hard": 65536
    },
    "open_files": {
      "soft": "unlimited",
      "hard": 4096
    },
    "max_user_processes": {
      "soft": "unlimited",
      "hard": 4127290
    }
  }

I have checked that the test will only check if the value is a number (or 'unlimited').

      assert(typeof limits.soft === 'number' || limits.soft === 'unlimited',
             `Invalid ${type} soft limit of ${limits.soft}`);

[technic960183]

I think I do something wrong when I try to rebase it. ~~I follow the steps but something goes wrong. Maybe it is because I hit Sync fork on my Github Fork?~~ I think that I mixed up upstream/HEAD and upstream/v23.x-staging. I'll try to fix it

Update: Fixed. Sorry for the trouble caused. As a first-time contributor to a large public project, it's hard to explain how nerve-wracking it was to realize at 1 AM that I had messed up the entire commit history and push it as a PR.

[legendecas]

Should the Node version be v23.4.0 in the version 5 report version history?

Please use REPLACEME to refer to the unreleased version: https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-3-code

Will these ever happened in a real report? If it will not happen, could I fix it in this PR or I should open another?

I think it is reasonable to update the example with a more realistic sample. Please feel free to update it in a separate PR as they are not related to this PR change.

I have checked that the test will only check if the value is a number (or 'unlimited').

I don't think we need to verify the value. They are retrieved from external source. The report should reflect what as exact as possible about the external source says.

[technic960183]

Bumped the report version and added the corresponding version history. And have used REPLACEME to refer to the unreleased version.

@legendecas would you mind taking a look again? Thanks.

[technic960183]

Modified as suggested. Thank you!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64012/

[technic960183]

@legendecas Some of the tests in the ci failed. But I don't understand why my commit caused them to fail. Do I need to modify anything? Thank you.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64020/

[legendecas]

I think they are unrelevant flaky tests: https://github.com/nodejs/node/issues/56190. Restarted the CI.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64025/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64030/

[nodejs-github-bot]

Landed in 938a581b4da945a468da5c423c4814be64c5bc97