kie-issues#1694: Apache KIE Extended Services extension issues #2802

yesamer · 2024-12-12T15:12:11Z

In this PR:

The main issue is the "ambitious" management of the Extended Service Quarkus App lifecycle.
The original idea was to run the Quarkus application just after a DMN / BPMN file is opened in the editor and close it when all files are closed. Unfortunately, that implementation didn't consider the following scenarios:

Validation checks are called when the Quarkus Application is starting up and not still ready
Heartbeat requests are called in the same scenario described previously
Close and opening a single DMN/BPMN file at the same moment leads the App to reach an inconsistent state, as the request of closing and starting are launched at the same moment

For that reason, I simplified the logic: the Extended Service Quarkus App is started when a DMN / BPMN file is opened for the first time and never stopped. The original idea was indeed better from a performance and resource consumption point of view, but it requires a more complex implementation.

Updated node-fetch to 3.3.2
extendedServices.connectionHeartbeatIntervalinSecs default value changed from 1 to 10. Having a heartbeat occurring every second contributed to the reported issue
Introduced Debug logging
Improved error message management
Improved Configuration management
Improved validation call body management, using the same terminology used in JIT (backend)

ljmotta

@yesamer Thanks for your PR! I've made some comments inline.

ljmotta · 2024-12-12T17:40:57Z

packages/extended-services-vscode-extension/src/configurations/Configuration.ts

+export const enableAutoRunID = "extendedServices.enableAutorun";
+export const connectionHeartbeatIntervalinSecsID = "extendedServices.connectionHeartbeatIntervalinSecs";
+export const extendedServicesURLID = "extendedServices.extendedServicesURL";
+const defaultExtendedServicesURL = "http://localhost:21345";


We should use the env value instead of hardcoding. To do so, take a look into the build/defaultEnvJson.ts file in some packages (e.g. online-editor).

@ljmotta It's a good point. I would like to retrieve that default value directly from the configuration here https://github.com/apache/incubator-kie-tools/pull/2802/files#diff-153d43651b25ee3120020fae349c9e8ed99ccf90aa15995784702244decf9499R91 to avoid duplication and a single place for that property value, do you think is a valid alternative should I consider?

yes, it is important to have an option to configure the url of extended services, users report issues about it #2759

@ljmotta In the examples I checked, the env variables are accessed using the useEnv() hook, but in this case, we are not managing a React app. How can I access to env variable from "plain" typescript?

@yesamer That's true. the defaultEnvJson is used with the webpack transform + fetch, which isn't the case here. Now, you could use the webpack's EnvironmentPlugin to set env variables. The online-editor uses this approach to set some values inside the codebase. Take a look into the online-editor/webpack.config.ts, and search for EnvironmentPlugin. The WEBPACK_REPLACE__* values are the ones used in the codebase.

@ljmotta It should be done, there are two points to discuss:

I added EnvironmentPlugin in both extension-main and extension-browser. There's a way to define that once?

Currently, the host and the post are parametrized, In my opinion, it would be better to have the entire URL as a parameter. E.g. http protocol is hardcoded, what if the user provides its own instance using https?

@yesamer

If you need to use in both, you can set the env variables in the commonConfig variable.

Good point. I'm not familiar on how the user set/use the Extended Services in the VS Code extension. Is it a embeded Extended Services or the user need to manually start the server? For the extended-services-java it will always use http. If the user wants to deploy it, the user will need to make the necessary adjustments to make it available through https.

@ljmotta

Do you have an example for that? I mean, I added the plugin entry for both web and node https://github.com/apache/incubator-kie-tools/pull/2802/files#diff-1768335dedd5cfa5e1b3830bb3b6c504f1d316dea21de55738df74f7f19c27a5R52
To create a single entry for the plugin, which target and entry should I use?

You're right, considering extended-services-java always uses http, we are safe here :)

ljmotta · 2024-12-12T17:49:58Z

packages/extended-services-vscode-extension/src/configurations/Configuration.ts

