@W-14955790 BazelEventsExtensionAPI #108

spuliaiev-sfdc · 2024-03-04T21:31:09Z

Adding BazelEventsExtensionAPI:
- onSyncStarted event - An event which is fired on start of a Bazel Sync session.
- onSyncEnded event - An event which is fired on end of a Bazel Sync session.
- onBazelProjectFileCreated - An event which is fired on creation of a .bazelproject file.
- onBazelProjectFileUpdated - An event which is fired on updates to the .bazelproject file.
- onSyncDirectoriesStarted - An event which is fired on start of a Bazel Sync Directories session.
- onSyncDirectoriesEnded - An event which is fired on end of a Bazel Sync Directories session.
Adding some logging to track the extension activities, including extension activation

Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

…into spuliaiev/api Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

github-actions · 2024-03-04T21:37:48Z

Test Results

2 files ±0 2 suites ±0 1d 17h 7m 23s ⏱️ + 23m 24s
7 tests ±0 7 ✅ ±0 0 💤 ±0 0 ❌ ±0
14 runs ±0 14 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 0e92fe0. ± Comparison against base commit cff08e9.

♻️ This comment has been updated with latest results.

guw · 2024-03-05T05:43:20Z

src/apiHandler.ts

+			onBazelProjectFileCreated,
+			onBazelProjectFileUpdated,
+			onSyncDirectoriesStarted,
+			onSyncDirectoriesEnded,


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?

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

guw · 2024-03-05T05:43:55Z

src/loggingTCPServer.ts

@@ -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.info('Adjusting vmargs for TCP Server.');


debug not info

guw · 2024-03-05T05:44:07Z

src/loggingTCPServer.ts

@@ -28,6 +30,7 @@ function startTCPServer(attempts = 0): Promise<number> {

 	return new Promise((resolve) => {
 		if (!server) {
+			LOGGER.info('Starting TCP Server.');


debug not info?

guw · 2024-03-05T05:44:20Z

src/loggingTCPServer.ts

@@ -50,10 +53,12 @@ function startTCPServer(attempts = 0): Promise<number> {
 					BazelLanguageServerTerminal.debug(
 						`Bazel log server listening on port ${port}`
 					);
+					LOGGER.info(`Bazel log server listening on port ${port}`);


debug not info?

guw · 2024-03-05T05:44:48Z

src/loggingTCPServer.ts

 					resolve(port);
 				}
 			} else {
-				BazelLanguageServerTerminal.error(`Failed to start bazel TCP server`);
+				LOGGER.info('Failed to start bazel TCP server');
+				BazelLanguageServerTerminal.error('Failed to start bazel TCP server');


Why log twice? I would expect the error be surfaced alrady?

guw · 2024-03-05T05:45:41Z

src/util.ts

@@ -41,6 +46,9 @@ export function initBazelProjectFile() {
 			join(workspaceRoot, '.eclipse', '.bazelproject'),
 			BAZELPROJECT_TEMPLATE
 		);
+		apiHandler.fireBazelProjectFileCreated(
+			join(workspaceRoot, '.eclipse', '.bazelproject')
+		);


What's the value of this creation event? Why would someone interested in this not just register regular file watches?

guw · 2024-03-05T05:46:02Z

src/extension.ts

@@ -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.info('Extension bazel-vscode-java is being activated');


debug instead of info?

guw · 2024-03-05T05:46:23Z

src/extension.ts

 			toggleBazelProjectSyncStatus(doc);
+			apiHandler.fireBazelProjectFileUpdated(doc.uri);


What's the value of this creation event? Why would someone interested in this not just register regular file watch?

guw · 2024-03-05T05:46:52Z

src/extension.ts

@@ -71,6 +78,7 @@ export async function activate(context: ExtensionContext) {
 	);
 	// create .eclipse/.bazelproject file is DNE
 	if (isBazelWorkspaceRoot()) {
+		LOGGER.info('Bazel Workspace Root detected.');


debug instead of info; also, additional details would be useful (location)

guw · 2024-03-05T05:47:08Z

src/extension.ts

@@ -84,6 +92,8 @@ export async function activate(context: ExtensionContext) {
 				openBazelProjectFile()
 			)
 		);
+	} else {
+		LOGGER.info('Bazel Workspace Root not detected.');


debug instead of info?

guw · 2024-03-05T05:47:37Z

src/extension.ts

 }

 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'
 		);


What is the value of duplicate messages?

guw · 2024-03-05T05:48:07Z

src/extension.ts


+	apiHandler.fireSyncStarted(workspaceRoot);


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

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

guw · 2024-03-05T05:48:26Z

src/extension.ts

+		.get('notification');
+	LOGGER.info(
+		`toggleBazelProjectSyncStatus: Is notification to be shown ${showNotification}`
+	);


debug instead of info

guw · 2024-03-05T05:49:09Z

src/extension.ts

+				} else {
+					LOGGER.error(
+						`toggleBazelProjectSyncStatus: Unexpected value returned from showWarningMessage ${val}`
+					);


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

guw · 2024-03-05T05:49:55Z

src/extension.ts

@@ -322,9 +362,12 @@ async function syncProjectViewDirectories() {
 						);
 				}
 			});
