Log Levels View #1138

vudayani · 2023-11-02T08:52:26Z

No description provided.

pivotal-cla · 2023-11-02T08:52:30Z

@vudayani-vmw Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

BoykoAlex

Some initial review... don't think I follow everything in there... I think i was most confused by getLoggers and getLoggersData thing...

BoykoAlex · 2023-11-06T18:31:46Z

...src/main/java/org/springframework/ide/vscode/commons/protocol/LiveProcessLoggersSummary.java

@@ -0,0 +1,72 @@
+package org.springframework.ide.vscode.commons.protocol;
+
+public class LiveProcessLoggersSummary {


Shall we switch this to record? :-) Looks like fields are not immutable thus seems to be a good candidate...

BoykoAlex · 2023-11-06T18:34:16Z

...nsions/src/main/java/org/springframework/ide/vscode/commons/protocol/STS4LanguageClient.java

+	void liveProcessLoggersDataUpdated(LiveProcessSummary processKey);
+
+	@JsonNotification("sts/liveprocess/loglevel/updated")
+	void liveProcessLogLevelUpdated(LiveProcessLoggersSummary processKey);


parameter name to adjust

BoykoAlex · 2023-11-06T18:37:56Z

...nsions/src/main/java/org/springframework/ide/vscode/commons/protocol/STS4LanguageClient.java

@@ -51,6 +51,12 @@ public interface STS4LanguageClient extends LanguageClient, SpringIndexLanguageC

 	@JsonNotification("sts/liveprocess/gcpauses/metrics/updated")
 	void liveProcessGcPausesMetricsDataUpdated(LiveProcessSummary processKey);
+
+	@JsonNotification("sts/liveprocess/loggers/updated")


What are these 2 methods are doing? My guess is once action is executed to refresh running process live data notification is sent out to the client to refresh UI etc? I see similar method for metrics but forgot the purpose :-\ I might figure it out while reviewing the rest of the code though ;-)
Does the notification with LiveProcessLoggersSummary parameter simply lets IDE know that the log level that was attempted to be changed with a post request or JMX Bean operation has indeed changed? Is that it?

What are these 2 methods are doing? My guess is once action is executed to refresh running process live data notification is sent out to the client to refresh UI etc? I see similar method for metrics but forgot the purpose :-\ I might figure it out while reviewing the rest of the code though ;-)

Yes, that's correct. It is to send notification to the client that actuator data is available. Fetch and refresh the UI.

Does the notification with LiveProcessLoggersSummary parameter simply lets IDE know that the log level that was attempted to be changed with a post request or JMX Bean operation has indeed changed? Is that it?

Yes, it just contains additional fields to let IDE know which package and log level was updated. This information is displayed as a notification in the IDE.
I didn't want to store this data and make another call to fetch the changed log level info to display in the UI as a notification.

BoykoAlex · 2023-11-06T23:49:16Z

.../java/org/springframework/ide/vscode/boot/java/livehover/v2/SpringProcessCommandHandler.java

