Improve extensions telemetry API

[jrieken]

In todays API there are some symbols that help extensions with telemetry, e.g machineId, sessionId, and most importantly isTelemetryEnabled. These are used by our telemetry module and by others.

The existing APIs help you to get the job done but offer little quality of life, aren't always easy to use, and lack hooks that we would like for transparency and cleansing. Our goal is to make it easy for extensions to do the right thing and to be transparent to our user what's happening.

Below is an alternative API that is around a TelemetryLogger and TelemetryAppender. The appender is what does the actual data collecting but it should be used through a logger. With that "indirection" we can do many nice things:

• we can do extra PII cleansing for extensions, e.g remove everything that looks like path-name, URI, user-name etc
• we can enable/disable telemetry per extension
• we echo everything that is logged to the telemetry output channel
• we check enablements: logger.logUsage() -> setting say NO ⛔ -> we don't call appender
• we can call logError on provider-errors and unhandled extension errors (superseeding Allow extensions to provide a global error handler for errors in extension code #45264)

declare module 'vscode' {

	export interface TelemetryLogger {
		readonly onDidChangeEnableStates: Event<TelemetryLogger>;
		readonly isUsageEnabled: boolean;
		readonly isErrorsEnabled: boolean;

                // calls TelemetryAppender#logUsage iff usage telemetry is enabled, echos data to output channel
                // called by extension
		logUsage(...args: any[]): void;

                // calls TelemetryAppender#logError iff error telemetry is enabled, echos data to output channel
                // called by extension
                // ALSO called by VS Code when the owning extension throws, e.g provider or command errors
		logError(err: Error): void;
	}

	export interface TelemetryAppender {
		logUsage(...args: any[]): void;
		logError(err: Error): void;
	}

	export namespace env {
		// BYOTA
		export function createTelemetryLogger(appender: TelemetryAppender): TelemetryLogger;
	}
}

The vscode-extension-telemetry-module would become an instance of a telemetry appender.

[lramos15]

Assigning to September milestone but definitely will not be complete for next release

[isidorn]

Once we get this API in a good proposed state let me know and I can reach out to all extension authors that report telemetry so they slowly start transitioning to this API.

[lramos15]

This API continues to look good and adapt. Hoping to have some in product testing in for debt week. Just missed getting that in this week.

[gjsjohnmurray]

Is per-extension settings going to be part of the initial release?

[lramos15]

Is per-extension settings going to be part of the initial release?

It is not. Just shared output channel, cleaning done by VS code core, and logging of ext host errors caused by your extension to your registered telemetry logger

[bwateratmsft]

I really like this idea, especially unifying all of the PII masking into one better-maintained place. One thing I wanted to bring up, though--there will always be examples of PII that the default filters can't catch. These could be domain-specific things like resource names, or more generic things like an obscure string ID format that isn't well known--perhaps an ID that's unique to a particular country or region.

As such, would it be possible to include a means of adding custom PII filtering logic? e.g. a method or list of methods to pass things through that input a string and output an amended string (or the same string if no changes are needed)?

[lramos15]

As such, would it be possible to include a means of adding custom PII filtering logic? e.g. a method or list of methods to pass things through that input a string and output an amended string (or the same string if no changes are needed)?

I don't think there are any plans now for anything like this. What would you expect this to look like? Is it a regex that can be run on all the values and you pass a regex and a replacement value sort of similar to the ReplacementOptions of the current module? I think right now our story for these advanced cases is that your TelemetryAppender (names subject to change) which you contribute and received a TelemetryLogger back is called last and therefore you can technically intercept and do anything with the data payload after it is returned to you in order to be sent. The downside being we log what is being sent before it goes off to your TelemetryAppender meaning the specialty PII that you're not sending is being logged to the output channel.

[bwateratmsft]

A regex + replacement is one option but it might be more flexible to accept a function that does string in -> string out. I think the issue of it being logged before going to the TelemetryAppender is significant; what the user sees logged, they are going to think is uploaded--and that belief would be reinforced if some of the PII is masked (the stuff the core library knows about), but not the additional things supplied by the extension.

[lramos15]

@bwateratmsft We discussed this today at the API sync and still are having a bit of hard time understanding how this can help.

This would be the current flow. I don't see how moving the your cleaning piece into our portion of the flow helps at all. We don't clean common properties because they're static and therefore should always be void of PII. So therefore we would just be calling your function right after you call log anyways. This means it's better for you to just call your cleaning and then call log.
Your cleaning -> you call log -> our cleaning -> we mix-in common properties -> output channel -> logged to endpoint

[bwateratmsft]

My main motivation is to have all the cleaning happen in one place, even if the logic for what is cleaned is somewhat dispersed. Your approach definitely works, though.

[jrieken]

Done!

[gjsjohnmurray]

@lramos15 @jrieken Please see #171996 (comment)

Do extensions need to be fixed?