From 50966d2c4ab2716fb209c3165474ad8467524c49 Mon Sep 17 00:00:00 2001 From: Tibor Blenessy Date: Tue, 17 Oct 2023 18:31:32 +0200 Subject: [PATCH 1/5] Report heap usage status with `sonar.javascript.node.debugMemory` --- bin/server | 7 ++-- package.json | 1 + packages/bridge/src/memory.ts | 27 +++++++++++++++- packages/bridge/src/server.ts | 13 ++++++-- packages/bridge/src/worker.js | 3 ++ packages/bridge/tests/server.test.ts | 2 +- packages/shared/src/helpers/context.ts | 1 + .../javascript/bridge/BridgeServerImpl.java | 32 +++++++++---------- .../bridge/BridgeServerImplTest.java | 11 +++++++ .../test/resources/mock-bridge/startServer.js | 3 +- 10 files changed, 74 insertions(+), 26 deletions(-) diff --git a/bin/server b/bin/server index afdffbfe5b..0465744dc6 100644 --- a/bin/server +++ b/bin/server @@ -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 = []; -if (process.argv[7]) { - bundles = process.argv[7].split(path.delimiter); +if (process.argv[8]) { + bundles = process.argv[8].split(path.delimiter); } -context.setContext({ workDir, shouldUseTypeScriptParserForJS, sonarlint, bundles }); +context.setContext({ workDir, shouldUseTypeScriptParserForJS, sonarlint, debugMemory, bundles }); server.start(Number.parseInt(port), host).catch(() => {}); diff --git a/package.json b/package.json index ab9402acca..76325d5ff6 100644 --- a/package.json +++ b/package.json @@ -151,6 +151,7 @@ "bin/" ], "_moduleAliases": { + "@sonar/bridge": "lib/bridge/src", "@sonar/css": "lib/css/src", "@sonar/html": "lib/html/src", "@sonar/jsts": "lib/jsts/src", diff --git a/packages/bridge/src/memory.ts b/packages/bridge/src/memory.ts index da96595096..2077f94e73 100644 --- a/packages/bridge/src/memory.ts +++ b/packages/bridge/src/memory.ts @@ -21,7 +21,9 @@ import * as v8 from 'v8'; import * as os from 'os'; import fs from 'fs'; -import { error, info, warn } from '@sonar/shared/helpers'; +import { debug, error, getContext, info, warn } from '@sonar/shared/helpers'; +import { constants, PerformanceObserver } from 'node:perf_hooks'; +import { NodeGCPerformanceDetail } from 'perf_hooks'; const MB = 1024 * 1024; @@ -83,3 +85,26 @@ export function logMemoryError(err: any) { break; } } + +export function registerGarbageCollectionObserver() { + const obs = new PerformanceObserver(items => { + items + .getEntries() + .filter( + item => + (item.detail as NodeGCPerformanceDetail).kind === constants.NODE_PERFORMANCE_GC_MAJOR, + ) + .forEach(item => { + debug(`Major GC event`); + debug(JSON.stringify(item)); + logHeapStatistics(); + }); + }); + obs.observe({ entryTypes: ['gc'] }); +} + +export function logHeapStatistics() { + if (getContext().debugMemory) { + debug(JSON.stringify(v8.getHeapStatistics())); + } +} diff --git a/packages/bridge/src/server.ts b/packages/bridge/src/server.ts index c0893d26d6..7e278606d9 100644 --- a/packages/bridge/src/server.ts +++ b/packages/bridge/src/server.ts @@ -32,7 +32,11 @@ import { debug, getContext } from '@sonar/shared/helpers'; import { timeoutMiddleware } from './timeout'; import { AddressInfo } from 'net'; import { Worker } from 'worker_threads'; -import { logMemoryConfiguration, logMemoryError } from './memory'; +import { + registerGarbageCollectionObserver, + logMemoryConfiguration, + logMemoryError, +} from './memory'; /** * The maximum request body size @@ -73,7 +77,7 @@ let worker: Worker; * which embeds it or directly with SonarLint. * * @param port the port to listen to - * @param host only for usage from outside of NodeJS - Java plugin, SonarLint, ... + * @param host only for usage from outside of Node.js - Java plugin, SonarLint, ... * @param timeout timeout in ms to shut down the server if unresponsive * @returns an http server */ @@ -83,6 +87,9 @@ export function start( timeout = SHUTDOWN_TIMEOUT, ): Promise { logMemoryConfiguration(); + if (getContext().debugMemory) { + registerGarbageCollectionObserver(); + } return new Promise(resolve => { debug('Starting the bridge server'); @@ -145,7 +152,7 @@ export function start( }); server.on('close', () => { - debug('The bridge server shutted down'); + debug('The bridge server shut down'); orphanTimeout.stop(); }); diff --git a/packages/bridge/src/worker.js b/packages/bridge/src/worker.js index 5039b42049..8a2a3ebea3 100644 --- a/packages/bridge/src/worker.js +++ b/packages/bridge/src/worker.js @@ -35,6 +35,7 @@ const { analyzeCSS } = require('@sonar/css'); const { analyzeHTML } = require('@sonar/html'); const { analyzeYAML } = require('@sonar/yaml'); const { APIError, ErrorCode } = require('@sonar/shared/errors'); +const { logHeapStatistics } = require('@sonar/bridge/memory'); /** * Delegate the handling of an HTTP request to a worker thread @@ -109,6 +110,7 @@ if (parentPort) { case 'on-create-program': { const { tsConfig } = data; + logHeapStatistics(); const { programId, files, projectReferences, missingTsConfig } = createAndSaveProgram(tsConfig); parentThread.postMessage({ @@ -128,6 +130,7 @@ if (parentPort) { case 'on-delete-program': { const { programId } = data; deleteProgram(programId); + logHeapStatistics(); parentThread.postMessage({ type: 'success', result: 'OK!' }); break; } diff --git a/packages/bridge/tests/server.test.ts b/packages/bridge/tests/server.test.ts index 7d8d041ab6..6e47488818 100644 --- a/packages/bridge/tests/server.test.ts +++ b/packages/bridge/tests/server.test.ts @@ -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'); }); }); diff --git a/packages/shared/src/helpers/context.ts b/packages/shared/src/helpers/context.ts index ca496fd7a8..c6905e6bd4 100644 --- a/packages/shared/src/helpers/context.ts +++ b/packages/shared/src/helpers/context.ts @@ -30,6 +30,7 @@ export interface Context { workDir: string; shouldUseTypeScriptParserForJS: boolean; sonarlint: boolean; + debugMemory?: boolean; bundles: string[]; } diff --git a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BridgeServerImpl.java b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BridgeServerImpl.java index f4dd770aee..ba10256987 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BridgeServerImpl.java +++ b/sonar-plugin/sonar-javascript-plugin/src/main/java/org/sonar/plugins/javascript/bridge/BridgeServerImpl.java @@ -71,6 +71,7 @@ private enum Status { // internal property to set "--max-old-space-size" for Node process running this server private static final String MAX_OLD_SPACE_SIZE_PROPERTY = "sonar.javascript.node.maxspace"; private static final String ALLOW_TS_PARSER_JS_FILES = "sonar.javascript.allowTsParserJsFiles"; + private static final String DEBUG_MEMORY = "sonar.javascript.node.debugMemory"; public static final String SONARJS_EXISTING_NODE_PROCESS_PORT = "SONARJS_EXISTING_NODE_PROCESS_PORT"; private static final Gson GSON = new Gson(); @@ -183,13 +184,12 @@ void startServer(SensorContext context, List deployedBundles) throws IOExc ); } + LOG.debug("Creating Node.js process to start the bridge server on port " + port); String bundles = deployedBundles .stream() .map(Path::toString) .collect(Collectors.joining(File.pathSeparator)); - initNodeCommand(context, scriptFile, context.fileSystem().workDir(), bundles); - - LOG.debug("Creating Node.js process to start the bridge server on port " + port); + nodeCommand = initNodeCommand(context, scriptFile, bundles); nodeCommand.start(); if (!waitServerToStart(timeoutSeconds * 1000)) { @@ -221,24 +221,21 @@ boolean waitServerToStart(int timeoutMs) { return true; } - private void initNodeCommand( - SensorContext context, - File scriptFile, - File workDir, - String bundles - ) throws IOException { - boolean allowTsParserJsFiles = context - .config() - .getBoolean(ALLOW_TS_PARSER_JS_FILES) - .orElse(true); - boolean isSonarLint = context.runtime().getProduct() == SonarProduct.SONARLINT; + private NodeCommand initNodeCommand(SensorContext context, File scriptFile, String bundles) + throws IOException { + var workdir = context.fileSystem().workDir().getAbsolutePath(); + var config = context.config(); + var allowTsParserJsFiles = config.getBoolean(ALLOW_TS_PARSER_JS_FILES).orElse(true); + var isSonarLint = context.runtime().getProduct() == SonarProduct.SONARLINT; if (isSonarLint) { LOG.info("Running in SonarLint context, metrics will not be computed."); } + var debugMemory = config.getBoolean(DEBUG_MEMORY).orElse(false); + + // enable per rule performance tracking https://eslint.org/docs/1.0.0/developer-guide/working-with-rules#per-rule-performance var outputConsumer = monitoring.isMonitoringEnabled() ? new LogOutputConsumer().andThen(new MonitoringOutputConsumer(monitoring)) : new LogOutputConsumer(); - // enable per rule performance tracking https://eslint.org/docs/1.0.0/developer-guide/working-with-rules#per-rule-performance nodeCommandBuilder .outputConsumer(outputConsumer) @@ -250,9 +247,10 @@ private void initNodeCommand( .scriptArgs( String.valueOf(port), hostAddress, - workDir.getAbsolutePath(), + workdir, String.valueOf(allowTsParserJsFiles), String.valueOf(isSonarLint), + String.valueOf(debugMemory), bundles ) .env(getEnv()); @@ -262,7 +260,7 @@ private void initNodeCommand( .getInt(MAX_OLD_SPACE_SIZE_PROPERTY) .ifPresent(nodeCommandBuilder::maxOldSpaceSize); - nodeCommand = nodeCommandBuilder.build(); + return nodeCommandBuilder.build(); } private Map getEnv() { diff --git a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java index f2281b4366..b6a1182646 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java +++ b/sonar-plugin/sonar-javascript-plugin/src/test/java/org/sonar/plugins/javascript/bridge/BridgeServerImplTest.java @@ -649,6 +649,17 @@ void should_skip_metrics_on_sonarlint() throws Exception { assertThat(logTester.logs()).contains("sonarlint: true"); } + @Test + void should_pass_debug_memory_option() throws Exception { + bridgeServer = createBridgeServer(START_SERVER_SCRIPT); + bridgeServer.deploy(); + context.setSettings(new MapSettings().setProperty("sonar.javascript.node.debugMemory", "true")); + bridgeServer.startServer(context, Arrays.asList(Paths.get("bundle1"), Paths.get("bundle2"))); + bridgeServer.stop(); + + assertThat(logTester.logs()).contains("debugMemory: true"); + } + @Test void should_use_default_timeout() { bridgeServer = diff --git a/sonar-plugin/sonar-javascript-plugin/src/test/resources/mock-bridge/startServer.js b/sonar-plugin/sonar-javascript-plugin/src/test/resources/mock-bridge/startServer.js index 85eab9c67e..6162e463d4 100644 --- a/sonar-plugin/sonar-javascript-plugin/src/test/resources/mock-bridge/startServer.js +++ b/sonar-plugin/sonar-javascript-plugin/src/test/resources/mock-bridge/startServer.js @@ -6,7 +6,8 @@ const host = process.argv[3]; console.log(`allowTsParserJsFiles: ${process.argv[5]}`); console.log(`sonarlint: ${process.argv[6]}`); -console.log(`additional rules: [${process.argv[7]}]`); +console.log(`debugMemory: ${process.argv[7]}`); +console.log(`additional rules: [${process.argv[8]}]`); const requestHandler = (request, response) => { let data = ""; From e61a0585361aa7eb3a29916f6ad17b11f4455c95 Mon Sep 17 00:00:00 2001 From: Tibor Blenessy Date: Tue, 17 Oct 2023 23:25:59 +0200 Subject: [PATCH 2/5] update its --- .../com/sonar/javascript/it/plugin/EslintBasedRulesTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/its/plugin/tests/src/test/java/com/sonar/javascript/it/plugin/EslintBasedRulesTest.java b/its/plugin/tests/src/test/java/com/sonar/javascript/it/plugin/EslintBasedRulesTest.java index 917d352122..d68a03f5be 100644 --- a/its/plugin/tests/src/test/java/com/sonar/javascript/it/plugin/EslintBasedRulesTest.java +++ b/its/plugin/tests/src/test/java/com/sonar/javascript/it/plugin/EslintBasedRulesTest.java @@ -266,6 +266,8 @@ void should_log_memory_config() { .setSourceEncoding("UTF-8") .setSourceDirs(".") .setProperty("sonar.javascript.node.maxspace", "500000") + .setProperty("sonar.javascript.node.debugMemory", "true") + .setDebugLogs(true) .setProjectDir(projectDir); var buildResult = orchestrator.executeBuild(build); @@ -281,6 +283,7 @@ void should_log_memory_config() { Pattern.DOTALL ); assertThat(buildResult.getLogs()).matches(warn); + assertThat(buildResult.getLogs()).contains("used_heap_size"); } @NotNull From e4e0fc8bd71108de6100c22e532a0a0850daf760 Mon Sep 17 00:00:00 2001 From: Tibor Blenessy Date: Wed, 18 Oct 2023 09:59:52 +0200 Subject: [PATCH 3/5] Update packages/bridge/src/memory.ts Co-authored-by: Yassin Kammoun <52890329+yassin-kammoun-sonarsource@users.noreply.github.com> --- packages/bridge/src/memory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/bridge/src/memory.ts b/packages/bridge/src/memory.ts index 2077f94e73..41b8980aaa 100644 --- a/packages/bridge/src/memory.ts +++ b/packages/bridge/src/memory.ts @@ -92,7 +92,7 @@ export function registerGarbageCollectionObserver() { .getEntries() .filter( item => - (item.detail as NodeGCPerformanceDetail).kind === constants.NODE_PERFORMANCE_GC_MAJOR, + (item.detail as NodeGCPerformanceDetail)?.kind === constants.NODE_PERFORMANCE_GC_MAJOR, ) .forEach(item => { debug(`Major GC event`); From 86fe4d558481f67ab626fc0b49e58858eaca5807 Mon Sep 17 00:00:00 2001 From: Tibor Blenessy Date: Wed, 18 Oct 2023 10:02:17 +0200 Subject: [PATCH 4/5] review --- bin/server | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/bin/server b/bin/server index 0465744dc6..2dfa41dbe2 100644 --- a/bin/server +++ b/bin/server @@ -22,10 +22,7 @@ const shouldUseTypeScriptParserForJS = process.argv[5] !== 'false'; const sonarlint = process.argv[6] === 'true'; const debugMemory = process.argv[7] === 'true'; -let bundles = []; -if (process.argv[8]) { - bundles = process.argv[8].split(path.delimiter); -} +const bundles = process.argv[8]?.split(path.delimiter) ?? []; context.setContext({ workDir, shouldUseTypeScriptParserForJS, sonarlint, debugMemory, bundles }); server.start(Number.parseInt(port), host).catch(() => {}); From 7cfda6ac519d53cb7b0325cad1a9b4e1a0b38d2b Mon Sep 17 00:00:00 2001 From: Tibor Blenessy Date: Wed, 18 Oct 2023 10:49:52 +0200 Subject: [PATCH 5/5] fix --- bin/server | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bin/server b/bin/server index 2dfa41dbe2..0465744dc6 100644 --- a/bin/server +++ b/bin/server @@ -22,7 +22,10 @@ const shouldUseTypeScriptParserForJS = process.argv[5] !== 'false'; const sonarlint = process.argv[6] === 'true'; const debugMemory = process.argv[7] === 'true'; -const bundles = process.argv[8]?.split(path.delimiter) ?? []; +let bundles = []; +if (process.argv[8]) { + bundles = process.argv[8].split(path.delimiter); +} context.setContext({ workDir, shouldUseTypeScriptParserForJS, sonarlint, debugMemory, bundles }); server.start(Number.parseInt(port), host).catch(() => {});