More repo info telemetry check to support windows repo perf issues#4339
Conversation
| return skipDiffResult('virtualFileSystem'); | ||
| } | ||
| } catch { | ||
| return skipDiffResult('virtualFileSystem'); |
There was a problem hiding this comment.
For all these err on the side of not collecting if we can't calculate the check we are doing.
There was a problem hiding this comment.
Pull request overview
Adds additional early-exit checks to first-party repo diff telemetry to avoid expensive Git operations in known slow Windows repo configurations (VFS/sparse checkout and large local commit ranges).
Changes:
- Skip repo info diff telemetry when
core.virtualfilesystemorcore.sparsecheckoutis configured (or config lookup fails). - Skip repo info diff telemetry when the commit range between merge-base and
HEADreaches a configured maximum. - Extend unit tests to cover the new skip conditions and update repo mocks with
getConfig/log.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/extension/prompt/node/repoInfoTelemetry.ts | Adds VFS/sparse-checkout and commit-count guards to skip costly diff collection. |
| src/extension/prompt/node/test/repoInfoTelemetry.spec.ts | Adds new test coverage for the guard conditions and updates repository mocks accordingly. |
| try { | ||
| const virtualFileSystem = await repository.getConfig('core.virtualfilesystem'); | ||
| const sparseCheckout = await repository.getConfig('core.sparsecheckout'); | ||
| if (virtualFileSystem === 'true' || sparseCheckout === 'true') { |
There was a problem hiding this comment.
core.virtualFileSystem isn't necessarily a boolean. If it has any truthy (for C-definition of truthy) value then it is enabled.
There was a problem hiding this comment.
"true" is used sometimes, but typically only in unit tests. Real repositories will have it set to the path of a hook so git can talk to the VFS.
There was a problem hiding this comment.
core.sparsecheckout should also be truthy - I see examples of both "true" and "1" for it in the git source code.
There was a problem hiding this comment.
So it looks like virtualFileSystem is just if it's specified, but sparse checkout is actually this https://git-scm.com/docs/git-config#Documentation/git-config.txt-boolean ? Looks like vscode's git extension doesn't do any type of type coercion there. So ['yes','true','on','1'] looks like the set to me, case insensitive. Was honestly a bit hard to tell from the docs exact what sparsecheckout was for sure, but I always saw enable and disable used to describe it, and since true and 1 mentioned went with the git boolean defintion.
More follow up to: #3774
The configuration of certain windows repos is still having perf issues after the change above. With help from a windows dev the most probably issues were seen as:
The configuration of virtualfilesystem and sparsecheckout causing issues with calculating diffs:
Skip repo info diff for VFS and sparse checkout enlistments #4308
Enough extensive local commits created (especially with renames) that just calculating the number of diffs in the change slows down vscode with expensive git processes
Skip repo info diff when commit count exceeds limit #4309
For this date our usual thing is to prioritize avoiding nasty perf issues at the expense of possibly dropping some data, so I'm implementing both changes here. I also do believe that in most cases of the commit count check we'd be pushing on too big a diff to log in telemetry in any case possibly. As with the other change we are logging why we don't check telemetry so if we start to not even enough events through we can re-examine.
Unit tests also updated.
For my local testing I did the following tests with a local build: