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

@W-14955790 BazelEventsExtensionAPI #108

Closed
wants to merge 9 commits into from
71 changes: 71 additions & 0 deletions src/apiHandler.ts
@@ -0,0 +1,71 @@
import { Emitter } from 'vscode-languageclient';
import {
BazelEventsExtensionAPI,
TerminalLogEvent,
TerminalLogPattern,
} from './extension.api';
import { LOGGER } from './util';

class ApiHandler {
private api!: BazelEventsExtensionAPI;
private onSyncStartedEmitter: Emitter<string> = new Emitter<string>();
private onBazelTerminalLogEmitter: Emitter<TerminalLogEvent> =
new Emitter<TerminalLogEvent>();
private onSyncDirectoriesStartedEmitter: Emitter<string[]> = new Emitter<
string[]
>();
private onSyncDirectoriesEndedEmitter: Emitter<string[]> = new Emitter<
string[]
>();

public bazelTerminalLogListeners: Map<string, TerminalLogPattern> = new Map();

public initApi() {
const onSyncStarted = this.onSyncStartedEmitter.event;
const onBazelTerminalLog = this.onBazelTerminalLogEmitter.event;
const onSyncDirectoriesStarted = this.onSyncDirectoriesStartedEmitter.event;
const onSyncDirectoriesEnded = this.onSyncDirectoriesEndedEmitter.event;
const appendBazelTerminalLogPattern = this.appendBazelTerminalLogPattern;

this.api = {
onSyncStarted,
onBazelTerminalLog,
onSyncDirectoriesStarted,
onSyncDirectoriesEnded,
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a lot of APIs. Can it be consolidated into less? I don't see why we need separate calls for start and end. We can submit just the result.

    /**
     * Event emitted when a sync operations completed.
     * @param syncMode - mode of synchronization (full, directories)
     * @param status - ok or cancelled or failed
     * @param workspacePath - the Bazel workspace that was synchronized
     * @param durationInMilliseconds - the duration of the synchronization in milliseconds
     */
    public onSyncFinished(syncMode: enum, status: string, workspacePath: string, durationInMilliseconds: number);

Also, it's a bit confusing why this is API. Would we expose/export a way to add event listeners and these listeners need to implement an interface with that method?

Copy link
Author

Choose a reason for hiding this comment

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

Added an interface to add event listeners for the BazelTerminalLog - via patterns.
Removed onSyncEnded - because it is based on the log - so it will be configured from telemetry extension

appendBazelTerminalLogPattern,
};
}

private appendBazelTerminalLogPattern(pattern: TerminalLogPattern): boolean {
LOGGER.debug(
`appendBazelTerminalLogPattern ${pattern.name} RegExp:${pattern.pattern}`
);
this.bazelTerminalLogListeners.set(pattern.name, pattern);
return true;
}

public fireSyncStarted(event: string) {
this.onSyncStartedEmitter.fire(event);
}

public fireBazelTerminalLog(event: TerminalLogEvent) {
this.onBazelTerminalLogEmitter.fire(event);
}

public fireSyncDirectoriesStarted(event: string[]) {
this.onSyncDirectoriesStartedEmitter.fire(event);
}

public fireSyncDirectoriesEnded(event: string[]) {
this.onSyncDirectoriesEndedEmitter.fire(event);
}

public getApi(): BazelEventsExtensionAPI {
if (!this.api) {
throw new Error(`ApiHandler is not initialized`);
}
return this.api;
}
}

export const apiHandler = new ApiHandler();
29 changes: 29 additions & 0 deletions src/bazelLangaugeServerTerminal.ts
@@ -1,14 +1,19 @@
import { Writable } from 'stream';
import { Terminal, window, workspace } from 'vscode';
import { apiHandler } from './apiHandler';
import { BazelTerminal } from './bazelTerminal';
import { TerminalLogPattern } from './extension.api';
import { LOGGER, getWorkspaceRoot } from './util';

const BAZEL_TERMINAL_NAME = 'Bazel Build Status';
const workspaceRoot = getWorkspaceRoot();

