Add the CLI version to telemetry events

[aeisenberg]

This adds the CLI version to telemetry command-usage events.

Note that the CLI server is created after the telemetry listener is created. The first few telemetry events may have a "not-set" value for the CLI version.

Checklist

• [n/a] 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.
• [n/a] [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]

Thanks for adding this, this will definitely be useful!

I'm somewhat concerned about the performance impact this will have on sending commands. If there is no version available, this will spawn a process to get the CLI version, which might delay the telemetry and possibly skip sending the telemetry if VS Code gets closed early.

I also think the CLI version would be useful on other telemetry events (especially errors). I think it would be good if we could use a addTelemetryProcessor to add the CLI version tag to every telemetry event we send. The telemetry processor is not async, but we could use the cached CLI version here. That would also solve my concern about performance. We should already always be retrieving the version in the CLI server (since it's also used by CLI version constraints and the status bar), so I think this would be a very viable option for adding the CLI version to all telemetry events without the possibility of spawning a process on every telemetry event.

[robertbrignull]

I agree with Koen's points. Calculating the CLI version while handling the telemetry event doesn't seem great, both in terms of performance and the chance of errors happening and stopping us sending telemetry. Could we set the CLI version once, and then maybe update it if the CLI changes? I also agree it would be nice to have this on all events, like how we include isCanary.

[aeisenberg]

Thanks for the feedback. I will change my approach. I'll add a callback to the CliServer that gets invoked whenever the CLI version changes. There will still be a time at startup when the cli version is not known since the telemetry listener is instantiated before the cli server (sometimes this takes a while).

I can also add the cli version to all telemetry events.

[aeisenberg]

I think I addressed all the comments.

[aeisenberg]

Not sure why I'm getting a failing test:

https://github.com/github/vscode-codeql/actions/runs/4693003381/jobs/8320617631#step:9:69

It doesn't seem related to my changes, but it is repeatable in CI.

Looks like this job on main failed in a similar way: https://github.com/github/vscode-codeql/actions/runs/4690867136/jobs/8314564124

[aeisenberg]

Maybe updating the branch will help.

[aeisenberg]

Nope.

[robertbrignull]

I'd guess we have some tests that are very close to the timeout value, and they've just tipped over recently 😢

[robertbrignull]

We have an internal issue about tests timing out and I've mentioned this there. Either we should look at making the tests faster or raising the timeout value.

[koesie10]

LGTM, thanks for addressing our concerns!

[robertbrignull]

LGTM too

[aeisenberg]

Here's my analysis:

The first error was indeed a timeout error:

    thrown: "Exceeded timeout of 20000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

The tests re-ran and somewhere during the retry, the original query finally completed, but the callback failed since the connection had already been disposed. So, I think increasing the timeout will work, and we should also figure out a way to properly cancel a query in the teardown.

Also, I found another error where removeAllDatabases throws. This method is called during teardown and makes a call to the query server to de-register the database. If the query server is already shut down, then an error will be thrown. But if the query server is shut down, then deregistering isn't necessary, so it's ok to ignore.

I will put a fix in for this and bump the timeouts. Both will be in a separate PR.

[aeisenberg]

I attempt to address this and some related concerns here: #2331