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-493 Add 'get-telemetry' endpoint on bridge-server #5011

Merged
merged 9 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from 8 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
6 changes: 5 additions & 1 deletion packages/bridge/src/handle-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/
import { analyzeCSS } from '../../css/src/analysis/analyzer.js';
import { analyzeHTML } from '../../html/src/index.js';
import { analyzeJSTS } from '../../jsts/src/analysis/analyzer.js';
import { analyzeJSTS, getTelemetry } from '../../jsts/src/analysis/analyzer.js';
import { analyzeProject } from '../../jsts/src/analysis/projectAnalysis/projectAnalyzer.js';
import { analyzeYAML } from '../../yaml/src/index.js';
import { logHeapStatistics } from './memory.js';
Expand Down Expand Up @@ -107,6 +107,10 @@ export async function handleRequest(request: BridgeRequest): Promise<RequestResu
const output = await analyzeProject(request.data);
return { type: 'success', result: output };
}
case 'on-get-telemetry': {
const output = getTelemetry();
return { type: 'success', result: output };
}
}
} catch (err) {
return { type: 'failure', error: serializeError(err) };
Expand Down
13 changes: 11 additions & 2 deletions packages/bridge/src/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,22 @@ import { TsConfigJson } from 'type-fest';
import { RuleConfig } from '../../jsts/src/linter/config/rule-config.js';
import { readFile } from '../../shared/src/helpers/files.js';
import { APIError, ErrorCode } from '../../shared/src/errors/error.js';
import { NamedDependency } from '../../jsts/src/rules/index.js';

export type RequestResult =
| {
type: 'success';
result: string | AnalysisOutput;
result: string | AnalysisOutput | Telemetry;
}
| {
type: 'failure';
error: ReturnType<typeof serializeError>;
};

export type Telemetry = {
dependencies: NamedDependency[];
};

export type RequestType = BridgeRequest['type'];

type MaybeIncompleteCssAnalysisInput = Omit<CssAnalysisInput, 'fileContent'> & {
Expand Down Expand Up @@ -61,7 +66,8 @@ export type BridgeRequest =
| DeleteProgramRequest
| InitLinterRequest
| NewTsConfigRequest
| TsConfigFilesRequest;
| TsConfigFilesRequest
| GetTelemetryRequest;

type CssRequest = {
type: 'on-analyze-css';
Expand Down Expand Up @@ -115,6 +121,9 @@ type TsConfigFilesRequest = {
type: 'on-tsconfig-files';
data: { tsConfig: string };
};
type GetTelemetryRequest = {
type: 'on-get-telemetry';
};

/**
* In SonarQube context, an analysis input includes both path and content of a file
Expand Down
1 change: 1 addition & 0 deletions packages/bridge/src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export default function (worker?: Worker): express.Router {
router.post('/init-linter', delegate('on-init-linter'));
router.post('/new-tsconfig', delegate('on-new-tsconfig'));
router.post('/tsconfig-files', delegate('on-tsconfig-files'));
router.get('/get-telemetry', delegate('on-get-telemetry'));

/** Endpoints running on the main thread */
router.get('/status', (_, response) => response.send('OK!'));
Expand Down
6 changes: 6 additions & 0 deletions packages/bridge/tests/router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,12 @@ describe('router', () => {
expect(json.filename).toBeTruthy();
expect(fs.existsSync(json.filename)).toBe(true);
});

it('should return empty get-telemetry on fresh server', async () => {
const response = (await request(server, '/get-telemetry', 'GET')) as string;
const json = JSON.parse(response);
expect(json).toEqual({ dependencies: [] });
});
});

function requestInitLinter(server: http.Server, rules: RuleConfig[]) {
Expand Down
9 changes: 8 additions & 1 deletion packages/jsts/src/analysis/analyzer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ import { getContext } from '../../../shared/src/helpers/context.js';
import { computeMetrics, findNoSonarLines } from '../linter/visitors/metrics/index.js';
import { getSyntaxHighlighting } from '../linter/visitors/syntax-highlighting.js';
import { getCpdTokens } from '../linter/visitors/cpd.js';
import { clearDependenciesCache } from '../rules/helpers/package-json.js';
import { clearDependenciesCache, getAllDependencies } from '../rules/index.js';
import { Telemetry } from '../../../bridge/src/request.js';

/**
* Analyzes a JavaScript / TypeScript analysis input
Expand Down Expand Up @@ -160,3 +161,9 @@ function computeExtendedMetrics(
};
}
}

export function getTelemetry(): Telemetry {
return {
dependencies: getAllDependencies(),
};
}
68 changes: 51 additions & 17 deletions packages/jsts/src/rules/helpers/package-json.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,37 @@ const findPackageJsons = createFindUp(PACKAGE_JSON);

const DefinitelyTyped = '@types/';

type MinimatchDependency = {
name: Minimatch;
version?: string;
};

export type NamedDependency = {
name: string;
version?: string;
};

type Dependency = MinimatchDependency | NamedDependency;

/**
* Cache for the available dependencies by dirname.
*/
const cache: Map<string, Set<string | Minimatch>> = new Map();
const cache: Map<string, Set<Dependency>> = new Map();

export function getAllDependencies(): NamedDependency[] {
const dependencies = [...cache.values()]
.flatMap(dependencies => [...dependencies])
Copy link
Contributor

Choose a reason for hiding this comment

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

if we have duplicates, would it simply overwrite and keep the last one?

.filter((dependency): dependency is NamedDependency => typeof dependency.name === 'string');
return Object.values(
dependencies.reduce(
(result, dependency) => ({
...result,
[dependency.name]: dependency,
}),
{},
),
);
Comment on lines +59 to +67
Copy link
Contributor

@kebetsi kebetsi Dec 12, 2024

Choose a reason for hiding this comment

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

what is this step doing? Since we already have a NemdDependency[] at the previous step.

}

/**
* Retrieve the dependencies of all the package.json files available for the given file.
Expand All @@ -46,9 +73,9 @@ export function getDependencies(filename: string, cwd: string) {
const dirname = Path.dirname(toUnixPath(filename));
const cached = cache.get(dirname);
if (cached) {
return cached;
return new Set([...cached].map(item => item.name));
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this doing? Just a clone or something more?

}
const result = new Set<string | Minimatch>();
const result = new Set<Dependency>();
cache.set(dirname, result);

getManifests(dirname, cwd, fs).forEach(manifest => {
Expand All @@ -59,7 +86,7 @@ export function getDependencies(filename: string, cwd: string) {
});
});

return result;
return new Set([...result].map(item => item.name));
}

/**
Expand All @@ -71,7 +98,7 @@ export function clearDependenciesCache() {
}

export function getDependenciesFromPackageJson(content: PackageJson) {
const result = new Set<string | Minimatch>();
const result = new Set<Dependency>();
if (content.name) {
addDependencies(result, { [content.name]: '*' });
}
Expand Down Expand Up @@ -99,30 +126,37 @@ export function getDependenciesFromPackageJson(content: PackageJson) {
}

function addDependencies(
result: Set<string | Minimatch>,
result: Set<Dependency>,
dependencies: PackageJson.Dependency,
isGlob = false,
) {
Object.keys(dependencies).forEach(name => addDependency(result, name, isGlob));
Object.keys(dependencies).forEach(name =>
addDependency(result, name, isGlob, dependencies[name]),
);
}

function addDependenciesArray(
result: Set<string | Minimatch>,
dependencies: string[],
isGlob = true,
) {
function addDependenciesArray(result: Set<Dependency>, dependencies: string[], isGlob = true) {
dependencies.forEach(name => addDependency(result, name, isGlob));
}

function addDependency(result: Set<string | Minimatch>, dependency: string, isGlob: boolean) {
function addDependency(
result: Set<Dependency>,
dependency: string,
isGlob: boolean,
version?: string,
) {
if (isGlob) {
result.add(new Minimatch(dependency, { nocase: true, matchBase: true }));
result.add({
name: new Minimatch(dependency, { nocase: true, matchBase: true }),
version,
});
} else {
result.add(
dependency.startsWith(DefinitelyTyped)
result.add({
name: dependency.startsWith(DefinitelyTyped)
? dependency.substring(DefinitelyTyped.length)
: dependency,
);
version,
});
}
}

Expand Down
47 changes: 45 additions & 2 deletions packages/jsts/tests/analysis/analyzer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ import { jsTsInput, parseJavaScriptSourceFile } from '../tools/index.js';
import { Linter, Rule } from 'eslint';
import { describe, beforeEach, it } from 'node:test';
import { expect } from 'expect';
import { getManifests, toUnixPath } from '../../src/rules/helpers/index.js';
import { getDependencies, getManifests, toUnixPath } from '../../src/rules/helpers/index.js';
import { setContext } from '../../../shared/src/helpers/context.js';
import { analyzeJSTS } from '../../src/analysis/analyzer.js';
import { analyzeJSTS, getTelemetry } from '../../src/analysis/analyzer.js';
import { APIError } from '../../../shared/src/errors/error.js';
import { RuleConfig } from '../../src/linter/config/rule-config.js';
import { initializeLinter } from '../../src/linter/linters.js';
Expand Down Expand Up @@ -899,6 +899,49 @@ describe('analyzeJSTS', () => {
expect(vueIssues[0].message).toEqual('call');
});

it('should populate dependencies after analysis', async () => {
const baseDir = path.join(currentPath, 'fixtures', 'dependencies');
const linter = new Linter();
linter.defineRule('custom-rule-file', {
create(context) {
return {
CallExpression(node) {
// Necessarily call 'getDependencies' to populate the cache of dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that we would not receive any dependencies in the telemetry in case the analysis never populated the cache?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a good chance that this happens?

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, we will only have dependencies once the analysis populates it. Chance is low. I guess one would have to remove all of the rules that call the package-jsons.ts cache. In that case, we wouldn't populate anything for them. That's a tradeoff. This way, this call is very cheap and doesn't incur an extra time-cost.

Copy link
Contributor

Choose a reason for hiding this comment

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

and could you document this behavior somewhere, preferably in the code base like in the JS doc of the getDependencies() function:

Returns the dependencies of the root package.json file collected in the cache.

As the cache is populated lazily, it could be null in case no rule execution has touched it.

const dependencies = getDependencies(toUnixPath(context.filename), baseDir);
if (dependencies.size) {
context.report({
node: node.callee,
message: 'call',
});
}
},
};
},
} as Rule.RuleModule);
const filePath = path.join(currentPath, 'fixtures', 'dependencies', 'index.js');
const sourceCode = await parseJavaScriptSourceFile(filePath);
linter.verify(
sourceCode,
{ rules: { 'custom-rule-file': 'error' } },
{ filename: filePath, allowInlineConfig: false },
);
const { dependencies } = getTelemetry();
expect(dependencies).toStrictEqual([
{
name: 'test-module',
version: '*',
},
{
name: 'pkg1',
version: '1.0.0',
},
{
name: 'pkg2',
version: '2.0.0',
},
]);
});

it('should return the AST along with the issues', async () => {
const rules = [{ key: 'S4524', configurations: [], fileTypeTarget: ['MAIN'] }] as RuleConfig[];
await initializeLinter(rules);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log("Hello World!");
11 changes: 11 additions & 0 deletions packages/jsts/tests/analysis/fixtures/dependencies/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "test-module",
"version": "1.0.0",
"author": "Your Name <email@example.com>",
"dependencies": {
"pkg1": "1.0.0"
},
"devDependencies": {
"pkg2": "2.0.0"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ void initLinter(

TsConfigFile createTsConfigFile(String content) throws IOException;

TelemetryResponse getTelemetry();

record JsAnalysisRequest(
String filePath,
String fileType,
Expand Down Expand Up @@ -275,4 +277,8 @@ public String toString() {
}

record TsProgramRequest(String tsConfig) {}

record TelemetryResponse(List<Dependency> dependencies) {}

record Dependency(String name, String version) {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,16 @@ public TsConfigFile createTsConfigFile(String content) {
return GSON.fromJson(response.json(), TsConfigFile.class);
}

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

private static <T> List<T> emptyListIfNull(@Nullable List<T> list) {
return list == null ? emptyList() : list;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@
import org.sonar.api.utils.TempFolder;
import org.sonar.api.utils.Version;
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 @@ -752,6 +754,16 @@ void test_ucfg_bundle_version() throws Exception {
);
}

@Test
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")))
);
}

@Test
void should_return_an_ast() throws Exception {
bridgeServer = createBridgeServer(START_SERVER_SCRIPT);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ const requestHandler = (request, response) => {
loc: {}}]}]}],
highlights: [{location: {startLine: 0, startColumn: 0, endLine: 0, endColumn: 0}}],
metrics: {}, highlightedSymbols: [{}], cpdTokens: [{}] }`);
} else if (request.url === '/get-telemetry') {
response.end('{"dependencies": [{"name": "pkg1", "version": "1.0.0"}]}');
} else {
// /analyze-with-program
// /analyze-js
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.sonar.plugins.javascript.analysis.HtmlSensor;
import org.sonar.plugins.javascript.analysis.JsTsChecks;
import org.sonar.plugins.javascript.analysis.JsTsSensor;
import org.sonar.plugins.javascript.analysis.PluginTelemetry;
import org.sonar.plugins.javascript.analysis.TsConfigProvider;
import org.sonar.plugins.javascript.analysis.YamlSensor;
import org.sonar.plugins.javascript.bridge.AnalysisWarningsWrapper;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ void analyzeFiles(List<InputFile> inputFiles) throws IOException {
)
);
}
var telemetry = bridgeServer.getTelemetry();
new PluginTelemetry(context).reportTelemetry(telemetry);
} finally {
if (success) {
progressReport.stop();
Expand Down
Loading
Loading