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

#320 Show active profiles on BootApp description when attached to liv… #321

Merged
merged 8 commits into from
Apr 11, 2024

Conversation

karthik20
Copy link
Contributor

Addresses improvement of #320 .

The changes include updating the BootApp description along with the systemProperty => spring.profiles.active.

Inline with the previous logic to display the other description items like contextPath and port, the property values need to be exposed for the IDE to display the comma separated profiles instead of astericks (****)

Quick video of how it works:

show_profiles-onstartup-comma.mp4

Copy link
Contributor

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

image

The system properties are hidden by default in the spring app. I doubt that showing such info ****** in the APPS node is helpful.

src/BootApp.ts Outdated Show resolved Hide resolved
@karthik20
Copy link
Contributor Author

karthik20 commented May 31, 2023

image

The system properties are hidden by default in the spring app. I doubt that showing such info ****** in the APPS node is helpful.

@testforstephen You are right about that. I presumably overdid on that 😁. New changes should take the profiles from the launch/prompt dialog and resets after the liveProcess ends. Let me know your thoughts.

One profile:

image

Multiple profiles:

multi_profile_fromPrompt

Default profile:

default(no)_profile_from_prompt

 - Take the launched profiles from the prompt at launch instead of system properties.
 - App description change and skip default profile being displayed
Copy link
Contributor

@testforstephen testforstephen left a comment

Choose a reason for hiding this comment

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

The new experience looks better, just some minor comments on the label format.

src/views/items/BootAppItem.ts Outdated Show resolved Hide resolved
@karthik20
Copy link
Contributor Author

Final result with changes:
image

@karthik20
Copy link
Contributor Author

@testforstephen, the test cases tend to fail everytime. Anything else to be done as part of this PR?

@testforstephen
Copy link
Contributor

Looks like the test case is not stable, we need to find some time to improve the test case implementation.

@karthik20
Copy link
Contributor Author

karthik20 commented Jun 12, 2023

Yes, seems so. In that case, I guess rest of the changes are good to go now? @testforstephen

@karthik20
Copy link
Contributor Author

@testforstephen this PR is pending for a while. Good to merge please?

@BoykoAlex
Copy link
Contributor

LGTM. I'd merge this one.

I'd also create an issue on the Boot LS side to implement the command to fetch all available profiles from a Spring Boot project... A single YAML properties file may define multiple profiles, surprisingly same goes for the .properties file now. Looks like this logic belongs to the LS rather than implemented on the IDE clients.

@BoykoAlex
Copy link
Contributor

@karthik20 on the second thought I think it'd be best to get active profile directly from the running app if live data is available.
In stsApi I'd add:

export async function getActiveProfiles(processKey: string) {
    const result = await stsApi?.getLiveProcessData({
        processKey: processKey,
        endpoint: "profiles"
    }) as string[];
    return result;
}

This should be available via the Spring Boot Tools extension snapshot with this commit: f52967d9f81e6b18369aa6cbac229c5a58bb94b8

Then once stsApi module has getActiveProfiles(processKey: string) I'd use it here: https://github.com/microsoft/vscode-spring-boot-dashboard/blob/main/src/controllers/LiveDataController.ts#L60 similar to port and pass a list of active profiles over to runningApp again similar to port.

@karthik20
Copy link
Contributor Author

@karthik20 on the second thought I think it'd be best to get active profile directly from the running app if live data is available. In stsApi I'd add:

export async function getActiveProfiles(processKey: string) {
    const result = await stsApi?.getLiveProcessData({
        processKey: processKey,
        endpoint: "profiles"
    }) as string[];
    return result;
}

This should be available via the Spring Boot Tools extension snapshot with this commit: f52967d9f81e6b18369aa6cbac229c5a58bb94b8

Then once stsApi module has getActiveProfiles(processKey: string) I'd use it here: https://github.com/microsoft/vscode-spring-boot-dashboard/blob/main/src/controllers/LiveDataController.ts#L60 similar to port and pass a list of active profiles over to runningApp again similar to port.

Makes sense, and yes. I saw your commit on sts4; I tested the changes with live process with the Spring Boot tools (v1.54.2024040604 (pre-release)) and functionality works as expected.