+  const value = vscode.workspace.getConfiguration().get(property) as T;
+  if (!value) {
+    console.warn("Property: " + property + " is missing, using the default: " + defaultValue);
  }
+  return value || defaultValue;
+};


I'm not sure if I followed. T can be a boolean, number or string. If is a boolean it will always be true, as it is the default value. For number, can't be 0, and string can't be ""? Or value can't be undefined? The (!value) can be tricky, please, make it explicity.

T can be a boolean, number or string. If is a boolean it will always be true, as it is the default value. For number, can't be 0, and string can't be ""

So, if the user explicitly removes the property the default value is always returned?

@yesamer I just considered the calls to this method. We have just 3 on this file, each one of them with T equals to boolean, number or string. The if will be true for the cases I listed, and we will have the log. Additionally, the return will always go to the default value for the same cases:

> "" || "my_default_value" 'my_default_value' > 0 || 25 25 > false || true true

If the value (has the type T due your casting) can have a different type, please make it explicity. Something like: T | null or T | undefined.

packages/extended-services-vscode-extension/src/requests/ValidationRequests.ts

jomarko

Thank you for the PR. I am happy we improve this extension.

01

Sometimes we use extended service (singletone) and sometimes we use extended services (plural).

02

The PR description says we never stop the extended services. Should then extension-browser.ts contain const stopExtendedServicesCommandUID: string = "extended-services-vscode-extension.stopExtendedServices"; and related code? Or do we keep the option to manually stop the extended services?

No manual check done, waiting until code changes are finished.

packages/extended-services-vscode-extension/src/Validator.ts

jomarko · 2024-12-13T08:34:08Z

packages/extended-services-vscode-extension/src/configurations/Configuration.ts

+export const enableAutoRunID = "extendedServices.enableAutorun";
+export const connectionHeartbeatIntervalinSecsID = "extendedServices.connectionHeartbeatIntervalinSecs";
+export const extendedServicesURLID = "extendedServices.extendedServicesURL";
+const defaultExtendedServicesURL = "http://localhost:21345";


yes, it is important to have an option to configure the url of extended services, users report issues about it #2759

packages/extended-services-vscode-extension/src/configurations/Configuration.ts

packages/extended-services-vscode-extension/src/Validator.ts

packages/extended-services-vscode-extension/src/extension/extension-browser.ts

packages/extended-services-vscode-extension/src/extension/extension-main.ts

# Conflicts: # packages/extended-services-vscode-extension/src/configurations/Configuration.ts

packages/extended-services-vscode-extension/env/index.js

yesamer · 2024-12-17T08:47:24Z

@jomarko
01
I updated all the references to use the plural form

02
Yes, the user can close the connection using a button in the bottom bar of the editor.

packages/extended-services-vscode-extension/src/configurations/Configuration.ts

jomarko · 2025-01-03T12:49:06Z

Closing the model file

Hi @yesamer or anyone, should closing the dmn/bpmn file to close also the message in problems tab? Currently it seems the problems tab is not cleared if model file is closed. Is that an expected behavior?

Disable Autorun and use URL

Once I disabled the autorun and started extended services as pnpm -F @kie-tools/extended-services-java start I was not able to connect VSCode extended services extension to the extended services instance.

…/Configuration.ts Co-authored-by: Luiz João Motta <[email protected]>

…1694

yesamer · 2025-01-08T15:45:04Z

@jomarko
/cc @ljmotta @tiagobento

I'm double-checking the behavior you reported. However, I'm not fully aware of the expected behavior of this feature, so my goal is to implement a minimal use-case scenario for this extension.

yesamer · 2025-01-10T14:13:35Z

@jomarko I fixed the first use case you reported, Closing the model file.
The second scenario, Disable Autorun and use URL, works correctly in my local

Screen.Recording.2025-01-10.at.15.10.28.mov

ljmotta · 2025-01-10T17:12:18Z

@jomarko I'm not sure if using 0.0.0.0 is correct here, please try with localhost.

