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

JS-498 Add runtime telemetry #5017

Merged
merged 6 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.sonar.api.batch.fs.InputFile;
import org.sonar.api.batch.fs.TextRange;
import org.sonar.api.scanner.ScannerSide;
import org.sonar.api.utils.Version;
import org.sonar.plugins.javascript.bridge.protobuf.Node;
import org.sonarsource.api.sonarlint.SonarLintSide;

Expand Down Expand Up @@ -68,7 +69,7 @@ void initLinter(

TsConfigFile createTsConfigFile(String content) throws IOException;

TelemetryResponse getTelemetry();
TelemetryData getTelemetry();

record JsAnalysisRequest(
String filePath,
Expand Down Expand Up @@ -278,7 +279,11 @@ public String toString() {

record TsProgramRequest(String tsConfig) {}

record TelemetryResponse(List<Dependency> dependencies) {}
record TelemetryEslintBridgeResponse(List<Dependency> dependencies) {}

record TelemetryData(List<Dependency> dependencies, RuntimeTelemetry runtimeTelemetry) {}

record Dependency(String name, String version) {}

record RuntimeTelemetry(Version version, String nodeExecutableOrigin) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -534,12 +534,22 @@ public TsConfigFile createTsConfigFile(String content) {
}

@Override
public TelemetryResponse getTelemetry() {
public TelemetryData getTelemetry() {
return new TelemetryData(
getTelemetryEslintBridgeResponse().dependencies(),
new RuntimeTelemetry(
nodeCommand.getActualNodeVersion(),
nodeCommand.getNodeExecutableOrigin()
)
);
}

private TelemetryEslintBridgeResponse getTelemetryEslintBridgeResponse() {
try {
var result = http.get(url("get-telemetry"));
return GSON.fromJson(result, TelemetryResponse.class);
return GSON.fromJson(result, TelemetryEslintBridgeResponse.class);
} catch (IOException e) {
return new TelemetryResponse(List.of());
return new TelemetryEslintBridgeResponse(List.of());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public class NodeCommand {
private final Map<String, String> env;
private Process process;
private final List<String> command;
private final String nodeExecutableOrigin;

NodeCommand(
ProcessWrapper processWrapper,
Expand All @@ -58,14 +59,16 @@ public class NodeCommand {
List<String> args,
Consumer<String> outputConsumer,
Consumer<String> errorConsumer,
Map<String, String> env
Map<String, String> env,
String nodeExecutableOrigin
) {
this.processWrapper = processWrapper;
this.command = buildCommand(nodeExecutable, nodeJsArgs, scriptFilename, args);
this.actualNodeVersion = actualNodeVersion;
this.outputConsumer = outputConsumer;
this.errorConsumer = errorConsumer;
this.env = env;
this.nodeExecutableOrigin = nodeExecutableOrigin;
}

/**
Expand Down Expand Up @@ -132,4 +135,8 @@ public String toString() {
public Version getActualNodeVersion() {
return actualNodeVersion;
}

public String getNodeExecutableOrigin() {
return nodeExecutableOrigin;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public class NodeCommandBuilderImpl implements NodeCommandBuilder {
private BundlePathResolver pathResolver;
private Version actualNodeVersion;
private Map<String, String> env = Map.of();
private String nodeExecutableOrigin = "none";

public NodeCommandBuilderImpl(ProcessWrapper processWrapper) {
this.processWrapper = processWrapper;
Expand Down Expand Up @@ -164,7 +165,8 @@ public NodeCommand build() throws NodeCommandException, IOException {
args,
outputConsumer,
errorConsumer,
env
env,
nodeExecutableOrigin
);
}

Expand Down Expand Up @@ -219,6 +221,7 @@ static Version nodeVersion(String versionString) throws NodeCommandException {
private String retrieveNodeExecutable(Configuration configuration)
throws NodeCommandException, IOException {
if (configuration.hasKey(NODE_EXECUTABLE_PROPERTY)) {
nodeExecutableOrigin = NODE_EXECUTABLE_PROPERTY;
String nodeExecutable = configuration.get(NODE_EXECUTABLE_PROPERTY).get();
File file = new File(nodeExecutable);
if (file.exists()) {
Expand Down Expand Up @@ -246,14 +249,18 @@ private String locateNode(boolean isForceHost) throws IOException {
if (embeddedNode.isAvailable() && !isForceHost) {
LOG.info("Using embedded Node.js runtime.");
defaultNode = embeddedNode.binary().toString();
nodeExecutableOrigin = "embedded";
} else if (processWrapper.isMac()) {
defaultNode = locateNodeOnMac();
nodeExecutableOrigin = "host";
} else if (processWrapper.isWindows()) {
defaultNode = locateNodeOnWindows();
nodeExecutableOrigin = "host";
}

if (isForceHost) {
LOG.info("Forcing to use Node.js from the host.");
nodeExecutableOrigin = "force-host";
}

LOG.info("Using Node.js executable: '{}'.", defaultNode);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ public static String getVersion(ProcessWrapper processWrapper, String nodeExecut
Map.of(
"RUN_NODE_ERROR_MSG",
"Couldn't find the Node.js binary. Ensure you have Node.js installed."
)
),
"host"
);
nodeCommand.start();
int exitValue = nodeCommand.waitFor();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
import org.sonar.plugins.javascript.bridge.BridgeServer.CssAnalysisRequest;
import org.sonar.plugins.javascript.bridge.BridgeServer.Dependency;
import org.sonar.plugins.javascript.bridge.BridgeServer.JsAnalysisRequest;
import org.sonar.plugins.javascript.bridge.BridgeServer.TelemetryResponse;
import org.sonar.plugins.javascript.bridge.BridgeServer.TsProgram;
import org.sonar.plugins.javascript.bridge.BridgeServer.TsProgramRequest;
import org.sonar.plugins.javascript.bridge.protobuf.Node;
Expand Down Expand Up @@ -759,9 +758,10 @@ void should_return_telemetry() throws Exception {
bridgeServer = createBridgeServer(START_SERVER_SCRIPT);
bridgeServer.startServer(serverConfig, emptyList());
var telemetry = bridgeServer.getTelemetry();
assertThat(telemetry).isEqualTo(
new TelemetryResponse(List.of(new Dependency("pkg1", "1.0.0")))
);
assertThat(telemetry.dependencies()).isEqualTo(List.of(new Dependency("pkg1", "1.0.0")));
var runtimeTelemetry = telemetry.runtimeTelemetry();
assertThat(runtimeTelemetry.version().isGreaterThanOrEqual(Version.create(22, 9))).isTrue();
assertThat(runtimeTelemetry.nodeExecutableOrigin()).isNotEmpty();
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ void test_executable_from_configuration() throws Exception {

List<String> value = captureProcessWrapperArgument();
assertThat(value).contains(nodeExecutable.toString());
assertThat(nodeCommand.getNodeExecutableOrigin()).isEqualTo(NODE_EXECUTABLE_PROPERTY);
await()
.until(() ->
logTester
Expand All @@ -213,6 +214,7 @@ void test_empty_configuration() throws Exception {

List<String> value = captureProcessWrapperArgument();
assertThat(value).contains("node");
assertThat(nodeCommand.getNodeExecutableOrigin()).isEqualTo("none");
}

private List<String> captureProcessWrapperArgument() throws IOException {
Expand Down Expand Up @@ -335,6 +337,7 @@ void test_command_on_mac() throws Exception {
.pathResolver(getPathResolver())
.build();
nodeCommand.start();
assertThat(nodeCommand.getNodeExecutableOrigin()).isEqualTo("host");
List<String> value = captureProcessWrapperArgument();
assertThat(value).hasSize(2);
assertThat(value.get(0)).endsWith("src/test/resources/package/bin/run-node");
Expand Down Expand Up @@ -366,7 +369,8 @@ void test_actual_node_version() throws Exception {
Collections.emptyList(),
noop,
noop,
Map.of()
Map.of(),
"host"
);
assertThat(nodeCommand.getActualNodeVersion().major()).isEqualTo(12);
}
Expand All @@ -390,6 +394,7 @@ void test_windows_default_node() throws Exception {
"C:\\Program Files\\node.exe",
"script.js"
);
assertThat(nodeCommand.getNodeExecutableOrigin()).isEqualTo("host");
}

@Test
Expand Down Expand Up @@ -420,6 +425,7 @@ void test_embedded_runtime() throws Exception {
// For some reason, using mockProcessWrapper to test for the used command does not yield the expected result
var expectedCommand = Paths.get(en.binary().toString()) + " " + PATH_TO_SCRIPT;
assertThat(nodeCommand.toString()).isEqualTo(expectedCommand);
assertThat(nodeCommand.getNodeExecutableOrigin()).isEqualTo("embedded");
}

@Test
Expand All @@ -443,6 +449,7 @@ void test_embedded_runtime_with_forceHost_for_macos() throws Exception {
.build();
var commandParts = nodeCommand.toString().split(" ");
assertThat(commandParts[0]).endsWith("src/test/resources/package/bin/run-node");
assertThat(nodeCommand.getNodeExecutableOrigin()).isEqualTo("force-host");
}

@Test
Expand All @@ -451,7 +458,7 @@ void test_skipping_nodejs_provisioning_property() throws Exception {
var settings = new MapSettings();
settings.setProperty(skipNodePropvisioning, true);

builder()
var nodeCommand = builder()
.configuration(settings.asConfig())
.script("script.js")
.pathResolver(getPathResolver())
Expand All @@ -460,6 +467,7 @@ void test_skipping_nodejs_provisioning_property() throws Exception {
assertThat(logTester.logs(Level.INFO))
.doesNotContain("Using embedded Node.js runtime")
.contains("Forcing to use Node.js from the host.");
assertThat(nodeCommand.getNodeExecutableOrigin()).isEqualTo("force-host");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,7 @@ void analyzeFiles(List<InputFile> inputFiles) throws IOException {
)
);
}
var telemetry = bridgeServer.getTelemetry();
new PluginTelemetry(context).reportTelemetry(telemetry);
new PluginTelemetry(context, bridgeServer).reportTelemetry();
Copy link
Contributor

Choose a reason for hiding this comment

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

optional - I would only pass value from bridgeServer.getTelemtry() instead of passing the whole bridgeServer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way, if telemetry is not available or not necessary to compute for some reason, we wouldn't be computing it.

} finally {
if (success) {
progressReport.stop();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,43 +16,58 @@
*/
package org.sonar.plugins.javascript.analysis;

import java.util.HashMap;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.sonar.api.batch.sensor.SensorContext;
import org.sonar.api.utils.Version;
import org.sonar.plugins.javascript.bridge.BridgeServer;
import org.sonar.plugins.javascript.bridge.BridgeServer.Dependency;
import org.sonar.plugins.javascript.bridge.BridgeServer.TelemetryResponse;

public class PluginTelemetry {

private static final Logger LOG = LoggerFactory.getLogger(PluginTelemetry.class);
private static final String KEY_PREFIX = "javascript.";
private static final String DEPENDENCY_PREFIX = KEY_PREFIX + "dependency.";
private static final String RUNTIME_PREFIX = KEY_PREFIX + "runtime.";

private final BridgeServer server;
private final SensorContext ctx;

public PluginTelemetry(SensorContext ctx) {
public PluginTelemetry(SensorContext ctx, BridgeServer server) {
this.ctx = ctx;
this.server = server;
}

void reportTelemetry(@Nullable TelemetryResponse telemetry) {
void reportTelemetry() {
var isTelemetrySupported = ctx
.runtime()
.getApiVersion()
.isGreaterThanOrEqual(Version.create(10, 9));
if (telemetry == null || !isTelemetrySupported) {
if (!isTelemetrySupported) {
// addTelemetryProperty is added in 10.9:
// https://github.com/SonarSource/sonar-plugin-api/releases/tag/10.9.0.2362
return;
}
var keyMapToSave = telemetry
.dependencies()
.stream()
.collect(
Collectors.toMap(dependency -> DEPENDENCY_PREFIX + dependency.name(), Dependency::version)
);
var telemetry = server.getTelemetry();
var keyMapToSave = new HashMap<>(
telemetry
.dependencies()
.stream()
.collect(
Collectors.toMap(dependency -> DEPENDENCY_PREFIX + dependency.name(), Dependency::version)
)
);
keyMapToSave.put(
RUNTIME_PREFIX + "node-executable-origin",
telemetry.runtimeTelemetry().nodeExecutableOrigin()
);
keyMapToSave.put(RUNTIME_PREFIX + "version", telemetry.runtimeTelemetry().version().toString());
keyMapToSave.put(
RUNTIME_PREFIX + "major-version",
Integer.toString(telemetry.runtimeTelemetry().version().major())
);
keyMapToSave.forEach(ctx::addTelemetryProperty);
LOG.debug("Telemetry saved: {}", keyMapToSave);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
import org.sonar.api.batch.rule.CheckFactory;
import org.sonar.api.batch.rule.internal.ActiveRulesBuilder;
import org.sonar.api.batch.rule.internal.NewActiveRule;
import org.sonar.api.batch.sensor.SensorContext;
import org.sonar.api.batch.sensor.cache.WriteCache;
import org.sonar.api.batch.sensor.highlighting.TypeOfText;
import org.sonar.api.batch.sensor.internal.DefaultSensorDescriptor;
Expand All @@ -82,7 +81,8 @@
import org.sonar.plugins.javascript.bridge.BridgeServer.AnalysisResponse;
import org.sonar.plugins.javascript.bridge.BridgeServer.Dependency;
import org.sonar.plugins.javascript.bridge.BridgeServer.JsAnalysisRequest;
import org.sonar.plugins.javascript.bridge.BridgeServer.TelemetryResponse;
import org.sonar.plugins.javascript.bridge.BridgeServer.RuntimeTelemetry;
import org.sonar.plugins.javascript.bridge.BridgeServer.TelemetryData;
import org.sonar.plugins.javascript.bridge.BridgeServer.TsProgram;
import org.sonar.plugins.javascript.bridge.EslintRule;
import org.sonar.plugins.javascript.bridge.PluginInfo;
Expand Down Expand Up @@ -148,6 +148,9 @@ public void setUp() throws Exception {
);
tsProgram = new TsProgram("", new ArrayList<>(), List.of());
when(bridgeServerMock.createProgram(any())).thenReturn(tsProgram);
when(bridgeServerMock.getTelemetry()).thenReturn(
new TelemetryData(List.of(), new RuntimeTelemetry(Version.create(22, 9), "host"))
);
context = SensorContextTester.create(baseDir);
context.fileSystem().setWorkDir(workDir);
context.setRuntime(
Expand Down Expand Up @@ -759,7 +762,10 @@ void log_debug_analyzed_filename() throws Exception {
void should_add_telemetry_for_scanner_analysis() throws Exception {
when(bridgeServerMock.analyzeJavaScript(any())).thenReturn(new AnalysisResponse());
when(bridgeServerMock.getTelemetry()).thenReturn(
new TelemetryResponse(List.of(new Dependency("pkg1", "1.1.0")))
new TelemetryData(
List.of(new Dependency("pkg1", "1.1.0")),
new RuntimeTelemetry(Version.create(22, 9), "embedded")
)
);
var sensor = createSensor();
context.setRuntime(
Expand All @@ -773,7 +779,7 @@ void should_add_telemetry_for_scanner_analysis() throws Exception {
createInputFile(context);
sensor.execute(context);
assertThat(logTester.logs(Level.DEBUG)).contains(
"Telemetry saved: {javascript.dependency.pkg1=1.1.0}"
"Telemetry saved: {javascript.runtime.node-executable-origin=embedded, javascript.runtime.major-version=22, javascript.dependency.pkg1=1.1.0, javascript.runtime.version=22.9}"
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,12 @@ public void setUp() throws Exception {
when(bridgeServerMock.isAlive()).thenReturn(true);
when(bridgeServerMock.analyzeTypeScript(any())).thenReturn(new AnalysisResponse());
when(bridgeServerMock.getCommandInfo()).thenReturn("bridgeServerMock command info");
when(bridgeServerMock.getTelemetry()).thenReturn(
new BridgeServer.TelemetryData(
List.of(),
new BridgeServer.RuntimeTelemetry(Version.create(22, 9), "host")
)
);
when(bridgeServerMock.loadTsConfig(any())).thenAnswer(invocationOnMock -> {
String tsConfigPath = (String) invocationOnMock.getArguments()[0];
FilePredicates predicates = context.fileSystem().predicates();
Expand Down
Loading
Loading