Remove codeQL.openVariantAnalysisQueryFile command

[robertbrignull]

I'm not sure if this PR is the right way to go, but I want to open it to ask the question. What's wrong with doing this?

To make clear what's going on here, the codeQL.openVariantAnalysisQueryFile is only used internally from code and is not available to the user in the command palette. In the two places we use this command, we could instead just call the openQueryFile method on the variant analysis manager directly. We have access to the variant analysis manager object already.

The reasons I can see for using a command are:

• It reduces coupling between the variant analysis manager and other parts of the codebase, but in this case the coupling is already there and unlikely to be removed.
• It adds more error handling/logging, but I don't think it adds much because we're always already either in a command or handling a webview message.

So does this PR actually make things any worse? It seems unclear to me. But it might still be working against our view of the perfect future.

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.

[elenatanasoiu]

I don't what the perfect future looks like, but if we're only calling this from code, I think you're right to nix it.

[koesie10]

Do we have separate telemetry that lets us know how many times a user has opened a variant analysis query file? It seems like in the query history manager, we don't have a good way of distinguishing between a local query and a variant analysis in the telemetry. It also doesn't seem like separate telemetry is being send when "View query" is clicked in the view.

I'm not sure if we need any telemetry for this command, but I think we do need to think about that.

[charisk]

Agree, I don't see a huge benefit in keeping the command, just decouples things a bit.

[robertbrignull]

Do we have separate telemetry that lets us know how many times a user has opened a variant analysis query file? It seems like in the query history manager, we don't have a good way of distinguishing between a local query and a variant analysis in the telemetry. It also doesn't seem like separate telemetry is being send when "View query" is clicked in the view.

Thanks for raising this point!

The state of telemetry right now is:

• Opening a variant analysis query file from the query history:
  • Command usage: codeQLQueryHistory.openQuery
  • Command usage: codeQL.openVariantAnalysisQueryFile
• Opening a local query file from the query history:
  • Command usage: codeQLQueryHistory.openQuery
• Opening a variant analysis query file from the view:
  • Command usage: codeQL.openVariantAnalysisQueryFile
• Opening a local query file from the view:
  • UI interaction: local-results-open-query-file

So you're right if we remove codeQL.openVariantAnalysisQueryFile then we won't be able to tell when a variant analysis query is opened from the view. My preferred solution to this would be to add a UI interaction telemetry event, so then it matches opening a local query from the view.

Being able to differentiate the query type when opened from the query history is a bit more complex. I'd argue we already can't tell, because we can't correlate two telemetry events together to know that one codeQLQueryHistory.openQuery called a codeQL.openVariantAnalysisQueryFile, and codeQL.openVariantAnalysisQueryFile could be called from multiple places. If we want to solve this I'd suggest adding separate commands for them, and using the context value to show different commands for local vs remote queries. I'd also argue it's not too important, because what we want to know from this telemetry event is that people are using the button in the UI, and we can tell usage of local vs remote queries from other metrics.

[robertbrignull]

I've updated the PR to emit a UI interaction telemetry event when opening the variant analysis query file from the view. But I'm still open to discuss options.

This has raised concerns that it's easy to accidentally remove telemetry, because sometimes it's not clear whether an action is covered by the command telemetry or other more dedicated telemetry. I suppose we mainly only have to think about this when removing a command, but still I wonder if we need a more principled approach if we want to be more sure we're not making a mistake.