@yesamer Please let me kwown when the PR is ready for another review.

yesamer · 2025-01-10T17:13:45Z

@ljmotta Yes, please :)

ljmotta

@yesamer Thanks for your patience, LGTM!

tiagobento · 2025-01-13T13:28:01Z

Deferring my review to @jomarko, feel free to merge when approved by him and green! Thanks for the great effort here.

jomarko

Thank you @yesamer , I put some comments to code inline.

One question is also what is the difference of extension-main.ts and extension-browser.ts as their content seems to be very similar.

My biggest problem is there is no vsix file produced by the PR, or am I searching inappropriately? I tried to build the extension locally: pnpm -F extended-services-vscode-extension... build:prod however in that case it seems to not work properly, I would like to use vsix produced by the CI.

packages/extended-services-vscode-extension/src/Validator.ts

packages/extended-services-vscode-extension/src/configurations/Configuration.ts

packages/extended-services-vscode-extension/src/extension/extension-browser.ts

packages/extended-services-vscode-extension/src/requests/PingRequest.ts

Co-authored-by: Jozef Marko <[email protected]>

…nsion-browser.ts Co-authored-by: Jozef Marko <[email protected]>

jomarko

Thank you @yesamer. Using the VSIX produced by the CI I was able to:

use extended-services extension in the auto run mode
use extended-services extension connecting to services started in standalone terminal

Approving the PR, however opening two points that are not blocking from the merge.

Extension info

Seems to show old information, probably it needs to be first pushed to marketplace? I do not know. Notice the ping interval value.

Icon

Extended-services liveness icon indicator (bottom right corner) is shown from my point of view in 'difficult to understand' way. From my point of view, in separate ticket it would be worth to do a simplification. Here is my proposal, however not sure what is opinion of others.

Once extended-services extension is installed:

we either show connection live icon
or connection not live icon
there should be no case icon is missing

yesamer · 2025-01-16T09:22:48Z

@jomarko Thank you for the review, it's indeed strange the Extension info is not updated. I checked, and the default value is correct in the package.json.
Regarding your proposal with the ICON, I agree with you that should be revisited, together with other components.
We agreed with @tiagobento to invest more time on this extension in the future, based on our priorities, so it makes sense to keep track of all elements/behavior you already found during this PR review, and open a discussion of its requirements

yesamer added 4 commits December 12, 2024 09:52

Packages

a9d5f35

WIP

bfd6f96

WIP

1e4cbdb

Fix

a9970d3

yesamer requested a review from tiagobento as a code owner December 12, 2024 15:12

WIP

497217f

ljmotta reviewed Dec 12, 2024

View reviewed changes

CR request

3260a41

yesamer requested a review from jomarko December 12, 2024 21:54

jomarko reviewed Dec 13, 2024

View reviewed changes

yesamer added 4 commits December 16, 2024 12:46

wip

25b5662

Merge branch 'refs/heads/main' into kie-issues#1694

ea6f438

# Conflicts: # packages/extended-services-vscode-extension/src/configurations/Configuration.ts

CR

1b86f15

Added env.

6f2a072

ljmotta reviewed Dec 16, 2024

View reviewed changes

packages/extended-services-vscode-extension/env/index.js Outdated Show resolved Hide resolved

wip

3d83bf8

yesamer mentioned this pull request Dec 16, 2024

Apache KIE Extended Services extension issues apache/incubator-kie-issues#1684

Closed

yesamer added 3 commits December 16, 2024 18:27

WIP

89a69b1

WIP

5d2b781

minor

f9fdc5f

yesamer added 3 commits December 17, 2024 11:55

minor

8eebccb

Merge remote-tracking branch 'origin/main' into kie-issues#1694

223e14d

Minor

bb98575

ljmotta reviewed Dec 30, 2024

View reviewed changes

packages/extended-services-vscode-extension/src/configurations/Configuration.ts Outdated Show resolved Hide resolved