@@ -43,6 +44,9 @@ public class SpringProcessCommandHandler {
 	private static final String COMMAND_LIST_CONNECTED = "sts/livedata/listConnected";
 	private static final String COMMAND_GET_METRICS = "sts/livedata/get/metrics";
 	private static final String COMMAND_GET_REFRESH_METRICS = "sts/livedata/refresh/metrics";
+	private static final String COMMAND_GET_LOGGERS = "sts/livedata/getLoggers";
+	private static final String COMMAND_FETCH_LOGGERS_DATA = "sts/livedata/fetch/loggersData";


Hmm... What's the loggers data for? Is this something to show in the UI? Something that is not included into loggers data structure already? (Might figure this out by the end of the review)

The COMMAND_GET_LOGGERS = "sts/livedata/getLoggers" - is to make a get request or JMX Bean operation to get the list of loggers async. Once the loggers are fetched it is stored in a loggersData map and a notification is sent to the client that logger data is available.
The COMMAND_FETCH_LOGGERS_DATA = "sts/livedata/fetch/loggersData" - is used to fetch this data from the map and show it in the IDE
The naming convention above is bad i guess and it is causing the confusion.

BoykoAlex · 2023-11-07T00:08:43Z

vscode-extensions/vscode-spring-boot/lib/set-log-levels-ui.ts

+    context.subscriptions.push(
+        VSCode.commands.registerCommand('vscode-spring-boot.set.log-levels', () => {
+            if (client.isRunning()) {
+                client.onNotification(LiveProcessLoggersUpdatedNotification.type, getLoggersList)


Hmm... is this inside the command handler? Might not be a good idea to register notification listeners every time command is executed...

BoykoAlex

Generally all looks good to me. I only had one comment about the code. The tests started passign for me once i rebased on the latest from main branch. I have tried the UI in the vscode - it worked just fine. Thus, overall, it all looks good.

There is however a broader question i had. My understanding is that we cache the loggers once receive them via JMX/Actuator endpoint and send a notification that data is there. I was thinking that perhaps unlike with memory metrics and other things we don't need to cache the loggers data? If the plan is just list current loggers in the UI for the set logger level action then we could just have a fetch command to show them within the action UI and then forget. While loggers are fetched indefinite progress bar is shown in the show pick UI with some message that loggers are fetched etc. What do you think?

BoykoAlex · 2023-12-06T19:50:53Z

vscode-extensions/vscode-spring-boot/lib/set-log-levels-ui.ts

+        const chosenPackage = await VSCode.window.showQuickPick(items);
+        if (chosenPackage) {
+            const chosenlogLevel = await VSCode.window.showQuickPick(loggers.levels);
+            const changeLogLevel = await VSCode.commands.executeCommand('sts/livedata/configure/logLevel', {"endpoint": "loggers"}, process, chosenPackage, {"configuredLevel":chosenlogLevel});


Feels like {"endpoint": "loggers"} is a redundant piece of info.

I agree with you. I will start working on it and aim to have the updates ready by next week.

Updated getLoggers and removed caching logic. A progress bar is shown in the status bar of the window when loggers are being fetched.

BoykoAlex · 2023-12-08T19:11:47Z

...ava/org/springframework/ide/vscode/boot/java/livehover/v2/SpringProcessConnectorService.java

+		if (connector != null) {
+			final IndefiniteProgressTask progressTask = getProgressTask(
+					"spring-process-connector-service-get-loggers-data" + springProcessParams.getProcessKey(), "Loggers", null);
+			System.out.println(progressTask);


Watch out for these System.out.println statements in the code :-) They are okay when you connect to LS via socket but once you build a VSCode extension and launch it without connecting to LS the comm goes over System in/out and that print statement would just corrupt the JSON RPC comm and shutdown the LS for unknown reason ;-)
So, yes, these statement would need to go or replaced with log statement :-)
I'm guilty for leaving System.out.println multiple times ;-)

BoykoAlex · 2023-12-19T14:44:40Z

@vudayani-vmw Just tried it today. Seems to be working and looks good overall. I'd just add the copyright headers for new files, mark the PR as "ready" and merge it.

martinlippert · 2023-12-19T14:59:04Z

@vudayani-vmw @BoykoAlex Keep in mind that newly added copyright headers should refer to Broadcom, Inc. instead of VMware, Inc.

BoykoAlex · 2023-12-20T21:12:44Z

LGTM - squashing and merging. Thanks @vudayani-vmw - great job!

vudayani force-pushed the main branch from 1929168 to adb8b42 Compare November 3, 2023 08:40

BoykoAlex reviewed Nov 7, 2023

View reviewed changes

vudayani force-pushed the main branch 2 times, most recently from 1f17d4d to 2072632 Compare November 23, 2023 05:26

vudayani force-pushed the main branch from 2072632 to b7fa023 Compare November 29, 2023 05:59

BoykoAlex reviewed Dec 6, 2023

View reviewed changes

vudayani force-pushed the main branch from b7fa023 to 4a1f660 Compare December 8, 2023 06:47

BoykoAlex reviewed Dec 8, 2023

View reviewed changes

vudayani force-pushed the main branch from 4a1f660 to 8c00f7e Compare December 14, 2023 10:09

V Udayani added 6 commits December 19, 2023 11:54

fetch and display loglevels

9a1c08b

change log levels for packages

c26e297

clean up logs

da522c9

address review comments

19f157d

switch the CONNECT_TO_LS key

0dc9a3e

Avoid caching loggers data

c4ede4c

vudayani force-pushed the main branch from 59fc4ac to c4ede4c Compare December 19, 2023 06:24

vudayani changed the title ~~Draft: Log Levels~~ Log Levels View Dec 19, 2023

add copyright headers

966f59a

vudayani marked this pull request as ready for review December 19, 2023 16:21

BoykoAlex merged commit e319e43 into spring-projects:main Dec 20, 2023
1 check failed

martinlippert added this to the 4.21.1.RELEASE milestone Dec 21, 2023

martinlippert added for: vscode something that is specific for VSCode theme: live-data type: enhancement labels Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log Levels View #1138

Log Levels View #1138

vudayani commented Nov 2, 2023

pivotal-cla commented Nov 2, 2023

BoykoAlex left a comment

BoykoAlex Nov 6, 2023

BoykoAlex Nov 6, 2023

BoykoAlex Nov 6, 2023

vudayani Nov 9, 2023

BoykoAlex Nov 6, 2023

vudayani Nov 9, 2023

BoykoAlex Nov 7, 2023

BoykoAlex left a comment •

edited

Loading

BoykoAlex Dec 6, 2023

vudayani Dec 7, 2023

vudayani Dec 19, 2023

BoykoAlex Dec 8, 2023

BoykoAlex commented Dec 19, 2023

martinlippert commented Dec 19, 2023

BoykoAlex commented Dec 20, 2023

		@@ -0,0 +1,72 @@
		package org.springframework.ide.vscode.commons.protocol;

		public class LiveProcessLoggersSummary {

Log Levels View #1138

Log Levels View #1138

Conversation

vudayani commented Nov 2, 2023

pivotal-cla commented Nov 2, 2023

BoykoAlex left a 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

BoykoAlex left a comment • edited Loading

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

BoykoAlex commented Dec 19, 2023

martinlippert commented Dec 19, 2023

BoykoAlex commented Dec 20, 2023

BoykoAlex left a comment •

edited

Loading