+			apiHandler.fireSyncDirectoriesEnded(bazelProjectFile.directories);


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

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.

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

guw · 2024-03-05T05:50:57Z

src/bazelLangaugeServerTerminal.ts

+		LOGGER.info(`Synchronization Summary detected. TimeTook ${timeTook}`);
+		apiHandler.fireSyncEnded(workspaceRoot, timeTook);
+	}
+}


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

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

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

@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.

Added that configurable patterns into PR

@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.

Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

* Removed Project file events * removed BazelSyncEnd event - instead added configurable BazelTerminalLog patterns * changed logging to debug - now they are not visible in the output panel Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

dkashyn-sfdc · 2024-03-05T22:41:48Z

src/loggingTCPServer.ts

 					resolve(port);
 				}
 			} else {
-				BazelLanguageServerTerminal.error(`Failed to start bazel TCP server`);
+				LOGGER.info('Failed to start bazel TCP server');


do we need port here?

AdwaitChavan and others added 6 commits February 13, 2024 03:32

add API to publish events for sync and bazel project file

2ca561a

refactor and add comments

6bd8928

amending apis

cb08be1

Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

Merge branch 'main' of https://github.com/salesforce/bazel-vscode-java …

e30dd27

…into spuliaiev/api Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

Adding onSyncEnded with time value

104ad79

Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

Cleanup and small refactoring

66ed849

Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

guw requested changes Mar 5, 2024

View reviewed changes

guw reviewed Mar 5, 2024

View reviewed changes

spuliaiev-sfdc added 3 commits March 5, 2024 10:55

rename the field to match the event field in RedHat Java events

cd6b10f

Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

amendments

0e92fe0

Signed-off-by: Sergii Puliaiev <spuliaiev@salesforce.com>

dkashyn-sfdc reviewed Mar 5, 2024

View reviewed changes

guw closed this Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@W-14955790 BazelEventsExtensionAPI #108

@W-14955790 BazelEventsExtensionAPI #108

spuliaiev-sfdc commented Mar 4, 2024

github-actions bot commented Mar 4, 2024 •

edited

guw Mar 5, 2024

spuliaiev-sfdc Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

spuliaiev-sfdc Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

guw Mar 5, 2024

spuliaiev-sfdc Mar 5, 2024

guw Mar 6, 2024

guw Mar 5, 2024

spuliaiev-sfdc Mar 5, 2024

spuliaiev-sfdc Mar 5, 2024

spuliaiev-sfdc Mar 5, 2024

spuliaiev-sfdc Mar 5, 2024

guw Mar 6, 2024

dkashyn-sfdc Mar 5, 2024

		toggleBazelProjectSyncStatus(doc);
		apiHandler.fireBazelProjectFileUpdated(doc.uri);

@W-14955790 BazelEventsExtensionAPI #108

@W-14955790 BazelEventsExtensionAPI #108

Conversation

spuliaiev-sfdc commented Mar 4, 2024

github-actions bot commented Mar 4, 2024 • edited

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 4, 2024 •

edited