-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
feat: add platform/arch information to sendInfo
#6497
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
base: master
Are you sure you want to change the base?
Conversation
49ff541 to
7451f66
Compare
|
Instead of adding one 'platform' variable to the 'info' object, I decided to add this as an object, where 'platform' is an object. Doing that, opens for future extensions without the risk of cluttering the 'info' object. Feel free to add or delete elements as you see fit. |
|
Runtime and arch make sense to me. |
I initially set it up that way as examples to show the possibilities, and therefore I commented 'Feel free to add or delete elements as you see fit'. My point was that making this an object instead of a single variable, makes it more open to future enhancements without cluttering the 'info' object. I will keep 'arch' and 'platform' (linux/win32/whatever). I am struggling with the Github test environment failing. I have pushed and revoked several times, and I cannot see any apparent reason for the failures. It has failed different places each time, but the 'e2e-tesst' is stubborn. From previous PRs, I have experienced several rounds of test failures before success, even if the code is the same. |
sendInfo
CommanderStorm
left a comment
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.
LGTM, thanks for this enhancement
|
why are you closing/reopening your PRs all the time? If e2e-tesst flakes but this is not an actual issue, I can rerun it. |
|
we should likely still fix the testcase to run reliably though. |
I haven't found any other way to restart the tests |
ℹ️ To keep reviews fast and effective, please make sure you’ve read our pull request guidelines
📝 Summary of changes done and why they are done
Refactored the
sendInfosocket handler to group system-related information into a dedicatedruntimeobject.Changes:
runtimeobject to the root of the info object.runtime:arch,nodeVersion,uptime, andmemory(RSS and Heap).infoobject for better readability.Reasoning:
infoobject as we add more metrics.runtimeinstead ofprocessto avoid cognitive confusion/naming collisions with the global Node.jsprocessvariable.📋 Related issues
📄 Checklist
Please follow this checklist to avoid unnecessary back and forth (click to expand)