Switch to VS Code's telemetry classes

[DanTup]

https://code.visualstudio.com/updates/v1_75#_telemetry

This simplifies things be handling opt-in/opt-out and ensuring no user info leaks into the data. Adopting it requires bumping he required VS Code version though.

[ghost]

#4425

[ghost]

@DanTup since our Analytics class has many different methods/events, but TelemetrySender has only 2 generic methods (sendEventData and sendErrorData), would it be ok if each Analytics method is turned into an event? What do you think?

Should we keep the Analytics class or have just the TelemetryLogger?

[DanTup]

since our Analytics class has many different methods/events, but TelemetrySender has only 2 generic methods (sendEventData and sendErrorData), would it be ok if each Analytics method is turned into an event? What do you think?

Those methods are just helpers for stronger typing from the calling code. They all call an event() method which converts everything into a map anyway.

So I think the Analytics class (which should take the TelemetryLogger returned from createTelemetryLogger()) should remain, and it should retain its logXxx methods, but the implementation of the current event() method would now call telemetryLogger.logUsage() instead of sending directly to GA.

Then the TelemetrySender (which was passed to createTelemetryLogger()) in its sendEventData method, would have the implementation of sending to GA.

By keeping the Analytics wrapper and having specific logXxx wrapper methods, it's easy to see exactly what analytics are being captured just from that one class, rather than lots of logUsage() calls scattered around the extension.

[ghost]

Got it, thanks!

[ghost]

There is a problem with this approach, the TelemetrySender class forces us to either repeat the send method on both methods or define it outside (globally) which is dirty. OR define it in the class and call the class from TelemetrySender. What do you think?

[DanTup]

@aitor-gomila I'm not sure exactly which method you mean. The idea I had (but I haven't tried it out) was something like this:

class Analytics {
	constructor(private readonly telemetryLogger: TelemetryLogger) {}

	// existing public methods
	public logFoo() {
		this.event(Category.Extension, EventAction.Activated);
	}

	private event(category: Category, action: Action) { 
		// This now calls the telemetry logger instead of doing the HTTP request itself
		this.telemetryLogger.logUsage(
			category,
			{
				"action": action,
				// + other fields currently added by Analytics.send() (settings etc.)
			},
		);
	}
}

// New sender class that does the HTTP requests
class GoogleAnalyticsTelemetrySender extends TelemetrySender {
	public sendEventData(eventName: string, data?: Record<string, any>) { 
		// existing code that's currently in Analytics.send() that
		// does the HTTP request, but is only using data supplied in this method
		// it doesn't read any config etc. (that's all done in Analytics.event() above
		// so that if VS Code shows the data to the user, they see the full set of data
		// being recorded)
	}
}




/// setup in extension.ts
const analytics = new Analytics(createTelemetryLogger(new GoogleAnalyticsTelemetrySender()));
analytics.logFoo();

Does this seem feasible?

[ghost]

Yes, this is what I was thinking. The problem is TelemetrySender/TelemetryLogger have 2 methods, sendEventData and sendErrorData. It is a good practice to send the errors using sendErrorData, but that would mean having to repeat the code for sending data to GAnalytics, or some kind of super.send() code?

[DanTup]

We could still have a private method instead TelemetrySender that both sendEventData and sendErrorData could call.

However, we don't actually use errors anymore. The only code that calls to log errors is in the legacy analysis server protocol, which is hardly used for anyone. I would just leave sendErrorData empty and not implement anything for errors. You can delete the logError method from Analytics and the single line that calls it :-)

[ghost]

Got it. Appreciate the clarification :)

Tried to create a private send method inside TelemetrySender but the compiler complained.

I actually think not logging errors is not a bad idea, but if we ever need them, we'll need to think about this issue, or just add an additional event type for errors. The only difference being how VSCode displays that.

[DanTup]

Tried to create a private send method inside TelemetrySender but the compiler complained.

Odd. I tested it here and this seemed ok:

class Foo implements TelemetrySender {
	public sendEventData(eventName: string, data?: Record<string, any> | undefined): void {
		throw new Error("Method not implemented.");
	}
	public sendErrorData(error: Error, data?: Record<string, any> | undefined): void {
		throw new Error("Method not implemented.");
	}

	private foo() {

	}

}

const sender = new Foo();
const logger = env.createTelemetryLogger(sender);

If you have issues though, let me know (although it's probably not important now if we just put the sending in sendEventData for now).

I actually think not logging errors is not a bad idea, but if we ever need them, we'll need to think about this issue, or just add an additional event type for errors.

Yeah, this can be something to consider for later. Right now we're not really capturing them so we shouldn't hold up migrating to the new APIs to change that.

[ghost]

Are you typechecking that code? Need to check that again.

[DanTup]

Yep, I initially had errors because I was trying to extend instead of implement but it's only an interface.

[ghost]

That's it then. It makes a lot of sense now.

[DanTup]

Done via #4457 :-)