Add telemetry for exporting variant analysis results

[robertbrignull]

This came about from wanting to deduplicate the usage of the codeQL.exportVariantAnalysisResults command, however I came to the conclusion we should keep the use of this command and instead add extra telemetry to differentiate different usages of it.

The reason I'm not proposing following the approach used in #2179 is because the codeQL.exportVariantAnalysisResults has a progress meter. This means it's not trivial to inline its implementation, because we'd have to call withProgress and this seems non-ideal as that method is only meant to be used from a command.

We call this command from three places:

• From the codeQLQueryHistory.exportResults command, so this case already has telemetry
• From the codeQL.exportSelectedVariantAnalysisResults command, so this case already has telemetry
• From the exportResults webview message

So by adding telemetry to the variant analysis results view we now have telemetry every place that triggers the codeQL.exportVariantAnalysisResults command. This means the telemetry on the command itself is not needed to differentiate user actions and we can accept that it's used from multiple places.

Does this reasoning make sense? Would it be better to instead use the approach from #2179 but work around it being a command with progress?

If this PR is merged, I can dismiss the code scanning alert for codeQL.exportVariantAnalysisResults and link to this PR in the dismissal comment.

Checklist

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[koesie10]

Could we instead move the withProgress call to be inside the exportVariantAnalysisResults function? Then we'd still only need to call this function, and don't need to inline any of the withProgress calls. I'd say that it's easier to understand if all commands are only called from 1 place (or none at all), rather than there being 1 or a few commands that are called from separate places.

[robertbrignull]

Just to confirm, here's what the situation looks like currently, with commands and progress and general code paths:

Thinking about this more this morning I'm inclined to now agree with you that it's ok to explicitly use withProgress and this is a justified use of it outside of commandRunnerWithProgress.

So in terms of changes to make, we remove the codeQL.exportVariantAnalysisResults command, and all uses of it instead call the exportVariantAnalysisResults function directly. We then move the use of withProgress to be inside exportVariantAnalysisResults.

I'm also wondering if we should move the "progress" bits out of commandRunner.ts. They're not actually strictly related, and it would make things like this a bit cleaner. That wouldn't be part of this PR, but I might look at opening another after this to get thoughts on that change.

[robertbrignull]

Current problems/thoughts I'm having with this PR:

• We can't call exportVariantAnalysisResults from VariantAnalysisView because it takes a VariantAnalysisManager as an argument. In the view we only are given a VariantAnalysisViewManager interface to avoid a circular dependency.
  • One option is to add a exportResults method to VariantAnalysisManager that then calls exportVariantAnalysisResults. The main goal of this indirection is to avoid adding more code to VariantAnalysisManager.
• I'm getting a little hung up on the exportSelectedVariantAnalysisResults which lives in export-results.ts which just calls variantAnalysisManager.exportResults which then delegates back to exportVariantAnalysisResults. I'm wondering if we can remove exportSelectedVariantAnalysisResults and merge it with handleExportResults from QueryHistoryManager. But I'm thinking I should just ignore that for this PR.
• We need to keep the telemetry when exporting results from within the view. We're now removing the command so it's even more important.

[robertbrignull]

I've pushed some code but I'm not 100% sure of it. There's other stuff that needs doing but I'll come back later and have another go at this.

[koesie10]

I think the code as-is looks good and correct. Like you mentioned there are some things that we can clean up, but we can also do this in follow-up PRs.

[robertbrignull]

Yeah, let's just go for it. We can do more cleanup is followups.

The remaining things I wanted to look at are:

• Should the exportSelectedVariantAnalysisResults method be moved to either the query history or variant analysis managers?
• Should the withProgress method and associated code be moved out of commandRunner.ts?