-
Notifications
You must be signed in to change notification settings - Fork 184
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
Report heap usage status with sonar.javascript.node.debugMemory
#4294
Conversation
@@ -151,6 +151,7 @@ | |||
"bin/" | |||
], | |||
"_moduleAliases": { | |||
"@sonar/bridge": "lib/bridge/src", |
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.
I had to do this to be able to require
memory
in worker, although they are in the same dir. Not sure if there is easier / better way.
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.
Looks great overall, nice work!
Could you address my questions before I approve?
@@ -30,6 +30,7 @@ export interface Context { | |||
workDir: string; | |||
shouldUseTypeScriptParserForJS: boolean; | |||
sonarlint: boolean; | |||
debugMemory?: boolean; |
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.
Did you mark the field as optional because some unit tests would fail otherwise?
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.
exactly, it's created in many places.
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.
👍
@@ -109,6 +110,7 @@ if (parentPort) { | |||
|
|||
case 'on-create-program': { | |||
const { tsConfig } = data; | |||
logHeapStatistics(); |
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.
Just to be sure that it's intentional, logs will only be printed when analyzing with programs, not with watch programs, right?
We might face a weird situation where SonarLint users see the log about out-of-memory suggesting to enable the property sonar.javascript.node.debugMemory
. However, even if they try to enable it, they won't see heap status.
Maybe the following line should be executed conditionally, when getContext().sonarlint === false
:
SonarJS/packages/bridge/src/memory.ts
Lines 70 to 72 in 0ef5749
error( | |
`You can see how Node.js heap usage evolves during analysis with "sonar.javascript.node.debugMemory=true"`, | |
); |
What do you think?
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.
yes, you are right, however, it's quite a corner case, I would not overcomplicate this
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.
👍
@@ -146,7 +146,7 @@ describe('server', () => { | |||
await new Promise(r => setTimeout(r, 600)); | |||
expect(server.listening).toBeFalsy(); | |||
|
|||
expect(console.log).toHaveBeenCalledWith('DEBUG The bridge server shutted down'); | |||
expect(console.log).toHaveBeenCalledWith('DEBUG The bridge server shut down'); |
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.
Thanks!
@@ -20,11 +20,12 @@ const host = process.argv[3]; | |||
const workDir = process.argv[4]; | |||
const shouldUseTypeScriptParserForJS = process.argv[5] !== 'false'; | |||
const sonarlint = process.argv[6] === 'true'; | |||
const debugMemory = process.argv[7] === 'true'; | |||
|
|||
let bundles = []; |
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.
Maybe we could get rid of the if-statement and replace the bundles-related lines with:
const bundles = process.argv[8]?.split(path.delimiter) ?? [];
Co-authored-by: Yassin Kammoun <[email protected]>
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.
Nice work!
SonarQube Quality Gate |
Fixes #4256