Skip to content
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

Merged
merged 5 commits into from
Oct 18, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions bin/server
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];
Copy link
Contributor

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) ?? [];

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(() => {});
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -281,6 +283,7 @@ void should_log_memory_config() {
Pattern.DOTALL
);
assertThat(buildResult.getLogs()).matches(warn);
assertThat(buildResult.getLogs()).contains("used_heap_size");
}

@NotNull
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@
"bin/"
],
"_moduleAliases": {
"@sonar/bridge": "lib/bridge/src",
Copy link
Contributor Author

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.

"@sonar/css": "lib/css/src",
"@sonar/html": "lib/html/src",
"@sonar/jsts": "lib/jsts/src",
Expand Down
27 changes: 26 additions & 1 deletion packages/bridge/src/memory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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,
saberduck marked this conversation as resolved.
Show resolved Hide resolved
)
.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()));
}
}
13 changes: 10 additions & 3 deletions packages/bridge/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
*/
Expand All @@ -83,6 +87,9 @@ export function start(
timeout = SHUTDOWN_TIMEOUT,
): Promise<http.Server> {
logMemoryConfiguration();
if (getContext().debugMemory) {
registerGarbageCollectionObserver();
}
return new Promise(resolve => {
debug('Starting the bridge server');

Expand Down Expand Up @@ -145,7 +152,7 @@ export function start(
});

server.on('close', () => {
debug('The bridge server shutted down');
debug('The bridge server shut down');
orphanTimeout.stop();
});

Expand Down
3 changes: 3 additions & 0 deletions packages/bridge/src/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -109,6 +110,7 @@ if (parentPort) {

case 'on-create-program': {
const { tsConfig } = data;
logHeapStatistics();

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:

error(
`You can see how Node.js heap usage evolves during analysis with "sonar.javascript.node.debugMemory=true"`,
);

What do you think?

Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

const { programId, files, projectReferences, missingTsConfig } =
createAndSaveProgram(tsConfig);
parentThread.postMessage({
Expand All @@ -128,6 +130,7 @@ if (parentPort) {
case 'on-delete-program': {
const { programId } = data;
deleteProgram(programId);
logHeapStatistics();
parentThread.postMessage({ type: 'success', result: 'OK!' });
break;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/bridge/tests/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

});
});

Expand Down
1 change: 1 addition & 0 deletions packages/shared/src/helpers/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export interface Context {
workDir: string;
shouldUseTypeScriptParserForJS: boolean;
sonarlint: boolean;
debugMemory?: boolean;

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?

Copy link
Contributor Author

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

bundles: string[];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -183,13 +184,12 @@ void startServer(SensorContext context, List<Path> 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)) {
Expand Down Expand Up @@ -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)
Expand All @@ -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());
Expand All @@ -262,7 +260,7 @@ private void initNodeCommand(
.getInt(MAX_OLD_SPACE_SIZE_PROPERTY)
.ifPresent(nodeCommandBuilder::maxOldSpaceSize);

nodeCommand = nodeCommandBuilder.build();
return nodeCommandBuilder.build();
}

private Map<String, String> getEnv() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "";
Expand Down