yesamer and others added 4 commits January 7, 2025 14:30

Update packages/extended-services-vscode-extension/src/configurations…

011beb5

…/Configuration.ts Co-authored-by: Luiz João Motta <[email protected]>

Merge remote-tracking branch 'origin/main' into kie-issues#1694

34a0095

Merge remote-tracking branch 'yeser/kie-issues#1694' into kie-issues#…

08bef0b

…1694

fix

a7b083d

yesamer added pr: wip PR is still under development pr: DO NOT MERGE Draft PR, not ready for merging labels Jan 8, 2025

yesamer added 3 commits January 8, 2025 18:16

CR

1bf8620

Merge remote-tracking branch 'origin/main' into kie-issues#1694

a556b1c

Merge remote-tracking branch 'origin/main' into kie-issues#1694

1e29bcb

ljmotta approved these changes Jan 10, 2025

View reviewed changes

yesamer removed pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development labels Jan 13, 2025

yesamer requested a review from jomarko January 13, 2025 13:18

tiagobento removed their request for review January 13, 2025 13:28

tiagobento added area:bpmn area:dmn labels Jan 13, 2025

jomarko reviewed Jan 16, 2025

View reviewed changes

yesamer and others added 3 commits January 16, 2025 08:00

Update packages/extended-services-vscode-extension/src/Validator.ts

51cb84d

Co-authored-by: Jozef Marko <[email protected]>

Update packages/extended-services-vscode-extension/src/extension/exte…

3e9cd1b

…nsion-browser.ts Co-authored-by: Jozef Marko <[email protected]>

CR

fa14e13

jomarko approved these changes Jan 16, 2025

View reviewed changes

yesamer merged commit 52040d7 into apache:main Jan 16, 2025
15 checks passed

yesamer deleted the kie-issues#1694 branch January 16, 2025 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kie-issues#1694: Apache KIE Extended Services extension issues #2802

kie-issues#1694: Apache KIE Extended Services extension issues #2802

yesamer commented Dec 12, 2024 •

edited

Loading

ljmotta left a comment

ljmotta Dec 12, 2024

yesamer Dec 12, 2024

jomarko Dec 13, 2024

yesamer Dec 16, 2024

ljmotta Dec 16, 2024

yesamer Dec 16, 2024

ljmotta Dec 17, 2024

yesamer Dec 18, 2024

ljmotta Dec 12, 2024

yesamer Dec 12, 2024

ljmotta Dec 13, 2024

jomarko left a comment

jomarko Dec 13, 2024

yesamer commented Dec 17, 2024

jomarko commented Jan 3, 2025

yesamer commented Jan 8, 2025

yesamer commented Jan 10, 2025

ljmotta commented Jan 10, 2025

yesamer commented Jan 10, 2025

ljmotta left a comment

tiagobento commented Jan 13, 2025

jomarko left a comment

jomarko left a comment

yesamer commented Jan 16, 2025 •

edited

Loading

kie-issues#1694: Apache KIE Extended Services extension issues #2802

kie-issues#1694: Apache KIE Extended Services extension issues #2802

Conversation

yesamer commented Dec 12, 2024 • edited Loading

ljmotta 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

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

jomarko left a comment

Choose a reason for hiding this comment

01

02

Choose a reason for hiding this comment

yesamer commented Dec 17, 2024

jomarko commented Jan 3, 2025

Closing the model file

Disable Autorun and use URL

yesamer commented Jan 8, 2025

yesamer commented Jan 10, 2025

ljmotta commented Jan 10, 2025

yesamer commented Jan 10, 2025

ljmotta left a comment

Choose a reason for hiding this comment

tiagobento commented Jan 13, 2025

jomarko left a comment

Choose a reason for hiding this comment

jomarko left a comment

Choose a reason for hiding this comment

Extension info

Icon

yesamer commented Jan 16, 2025 • edited Loading

yesamer commented Dec 12, 2024 •

edited

Loading

yesamer commented Jan 16, 2025 •

edited

Loading