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
Changes from all commits
2ca561a
6bd8928
cb08be1
e30dd27
104ad79
66ed849
cd6b10f
41fb8b8
0e92fe0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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, | ||
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(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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()); | ||
|
@@ -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 | ||
|
@@ -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, | ||
}); | ||
} | ||
} | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added that configurable patterns into PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @spuliaiev-sfdc I am talking about this extension in the Bazel LS extension: https://github.com/salesforce/bazel-eclipse/blob/main/bundles/com.salesforce.bazel.eclipse.core/src/com/salesforce/bazel/eclipse/core/model/SynchronizationParticipant.java That's much more stable for telemetry. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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 | ||
|
@@ -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); | ||
} | ||
}); | ||
|
@@ -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'); | ||
|
@@ -84,6 +91,8 @@ export async function activate(context: ExtensionContext) { | |
openBazelProjectFile() | ||
) | ||
); | ||
} else { | ||
LOGGER.debug('Bazel Workspace Root not detected.'); | ||
} | ||
|
||
context.subscriptions.push( | ||
|
@@ -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' | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we are not doing telemetry here. Just triggering events. |
||
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') | ||
|
@@ -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}` | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') { | ||
|
@@ -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}` | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -322,9 +361,12 @@ async function syncProjectViewDirectories() { | |
); | ||
} | ||
}); | ||
apiHandler.fireSyncDirectoriesEnded(bazelProjectFile.directories); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no duration - it is calculated in a telemetry extension. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} catch (err) { | ||
throw new Error(`Could not read bazelproject file: ${err}`); | ||
} | ||
} else { | ||
LOGGER.warn('Workspace root is not defined for Directories sync.'); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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'); | ||
|
@@ -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; | ||
|
||
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
); | ||
|
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
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