Use asExternalUri For Server Url #196

williamsyang-work · 2024-01-30T20:45:35Z

Refactors TspClientProvider to use VSCode.asExternalUri method to generate a dynamic
uri. This will automatically detect if we are using remote-ssh, initialize port forwarding
to the server's port, and change the provided url to a web-worker that is managing
port-forwarding to the VSCode backend.

Removes the TraceServerUrlManager class. The functionality to generate new TspClients
when the Uri is changed is moved into this repo's TspClientProvider implementation.

Removes the tspClient.ts file. This functionality did not work with the async nature of
asExternalUri, and was not designed to handle Uri changes. Most functionality was already
implemented in TspClientProviderImpl.ts.

Creates VSCodeBackendTspClientProvider class. This is a child class of TspClientProviderImpl
with expanded functionality that should only be used on the extension's backend.

Signed-off-by: Will Yang [email protected]

williamsyang-work · 2024-01-30T20:55:00Z

I'm re-submitting this PR. I attempted to make this change before, but multiple bugs were brought up for the remote use-case.
You can view the old PR here: PR #173.

I would like for this change to be merged in, and to address the concerns in PR #173 as separate feature additions / merge requests. The changes that I tried to implement before were way out of scope of the base change.

williamsyang-work · 2024-01-30T20:57:10Z

...-extension/src/trace-explorer/opened-traces/trace-explorer-opened-traces-webview-provider.ts

+                    case VSCODE_MESSAGES.EXPERIMENT_SELECTED:
+                        // Need to block scope the variable 'experiment'...
+                        if (true) {
+                            let experiment: Experiment | undefined;
+                            if (message.data && message.data.wrapper) {
+                                experiment = convertSignalExperiment(JSONBig.parse(message.data.wrapper));
+                            } else {
+                                experiment = undefined;
+                            }
+                            signalManager().fireExperimentSelectedSignal(experiment);
                        }
-                        signalManager().fireExperimentSelectedSignal(experiment);
-                    }
+                        return;


The variable name experiment is also used above in line 107 of this code.

I was getting an error for "'experiment' has already been declared". So, I block-scoped the code to appease the typescript compiler. Otherwise, it would not build.

No logic changed here.

No need of an if(true) statement, just wrap both switch cases with braces so that any variable declared within that block is only visible within the block.

I just removed this code from the PR.

marcdumais-work · 2024-02-01T18:56:38Z

@williamsyang-work Please ignore the 3PP license check CI job failing ATM - it's nothing related to your PR and will not prevent merging. FYI, IP check tickets are opened for the baseline dependencies reported as errors (unapproved), and they should be approved soon, letting the job pass. But until then, no worries.

bhufmann · 2024-02-06T13:36:41Z

I'm re-submitting this PR. I attempted to make this change before, but multiple bugs were brought up for the remote use-case. You can view the old PR here: PR #173.

I would like for this change to be merged in, and to address the concerns in PR #173 as separate feature additions / merge requests. The changes that I tried to implement before were way out of scope of the base change.

The issue of pending back-end calls reported here #173 (review), that causes the application not to function until the port forwarding manually removed, is still an issue. Could you please describe what's the plan to address them?

bhufmann

It looks like that this update breaks the local case. When testing it for a local use case I noticed some issues when opening a trace. The trace explorer on the left with the 3 webviews disappear, as well as the status bar indicates that the server is down (while the trace panel is opened in the middle).

bhufmann · 2024-02-06T14:12:43Z