export namespace BazelLanguageServerTerminal {
export function stream(): Writable {
const s = new Writable();
s._write = (chunk: Buffer, encoding, next) => {
getBazelTerminal().sendText(chunk.toString());
catchBazelLog(chunk.toString());
next();
};
s.on('unpipe', () => s.end());
Expand Down Expand Up @@ -40,6 +45,7 @@ export namespace BazelLanguageServerTerminal {
} // gray
}

// Catch some bazel log messages to trigger API events
export function getBazelTerminal(): Terminal {
const term = window.terminals.find(
(term) => term.name === BAZEL_TERMINAL_NAME
Expand Down Expand Up @@ -75,3 +81,26 @@ function getLogLevel(): LogLevel {
return LogLevel.INFO;
}
}

function catchBazelLog(chunk: string) {
// apiHandler.bazelTerminalLogListeners['BazelTest']={ name: 'BazelTest', pattern: RegExp(/100.0%\s+Synchronizing\s+core\s+(\S+)\n/), sendFullMessage: true}
apiHandler.bazelTerminalLogListeners.forEach(
(logPattern: TerminalLogPattern, name: string) => {
const patternToMatch = logPattern.pattern;
const patternMatched = patternToMatch.exec(chunk);
if (patternMatched) {
LOGGER.debug(`catchBazelLog ${name}`);
let textPiece = patternMatched[0];
if (logPattern.sendFullMessage) {
textPiece = chunk;
}
apiHandler.fireBazelTerminalLog({
name: name,
pattern: patternToMatch,
fullMessage: textPiece,
workspaceRoot: workspaceRoot,
});
}
}
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a brittle contract. We should not be doing this. Instead we should hook into the Java side and emit the event reliably. API already exists

Copy link
Author

Choose a reason for hiding this comment

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

If you mean the LanguageClient api - this api client is created in RedHat Java - and not visible for all the other extensions.
Multiple connections to the same LanguageServer is still not supported unfortunately, and the client is not exposed in the redhat extension.
microsoft/language-server-protocol#1359
microsoft/language-server-protocol#1160
microsoft/vscode-languageserver-node#837

Copy link
Author

Choose a reason for hiding this comment

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

We can adjust the RedHat java extension, but that would not be easy.

Copy link
Author

Choose a reason for hiding this comment

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

@lonhutt suggested to hook on the terminal and catch log messages to identify events.
That's what is done now.
It might be more configurable - to just hold the list of patterns and trigger events basing on matches.
So we can configure them from the interested extensions.

Copy link
Author

Choose a reason for hiding this comment

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

Added that configurable patterns into PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

50 changes: 50 additions & 0 deletions src/extension.api.ts
@@ -0,0 +1,50 @@
import * as vscode from 'vscode';

export interface TerminalLogEvent {
name: string;
pattern: RegExp;
fullMessage: string;
workspaceRoot: string;
}

export interface TimeEvent {
workspaceRoot: string;
// Duration in seconds
duration: number;
}

export interface TerminalLogPattern {
name: string;
pattern: RegExp;
sendFullMessage: boolean;
}

export type AppendBazelTerminalLogPattern = (
pattern: TerminalLogPattern
) => boolean;

export interface BazelEventsExtensionAPI {
/* An event which is fired on start of a Bazel Sync session.
* The string points to path of the Workspace root.
*/
readonly onSyncStarted: vscode.Event<string>;
/* An event which is fired on start of a Bazel Sync Directories session.
* The list of strings stores the list of directories in .bazelproject
* that are being synced.
*/
readonly onSyncDirectoriesStarted: vscode.Event<string[]>;
/* An event which is fired on end of a Bazel Sync Directories session.
* The list of strings stores the list of directories in .bazelproject
* that are being synced.
*/
readonly onSyncDirectoriesEnded: vscode.Event<string[]>;
/* An event which is fired when BazelTerminal listener catches a message which
* matches a registered pattern.
* The string points to path of the Workspace root.
*/
readonly onBazelTerminalLog: vscode.Event<TerminalLogEvent>;
/* A method do register a patter to be matched agains BazelTerminalLog to trigger
* the event onBazelTerminalLog
*/
readonly appendBazelTerminalLogPattern: AppendBazelTerminalLogPattern;
}
46 changes: 44 additions & 2 deletions src/extension.ts
Expand Up @@ -13,18 +13,21 @@ import {
window,
workspace,
} from 'vscode';
import { apiHandler } from './apiHandler';
import {
BazelLanguageServerTerminal,
getBazelTerminal,
} from './bazelLangaugeServerTerminal';
import { BazelTaskManager } from './bazelTaskManager';
import { getBazelProjectFile } from './bazelprojectparser';
import { Commands, executeJavaLanguageServerCommand } from './commands';
import { BazelEventsExtensionAPI } from './extension.api';
import { registerLSClient } from './loggingTCPServer';
import { BazelRunTargetProvider } from './provider/bazelRunTargetProvider';
import { BazelTaskProvider } from './provider/bazelTaskProvider';
import { ExcludeConfig, FileWatcherExcludeConfig } from './types';
import {
LOGGER,
getWorkspaceRoot,
initBazelProjectFile,
isBazelWorkspaceRoot,
Expand All @@ -42,7 +45,7 @@ export async function activate(context: ExtensionContext) {
// fetch all projects loaded into LS and display those too
// show both .vscode and .eclipse folders
//

LOGGER.debug('Extension bazel-vscode-java is being activated');
window.registerTreeDataProvider(
'bazelTaskOutline',
BazelRunTargetProvider.instance
Expand All @@ -53,6 +56,9 @@ export async function activate(context: ExtensionContext) {

workspace.onDidSaveTextDocument((doc) => {
if (doc.fileName.includes('bazelproject')) {
LOGGER.debug(
`File with name including bazelproject has been changed. ${doc.fileName}`
);
toggleBazelProjectSyncStatus(doc);
}
});
Expand All @@ -71,6 +77,7 @@ export async function activate(context: ExtensionContext) {
);
// create .eclipse/.bazelproject file is DNE
if (isBazelWorkspaceRoot()) {
LOGGER.debug(`Bazel Workspace Root detected. ${getWorkspaceRoot()}`);
initBazelProjectFile();
const showBazelprojectConfig =
workspace.getConfiguration('bazel.projectview');
Expand All @@ -84,6 +91,8 @@ export async function activate(context: ExtensionContext) {
openBazelProjectFile()
)
);
} else {
LOGGER.debug('Bazel Workspace Root not detected.');
}

context.subscriptions.push(
Expand Down Expand Up @@ -126,22 +135,33 @@ export async function activate(context: ExtensionContext) {

// always update the project view after the initial project load
registerLSClient();

LOGGER.debug('Activation is complete.');

return new Promise<BazelEventsExtensionAPI>((resolve, reject) => {
apiHandler.initApi();
resolve(apiHandler.getApi());
});
}

export function deactivate() {}

function syncProjectView(): void {
if (!isRedhatJavaReady()) {
LOGGER.warn('RedHat Java is not ready for Bazel Project Sync.');
window.showErrorMessage(
'Unable to sync project view. Java language server is not ready'
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the value of duplicate messages?

return;
}
LOGGER.debug('RedHat Java is ready. Starting Bazel Project Sync.');

apiHandler.fireSyncStarted(workspaceRoot);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the value of this creation event? Why not start measuring here?

Copy link
Author

Choose a reason for hiding this comment

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

Because we are not doing telemetry here. Just triggering events.
measuring and creation of spans is done in a separate extensions.

const launchMode = workspace
.getConfiguration('java.server')
.get('launchMode');
// if the launchMode is not Standard it should be changed and the window reloaded to apply that change
LOGGER.debug(`Java server launch mode is ${launchMode} .`);
if (!launchMode || launchMode !== 'Standard') {
workspace
.getConfiguration('java.server')
Expand Down Expand Up @@ -194,13 +214,21 @@ function isRedhatJavaReady(): boolean {
}

function toggleBazelProjectSyncStatus(doc: TextDocument) {
if (workspace.getConfiguration('bazel.projectview').get('notification')) {
const showNotification = workspace
.getConfiguration('bazel.projectview')
.get('notification');
LOGGER.debug(
`toggleBazelProjectSyncStatus: Is notification to be shown ${showNotification}`
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

debug instead of info

if (showNotification) {
LOGGER.debug(`toggleBazelProjectSyncStatus: Notification is required.`);
window
.showWarningMessage(
`The Bazel Project View changed. Do you want to synchronize? [details](https://github.com/salesforce/bazel-eclipse/blob/main/docs/common/projectviews.md#project-views)`,
...['Java Projects', 'Only Directories', 'Do Nothing']
)
.then((val) => {
LOGGER.debug(`toggleBazelProjectSyncStatus: Sync requested for ${val}`);
if (val === 'Java Projects') {
syncProjectView();
} else if (val === 'Only Directories') {
Expand All @@ -209,21 +237,32 @@ function toggleBazelProjectSyncStatus(doc: TextDocument) {
workspace
.getConfiguration('bazel.projectview')
.update('notification', false);
} else {
throw new Error(
`toggleBazelProjectSyncStatus: Unexpected value returned from showWarningMessage ${val}`
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

that should raise an error, a log message is useless and requires extra support work to investigat/handle/diagnose

}
});
} else {
LOGGER.debug(`toggleBazelProjectSyncStatus: Notification is not required.`);
}
}

async function syncProjectViewDirectories() {
if (workspaceRoot) {
LOGGER.debug(
'Workspace root is defined for Directories sync. Syncing bazel project view.'
);
BazelLanguageServerTerminal.debug('Syncing bazel project view');
const displayFolders = new Set<string>(['.eclipse', '.vscode']); // TODO bubble this out to a setting
try {
const bazelProjectFile = await getBazelProjectFile();
let viewAll = false;
if (bazelProjectFile.directories.includes('.')) {
viewAll = true;
apiHandler.fireSyncDirectoriesStarted([]);
} else {
apiHandler.fireSyncDirectoriesStarted(bazelProjectFile.directories);
bazelProjectFile.directories.forEach((d) => {
const dirRoot = d.split('/').filter((x) => x)[0];
displayFolders.add(dirRoot);
Expand Down Expand Up @@ -322,9 +361,12 @@ async function syncProjectViewDirectories() {
);
}
});
apiHandler.fireSyncDirectoriesEnded(bazelProjectFile.directories);
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to be adapted to api including workspace path, event type, result message and duration

Copy link
Author

Choose a reason for hiding this comment

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

There is no duration - it is calculated in a telemetry extension.
No event type - as we do not have onSyncEnded now - so there is no sense to create a typed event.
The only thing left is workspace path - so it's only that provided as parameter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's brittle. What if there is a crash or something else and events are received/fired out of order?

This API is not good. We should measure duration here and then submit one single event, similar to how the Bazel LS extension does it.

https://github.com/salesforce/bazel-eclipse/blob/main/bundles/com.salesforce.bazel.eclipse.core/src/com/salesforce/bazel/eclipse/core/events/SyncFinishedEvent.java

} catch (err) {
throw new Error(`Could not read bazelproject file: ${err}`);
}
} else {
LOGGER.warn('Workspace root is not defined for Directories sync.');
}
}

Expand Down
9 changes: 5 additions & 4 deletions src/loggingTCPServer.ts
Expand Up @@ -3,6 +3,7 @@ import { setTimeout } from 'timers/promises';
import { commands, workspace } from 'vscode';
import { BazelLanguageServerTerminal } from './bazelLangaugeServerTerminal';
import { Commands } from './commands';
import { LOGGER } from './util';

const SERVER_START_RETRIES = 10;
const PORT_REGISTRATION_RETRIES = 10;
Expand All @@ -13,6 +14,7 @@ let server: Server | undefined;
function startTCPServer(attempts = 0): Promise<number> {
let port = 0;
if (workspace.getConfiguration('java').has('jdt.ls.vmargs')) {
LOGGER.debug('Adjusting vmargs for TCP Server.');
const vmargs = workspace
.getConfiguration('java')
.get<string>('jdt.ls.vmargs');
Expand All @@ -28,6 +30,7 @@ function startTCPServer(attempts = 0): Promise<number> {

return new Promise((resolve) => {
if (!server) {
LOGGER.debug('Starting TCP Server.');
server = createServer((sock: Socket) => {
attempts = 0;

Expand All @@ -47,13 +50,11 @@ function startTCPServer(attempts = 0): Promise<number> {
const address = server.address();
if (address) {
const port = (address as AddressInfo).port;
BazelLanguageServerTerminal.debug(
`Bazel log server listening on port ${port}`
);
LOGGER.debug(`Bazel log server listening on port ${port}`);
resolve(port);
}
} else {
BazelLanguageServerTerminal.error(`Failed to start bazel TCP server`);
LOGGER.info('Failed to start bazel TCP server');

Choose a reason for hiding this comment

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

do we need port here?

setTimeout<number>(1000 * attempts).then(() =>
startTCPServer(attempts + 1)
);
Expand Down
7 changes: 6 additions & 1 deletion src/util.ts
@@ -1,6 +1,7 @@
import { existsSync, mkdirSync, writeFileSync } from 'fs';
import { dirname, join } from 'path';
import { workspace } from 'vscode';
import { window, workspace } from 'vscode';
import { apiHandler } from './apiHandler';

// TODO: pull this template out into a file
const BAZELPROJECT_TEMPLATE = `
Expand All @@ -17,6 +18,10 @@ directories:
derive_targets_from_directories: true
`;

export const LOGGER = window.createOutputChannel('bazel-vscode-java', {
log: true,
});

export function getWorkspaceRoot(): string {
if (workspace.workspaceFile) {
return dirname(workspace.workspaceFile.path);
Expand Down