Help to review the latest changes and let me know. This PR is almost a year old ;)

@BoykoAlex
Copy link
Contributor

LGTM. Good to merge from my standpoint.
@testforstephen what do you think? :-)

@karthik20
Copy link
Contributor Author

Tests still fail I guess. @testforstephen and @BoykoAlex, can be merged then?

@testforstephen
Copy link
Contributor

I attempted to locally test this PR using the spring-petclinic sample project. However, despite the application having started successfully, the app icon is still spinning in the dashboard view. It might be an environment issue in my machine, I need find some time to test it again.

@testforstephen
Copy link
Contributor

@BoykoAlex when I start spring-petclinic in VS Code, I got the error "Failed to refresh live data from process service:jmx:rmi:///jndi/rmi://127.0.0.1:55504/jmxrmi after retries: 10" from Spring Boot Tools extension a few times. Do you have any idea on what happens?

@BoykoAlex
Copy link
Contributor

BoykoAlex commented Apr 9, 2024

@testforstephen looks like it is me to blame... We are re-working the UI and the backend logic for connecting to live process in order to prepare the Boot LS to be compiled into native image. One of the blockers for native compilation was the usage of Virtual VM API which helped us to list local JVM process and to setup JMX connection etc. If this API is not available then the Boot App process needs to be setup for the JMX connection by us via launch config. Hence you see the JMX URL in the "trying to connect" progress status bar.
Looks like pid and process type are not what is expected in some places. The process type was "remote" in this particular case - I switched it to "local" for the case of local process JMX connection so now with Spring Boot Tools snapshot the spinning wheel issue will be fixed.
However, looks like there might be more work on the Boot Dashboard due to this change... For example I see an error showing up sometimes in the console in memory view on this line: https://github.com/microsoft/vscode-spring-boot-dashboard/blob/main/src/views/memory.ts#L156

The consequence of this change is processKey is not a PID but a JMX URL, PID is in the pid property of the LiveProcessSummary that you usually get from the sts api.

I'd wait for the April 9 build at the bottom of this page, download and install the VSIX. The pre-release went out tonight without the change you're looking for :-\

@karthik20
Copy link
Contributor Author

@testforstephen looks like it is me to blame... We are re-working the UI and the backend logic for connecting to live process in order to prepare the Boot LS to be compiled into native image. One of the blockers for native compilation was the usage of Virtual VM API which helped us to list local JVM process and to setup JMX connection etc. If this API is not available then the Boot App process needs to be setup for the JMX connection by us via launch config. Hence you see the JMX URL in the "trying to connect" progress status bar. Looks like pid and process type are not what is expected in some places. The process type was "remote" in this particular case - I switched it to "local" for the case of local process JMX connection so now with Spring Boot Tools snapshot the spinning wheel issue will be fixed. However, looks like there might be more work on the Boot Dashboard due to this change... For example I see an error showing up sometimes in the console in memory view on this line: https://github.com/microsoft/vscode-spring-boot-dashboard/blob/main/src/views/memory.ts#L156

The consequence of this change is processKey is not a PID but a JMX URL, PID is in the pid property of the LiveProcessSummary that you usually get from the sts api.

I'd wait for the April 9 build at the bottom of this page, download and install the VSIX. The pre-release went out tonight without the change you're looking for :-\

Glad you mentioned in detail. For my testing as well couple times I had to toggle the process type from remote to local. I assumed that since I was using WSL2 and VSCode remote for my testing, the process type was remote. Your explanation helps.

@karthik20
Copy link
Contributor Author

karthik20 commented Apr 10, 2024

@BoykoAlex Thanks. I used the VSIX and I can see that the process type is now local as opposed to remote last time.

VSIX installed:
image

image

I don't see that spinning wheel anymore either (WSL2 tested). @testforstephen please verify from yourside.

image

P.S: Please note that I updated the spring-petclinic project git sub-module to the latest that brings Spring boot 3.2.x.

@testforstephen
Copy link
Contributor

it works fine in the latest Spring Boot Tools v1.54.2024041104 (pre-release).

@testforstephen testforstephen merged commit a4d57ac into microsoft:main Apr 11, 2024
1 of 4 checks passed
@karthik20 karthik20 deleted the show-active-profiles branch April 11, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants