Report workspace stats in shared process

[RMacfarlane]

This adds back some workspace metadata telemetry, collected in the shared process on start up. I didn't move any of the other reporting there, not sure if we want to shift that as well.

[bpasero]

Adding @joaomoreno to review the related changes for allowing to send data to the shared process via IPC.

[bpasero]

@RMacfarlane I see we have now added back the previously commented (rather expensive) code to collectWorkspaceStats and it is being called from the shared process as suggested. However, I see this method also called from other contexts (e.g. remote, client), so my understanding is that the performance issue would come back in these contexts? I think we should maybe add a flag to this method to indicate if we want the expensive scanning or not and then only do so when run from the shared process?

Alternatively, even better: can we remove collectWorkspaceStats from being used anywhere except from the shared process?

[joaomoreno]

This makes it that the renderer sends the info over to main which just passes it along to the shared process. Every renderer already has an open connection to the shared process, we should send it via that instead. Example: telemetry service.

Since workspace stats is so much related to telemetry, why not simply add a method on the telemetry service for this?

[joaomoreno]

The whole point of the IPC abstraction is that we stop using strings. This should be reportWorkspaceStats(workspace: IWorkspace) instead.

[joaomoreno]

I hit Approve when I wanted to hit Request changes.

[bpasero]

@RMacfarlane one additional thought: can we not simply move the entire WorkspaceStats thing over to the shared process? Maybe Joao suggested this already: simply have a method on the telemetry service to report workspace info (everything) via the shared process and never have this method run on the renderer at all.

[RMacfarlane]

@bpasero There are two parts to collectWorkspaceStats - collecting the number of different file types in a workspace, walking the directory for up to 20000 files, and reading the types within the launch configs in the workspace. The expensive walk was never removed from this path, what I added back was collecting launch config data. This does not walk, it only loops over the folders in the workspace and looks for a launch.json in the .vscode folder.

The other places this is called are

• When running --status
• When collecting information to report a performance issue

In these cases, the workspace stats aren't meant to be logged by telemetry, but reported to the user/us to help understand problems. If we think this isn't valuable to collect, we can remove them. I think moving this to the telemetry service would only make sense if collectWorkspaceStats wasn't used in these other places.

[RMacfarlane]

I'm wondering if it makes sense to move the entire diagnosticsService into the shared process. Here's how communication currently works in the other collectWorkspaceStatsCases:

• --status
  The new instance of code connects to the existing main process and gets info from it, what its pid, arguments list, and windows are. It then runs the rest of the calculation in its own main process using the diagnosticsService.

• Performance issue reporting
  The issue reporter sends an IPC message back to the service in main the created it. This service calls on the diagnosticsService after collecting the main process info, so calculations are once again done in main. If the user has a remote, we try to collect the same information on that machine as well, which requires going through the renderer process since this has the connection to the remote agent. The remote agent calculates the stats and sends them back, which hops from renderer to main to the issue reporter.

So, if diagnosticsService moves to the shared process

• --status
  The new instance of code connects to the shared process. Either the shared process communicates with main to get the info it needs, or has this passed to it.

• Performance issue reporting
  If possible, talk to the shared process directly. Once again either need shared process to grab the main process info, or have the issue reporter get it from main and pass it along. For the remote case, the shared process still needs to grab data from the renderer.

[bpasero]

@RMacfarlane yes I like your proposal from #75291 (comment) very much. I think in all cases the shared process will be up and ready because --status for example requires an existing running instance to be there.

[RMacfarlane]

OK, I've moved the diagnostic service implementation to be in the shared process. Getting the remote diagnostics is a little clumsy still, I couldn't find an example of using a channel in a bi-directional way. So instead of fetching the remote info from the shared process, I just pass it over.

[bpasero]

@RMacfarlane going into a good direction, moving this over to shared process 👍

Can we make sure that the IDiagnosticsService is not being contributed in general (to workbench.main.ts) but rather only as part of the workbench stats contribution? That seems like the only client of this service and otherwise you would need to provide some stub for the web version where we probably need a different solution.

[RMacfarlane]

I removed the IDiagnosticsService from workbench.main.ts and am now just using the ISharedProcessService in workbench stats to make the call to the shared process. I had to move the workbench stats file to electron-browser for proper layering when importing ISharedProcessService, which caused a bunch of other file changes to update their import path to it.

[joaomoreno]

IPC looks good to me. 👍

[bpasero]

@RMacfarlane generally OK to me, some feedback:

• noticed you had to move a bit of code to electron-browser due to some weird direct dependency to workspaceStats, I filed WorkspaceStat.TAGS asks for trouble #76002 as follow up
• I noticed launch service is now in common which is rather unlikely to ever happen, can we only move IMainProcessInfo if that is the only thing that is needed?

[RMacfarlane]

@bpasero
Yeah, the TAGS dependency is strange and surprised me as well, I'll address that separately. I moved everything except IMainProcessInfo and the IWindowInfo that it uses back into electron-main