Switch to VS Code's telemetry classes #4377

Activity

1. DanTup

   added

   is non-functional

   on Feb 6, 2023
2. DanTup

   added this to the On Deck milestone on Feb 6, 2023
5. DanTup commented on Mar 9, 2023

   

   DanTup

   on Mar 9, 2023

   MemberAuthor

   More actions

   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.
8. DanTup commented on Mar 23, 2023

   

   DanTup

   on Mar 23, 2023

   MemberAuthor

   More actions

   @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?
10. DanTup commented on Mar 23, 2023

    

    DanTup

    on Mar 23, 2023

    MemberAuthor

    More actions

    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 :-)
12. DanTup commented on Mar 23, 2023

    

    DanTup

    on Mar 23, 2023

    Last edited by DanTup

    MemberAuthor

    More actions

    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.
14. DanTup commented on Mar 23, 2023

    

    DanTup

    on Mar 23, 2023

    MemberAuthor

    More actions

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

    modified the milestones: On Deck, v3.64.0 on Mar 23, 2023
17. DanTup

    added

    is enhancementAn enhancement or improvement that should be listed in release notes but is not a bug fix.An enhancement or improvement that should be listed in release notes but is not a bug fix.

    and removed

    is non-functional

    on Mar 23, 2023
18. DanTup

    modified the milestones: v3.64.0, v3.62.0 on Mar 29, 2023
19. DanTup commented on Mar 29, 2023

    

    DanTup

    on Mar 29, 2023

    MemberAuthor

    More actions

    Done via #4457 :-)
20. DanTup

    closed this as completedon Mar 29, 2023