vscode-trace-extension/src/extension.ts

    const analysisProvider = new AnalysisProvider();
    // TODO: For now, a different command opens traces from file explorer. Remove when we have a proper trace finder
    const fileOpenHandler = fileHandler(analysisProvider);
    context.subscriptions.push(
        vscode.commands.registerCommand('traces.openTraceFile', async (file: vscode.Uri) => {
            await startTraceServerIfAvailable(file.fsPath);
+            await updateUris();


I don't think we should call it at every open. updateUris() will each webview (e.g. open traces) with the new url and the tspclient instance is refreshed. This will require a refressh of each webview which doesn't exist. I think the URLs should be only updated when changed.

bhufmann · 2024-02-06T14:13:37Z

vscode-trace-extension/src/extension.ts

@@ -162,6 +168,7 @@ export function activate(context: vscode.ExtensionContext): ExternalAPI {
                return;
            }
            await startTraceServerIfAvailable(traceUri.fsPath);
+            await updateUris();


same comment here. The URL should not be updated everytime a trace is opened. It should be only be done if necessary.

bhufmann

Is this still a draft? There are compilation errors as well linting error. Ignore the license check, that error will go away once license check by Eclipse is finished.

Refactors TspClientProvider to use VSCode.asExternalUri method to generate a dynamic uri. This will automatically detect if we are using remote-ssh, initialize port forwarding to the server's port, and change the provided url to a web-worker that is managing port-forwarding to the VSCode backend. Removes the TraceServerUrlManager class. The functionality to generate new TspClients when the Uri is changed is moved into this repo's TspClientProvider implementation. Removes the tspClient.ts file. This functionality did not work with the async nature of asExternalUri, and was not designed to handle Uri changes. Most functionality was already implemented in TspClientProviderImpl.ts. Creates VSCodeBackendTspClientProvider class. This is a child class of TspClientProviderImpl with expanded functionality that should only be used on the extension's backend. Signed-off-by: Will Yang <[email protected]>

williamsyang-work · 2024-02-14T18:57:40Z

Is this still a draft? There are compilation errors as well linting error. Ignore the license check, that error will go away once license check by Eclipse is finished.

I just fixed the linting errors, build errors, and rebased with the current master. It's now ready to continue the review process!

williamsyang-work · 2024-02-15T21:56:57Z

It looks like that this update breaks the local case. When testing it for a local use case I noticed some issues when opening a trace. The trace explorer on the left with the 3 webviews disappear, as well as the status bar indicates that the server is down (while the trace panel is opened in the middle).

I'm not getting this behavior when I am running the local version. But it seems Neel is also running into this error with the current code: #204 (comment).

bhufmann · 2024-02-20T15:17:15Z

It looks like that this update breaks the local case. When testing it for a local use case I noticed some issues when opening a trace. The trace explorer on the left with the 3 webviews disappear, as well as the status bar indicates that the server is down (while the trace panel is opened in the middle).

I'm not getting this behavior when I am running the local version. But it seems Neel is also running into this error with the current code: #204 (comment).

When using the latest update, I didn't get the issue for the local case anymore. So, I focused on the remote case, which there seems to be differences when running in vscodium vs vscode. I need investigate more to be able to pinpoint what's going on.

I'm trying the local case as well again.

ngondalia · 2024-02-22T19:46:32Z

url-change-fix-not-working.mp4

ngondalia · 2024-02-22T19:48:07Z

url-change-fix-not-working.mp4

Hope this helps in being able to reproduce it!

williamsyang-work · 2024-03-05T18:02:22Z

Closing this PR.

williamsyang-work commented Jan 30, 2024

View reviewed changes

williamsyang-work force-pushed the as-external-uri-remote-fix branch from 7b3f2ec to 48f7b23 Compare January 31, 2024 20:34

hriday-panchasara self-requested a review February 1, 2024 23:53

williamsyang-work force-pushed the as-external-uri-remote-fix branch 2 times, most recently from c4bfc0c to c61d1d9 Compare February 5, 2024 19:14

bhufmann requested changes Feb 6, 2024

View reviewed changes

williamsyang-work force-pushed the as-external-uri-remote-fix branch from b3142da to cc4d9e5 Compare February 6, 2024 22:57

bhufmann reviewed Feb 13, 2024

View reviewed changes

williamsyang-work force-pushed the as-external-uri-remote-fix branch 2 times, most recently from 287bc2a to 4dd1784 Compare February 14, 2024 17:40

williamsyang-work force-pushed the as-external-uri-remote-fix branch from 4dd1784 to dc685dc Compare February 14, 2024 17:42

lint

120b50f

williamsyang-work mentioned this pull request Feb 15, 2024

Proper Handling of the URL Configuration Change #204

Merged

williamsyang-work requested a review from bhufmann February 15, 2024 21:41

williamsyang-work marked this pull request as draft February 27, 2024 22:47

williamsyang-work closed this Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use asExternalUri For Server Url #196

Use asExternalUri For Server Url #196

williamsyang-work commented Jan 30, 2024 •

edited

Loading

williamsyang-work commented Jan 30, 2024

williamsyang-work Jan 30, 2024 •

edited

Loading

hriday-panchasara Feb 2, 2024 •

edited

Loading

williamsyang-work Feb 5, 2024

marcdumais-work commented Feb 1, 2024

bhufmann commented Feb 6, 2024 •

edited

Loading

bhufmann left a comment

bhufmann Feb 6, 2024

bhufmann Feb 6, 2024

bhufmann left a comment

williamsyang-work commented Feb 14, 2024

williamsyang-work commented Feb 15, 2024

bhufmann commented Feb 20, 2024 •

edited

Loading

ngondalia commented Feb 22, 2024

ngondalia commented Feb 22, 2024

williamsyang-work commented Mar 5, 2024

Use asExternalUri For Server Url #196

Use asExternalUri For Server Url #196

Conversation

williamsyang-work commented Jan 30, 2024 • edited Loading

williamsyang-work commented Jan 30, 2024

williamsyang-work Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

hriday-panchasara Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

williamsyang-work Feb 5, 2024

Choose a reason for hiding this comment

marcdumais-work commented Feb 1, 2024

bhufmann commented Feb 6, 2024 • edited Loading

bhufmann left a comment

Choose a reason for hiding this comment

bhufmann Feb 6, 2024

Choose a reason for hiding this comment

bhufmann Feb 6, 2024

Choose a reason for hiding this comment

bhufmann left a comment

Choose a reason for hiding this comment

williamsyang-work commented Feb 14, 2024

williamsyang-work commented Feb 15, 2024

bhufmann commented Feb 20, 2024 • edited Loading

ngondalia commented Feb 22, 2024

ngondalia commented Feb 22, 2024

williamsyang-work commented Mar 5, 2024

williamsyang-work commented Jan 30, 2024 •

edited

Loading

williamsyang-work Jan 30, 2024 •

edited

Loading

hriday-panchasara Feb 2, 2024 •

edited

Loading

bhufmann commented Feb 6, 2024 •

edited

Loading

bhufmann commented Feb 20, 2024 •

edited

Loading