Introduce showAndLogExceptionWithTelemetry

[robertbrignull]

This PR introduces showAndLogExceptionWithTelemetry as an optional replacement for showAndLogErrorMessage when you also want to emit telemetry about the error. It then delegates to showAndLogErrorMessage so it will also show a notification and output the message to the extension logs.

I chose to make a new method instead of adding extra functionality to showAndLogErrorMessage because I want it to be obvious when telemetry is being sent and when it isn't. Another idea I toyed with was make the type of the message be Error | string and only send telemetry if we were given an Error, but I decided against this because it would be easy to accidentally not send telemetry and not realise it.

I think a potential area of contention will be the telemetryErrorType field and the ErrorType type. The problem we're trying to solve here is that we need to easily differentiate and group errors, but we want to make it impossible to accidentally send sensitive information to the telemetry. That's why ErrorType is a fixed set of strings rather than just using the string type, because it stops you building an arbitrary string from unsafe information. We do not want to include things like user paths, database names, or query results in the telemetry.

Another element of the telemetry event is a stack trace. The main worry here is user paths, but thankfully the vscode-telemetry and appinsights libraries are already redacting this information. There isn't really likely to be any other sensitive information in a stack trace, since it consists entirely of file paths and class/method names. The file paths are redacted and the other information comes from the github/vscode-codeql codebase rather than user data.

The final element of telemetry events is an arbitrary set of name/value pairs. The idea here is that sometimes extra arbitrary data will be needed to help investigate and fix errors. While you could obviously include sensitive information here, I think it won't happen accidentally. When a developer adds some extra data to a telemetry event to help debug it, it's a very intentional act and therefore hopefully the developer is thinking about the data they are including.

I'm not 100% happy with ErrorType and the general signature of the showAndLogExceptionWithTelemetry method. While writing this PR I found it quite clunky coming up with names for each of the different errors. However I can't think of a better solution that allows us to group similar errors and make it hard to accidentally send sensitive information. This means sending the error message in the telemetry is not allowed. A possible alternative could be to do all grouping purely on the stack trace, but I fear this will not be consistent enough.

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.

[aeisenberg]

Nice.

[robertbrignull]

I've had a new thought on this today. Should we include the class of the error object in the telemetry? We can do something like e.constructor.name to get the class name. A lot of the time the class will just be Error, but if a library has used a custom error class this could be useful information.

For our own errors we throw internally, we could also make use of custom error classes. We could create a SanitisedError, or whatever we want to call it, where we know the error message doesn't contain personal user information. Or we could have different classes for each error type, though this may be a bit much and too tedious.

[robertbrignull]

I've merged in a branch by @koesie10 and made an adjustment so that RedactableError can now keep track of the original stack trace, so we can avoid passing an extra parameter to the logging method. Is this a legal thing to do with javascript errors?

[aeisenberg]

This looks great. You can ignore my previous comments now that that part of the code has been removed. Still some test failures, but no comments on the code.

[aeisenberg]

I've merged in a branch by @koesie10 and made an adjustment so that RedactableError can now keep track of the original stack trace, so we can avoid passing an extra parameter to the logging method. Is this a legal thing to do with javascript errors?

I don't see why not. I think this is a valid use of template strings (I've never actually seen one IRL before).

[adityasharad]

I've had a new thought on this today. Should we include the class of the error object in the telemetry? We can do something like e.constructor.name to get the class name. A lot of the time the class will just be Error, but if a library has used a custom error class this could be useful information.

For our own errors we throw internally, we could also make use of custom error classes. We could create a SanitisedError, or whatever we want to call it, where we know the error message doesn't contain personal user information. Or we could have different classes for each error type, though this may be a bit much and too tedious.

We do something like this in the CodeQL Action telemetry and IMO it is very useful. You don't need too many error classes, but even the distinction between UserError and other errors is handy.

[adityasharad]

Passing comment: I'm leaving the close review of this PR to the experts, but I am really happy to see the level of care you've put into usefully expanding the telemetry while being explicit about 1) when telemetry is sent and 2) filtering out user information and reducing the risk of mistakes in this area.

[robertbrignull]

Tests are passing, and I'm now happy with the way this works, so this is now ready for another proper review.

[robertbrignull]

But note, I don't expect to merge this PR until we can update the privacy statement. So there's no rush to get this in today.

[aeisenberg]

Nice work.

[robertbrignull]

Thanks @aeisenberg for the review. I've pushed some more small changes. There's now a secret opt-in config flag which stops the new telemetry from being output for normal users for now. So I've converted the error telemetry to be behind this flag too. This means we can merge this now without being blocking by the privacy docs changes.

I've also added some simple tests of the error telemetry because I realised I'd neglected that until now.

[robertbrignull]

Just merged in main, and I'll merge this PR once the tests are green.