fix telemetry in browser extension comlink

- Need to just call recordEvent instead of pass the entire TelemetryRecorder object across the Comlink interface.
- Comlink's `Remote<...>` generic type struggles with the generics on the `TelemetryRecorder.recordEvent`, so just use strings.

Author: sqs

client/browser/BUILD.bazel

@@ -206,7 +206,6 @@ ts_project(
         "src/shared/platform/inlineExtensionsService.ts",
         "src/shared/platform/ports.ts",
         "src/shared/platform/settings.ts",
-        "src/shared/platform/worker.ts",
         "src/shared/polyfills.ts",
         "src/shared/repo/backend.tsx",
         "src/shared/repo/index.tsx",

client/shared/src/api/client/mainthread-api.ts

@@ -1,5 +1,5 @@
 import { type Remote, proxy } from 'comlink'
-import { type Unsubscribable, Subscription, from, of, lastValueFrom, ReplaySubject } from 'rxjs'
+import { type Unsubscribable, from, lastValueFrom, of, ReplaySubject, Subscription } from 'rxjs'
 import { share, switchMap } from 'rxjs/operators'
 
 import { logger } from '@sourcegraph/common'
@@ -141,7 +141,7 @@ export const initMainThreadAPI = (
             return proxySubscribable(of([]))
         },
         logEvent: (eventName, eventProperties) => platformContext.telemetryService?.log(eventName, eventProperties),
-        getTelemetryRecorder: () => platformContext.telemetryRecorder,
+        recordEvent: platformContext.telemetryRecorder.recordEvent as MainThreadAPI['recordEvent'],
         logExtensionMessage: (...data) => logger.log(...data),
     }
 

client/shared/src/api/contract.ts

@@ -1,22 +1,22 @@
-import type { Remote, ProxyMarked } from 'comlink'
+import type { ProxyMarked, Remote } from 'comlink'
 import type { Unsubscribable } from 'rxjs'
 
 import type {
     Contributions,
     Evaluated,
+    HoverMerged,
     Raw,
     TextDocumentPositionParameters,
-    HoverMerged,
 } from '@sourcegraph/client-api'
 import type { MaybeLoadingResult } from '@sourcegraph/codeintellify'
 import type * as clientType from '@sourcegraph/extension-api-types'
 import type { GraphQLResult } from '@sourcegraph/http-client'
+import type { KnownKeys, TelemetryEventParameters } from '@sourcegraph/telemetry'
 
 import type { DocumentHighlight, ReferenceContext } from '../codeintel/legacy-extensions/api'
 import type { Occurrence } from '../codeintel/scip'
 import type { ConfiguredExtension } from '../extensions/extension'
 import type { SettingsCascade } from '../settings/settings'
-import type { TelemetryV2Props } from '../telemetry'
 
 import type { SettingsEdit } from './client/services/settings'
 import type { ExecutableExtension } from './extension/activation'
@@ -75,7 +75,6 @@ export interface FlatExtensionHostAPI extends CodeIntelExtensionHostAPI {
     /**
      * Sets the given context keys and values.
      * If a value is `null`, the context key is removed.
-     *
      * @param update Object with context keys as values
      */
     updateContext: (update: { [k: string]: unknown }) => void
@@ -89,7 +88,6 @@ export interface FlatExtensionHostAPI extends CodeIntelExtensionHostAPI {
     /**
      * Returns an observable that emits all contributions (merged) evaluated in the current model
      * (with the optional scope). It emits whenever there is any change.
-     *
      * @template T Extra allowed property value types for the {@link Context} value. See
      * {@link Context}'s `T` type parameter for more information.
      * @param scope The scope in which contributions are fetched. A scope can be a sub-component of
@@ -111,7 +109,6 @@ export interface FlatExtensionHostAPI extends CodeIntelExtensionHostAPI {
 
     /**
      * Add a viewer.
-     *
      * @param viewer The description of the viewer to add.
      * @returns The added code viewer (which must be passed as the first argument to other
      * viewer methods to operate on this viewer).
@@ -125,7 +122,6 @@ export interface FlatExtensionHostAPI extends CodeIntelExtensionHostAPI {
 
     /**
      * Sets the selections for a CodeEditor.
-     *
      * @param codeEditor The editor for which to set the selections.
      * @param selections The new selections to apply.
      * @throws if no editor exists with the given editor ID.
@@ -136,7 +132,6 @@ export interface FlatExtensionHostAPI extends CodeIntelExtensionHostAPI {
     /**
      * Removes a viewer.
      * Also removes the corresponding model if no other viewer is referencing it.
-     *
      * @param viewer The viewer to remove.
      */
     removeViewer(viewer: ViewerId): void
@@ -176,15 +171,29 @@ export interface MainThreadAPI {
 
     /**
      * Log an event (by sending it to the server).
-     *
-     * @deprecated use getTelemetryRecorder().recordEvent instead
+     * @deprecated use {@link MainThreadAPI.recordEvent} instead
      */
     logEvent: (eventName: string, eventProperties?: any) => void
 
     /**
-     * Get a TelemetryRecorder for recording telemetry events to the server.
+     * Record a telemetry event.
+     *
+     * The type signature should be kept in sync with {@link TelemetryRecorder.recordEvent}.
      */
-    getTelemetryRecorder: () => TelemetryV2Props['telemetryRecorder']
+    recordEvent: (
+        feature: string,
+        action: string,
+        parameters?: TelemetryEventParameters<
+            KnownKeys<
+                string,
+                {
+                    [key in string]: number
+                }
+            >,
+            string,
+            string
+        >
+    ) => void
 
     /**
      * Log messages from extensions in the main thread. Makes it easier to debug extensions for applications

client/shared/src/api/extension/activation.ts

@@ -4,7 +4,7 @@ import { catchError, concatMap, distinctUntilChanged, first, map, switchMap, tap
 import type sourcegraph from 'sourcegraph'
 
 import type { Contributions } from '@sourcegraph/client-api'
-import { asError, isErrorLike, hashCode, logger } from '@sourcegraph/common'
+import { asError, hashCode, isErrorLike, logger } from '@sourcegraph/common'
 
 import {
     type ConfiguredExtension,
@@ -66,18 +66,17 @@ const DEPRECATED_EXTENSION_IDS = new Set(['sourcegraph/code-stats-insights', 'so
 
 export function activateExtensions(
     state: Pick<ExtensionHostState, 'activeExtensions' | 'contributions' | 'haveInitialExtensionsLoaded' | 'settings'>,
-    mainAPI: Remote<Pick<MainThreadAPI, 'logEvent' | 'getTelemetryRecorder'>>,
+    mainAPI: Remote<Pick<MainThreadAPI, 'logEvent' | 'recordEvent'>>,
     createExtensionAPI: (extensionID: string) => typeof sourcegraph,
-    mainThreadAPIInitializations: Observable<boolean>,
     /**
      * Function that activates an extension.
      * Returns a promise that resolves once the extension is activated.
-     * */
+     */
     activate = activateExtension,
     /**
      * Function that de-activates an extension.
      * Returns a promise that resolves once the extension is de-activated.
-     * */
+     */
     deactivate = deactivateExtension
 ): Subscription {
     const previouslyActivatedExtensions = new Set<string>()
@@ -168,8 +167,7 @@ export function activateExtensions(
                                                 .catch(() => {
                                                     // noop
                                                 })
-                                            const telemetryRecorder = await mainAPI.getTelemetryRecorder()
-                                            telemetryRecorder.recordEvent('blob.extension', 'activate', {
+                                            mainAPI.recordEvent('blob.extension', 'activate', {
                                                 privateMetadata: { extensionID: telemetryExtensionID },
                                             })
                                         } catch (error) {
@@ -365,7 +363,6 @@ export interface ExecutableExtension extends Pick<ConfiguredExtension, 'id' | 'm
  *
  * Because `require` is replaced on each extension activation with the API created for that extension,
  * the API can only be imported once to prevent extensions importing APIs created for other extensions.
- *
  * @param extensionAPI The extension API instance for the extension to be activated.
  * @throws error to give extension authors feedback if they try to import an API instance that was
  * already imported (e.g. if they asynchronously import the extension API and the current `require` was

client/shared/src/api/extension/extensionHost.ts

@@ -35,7 +35,6 @@ export interface InitData {
  *
  * It expects to receive a message containing {@link InitData} from the client application as the
  * first message.
- *
  * @param endpoints The endpoints to the client.
  * @returns An unsubscribable to terminate the extension host.
  */
@@ -74,7 +73,6 @@ export function startExtensionHost(
  *
  * The extension API is made globally available to all requires/imports of the "sourcegraph" module
  * by other scripts running in the same JavaScript context.
- *
  * @param endpoints The endpoints to the client.
  * @param initData The information to initialize this extension host.
  * @returns An unsubscribable to terminate the extension host.
@@ -137,7 +135,7 @@ function createExtensionAndExtensionHostAPIs(
     const createExtensionAPI = createExtensionAPIFactory(extensionHostState, proxy, initData)
 
     // Activate extensions. Create extension APIs on extension activation.
-    subscription.add(activateExtensions(extensionHostState, proxy, createExtensionAPI, mainThreadAPIInitializations))
+    subscription.add(activateExtensions(extensionHostState, proxy, createExtensionAPI))
 
     // Observe settings and update active loggers state
     subscription.add(setActiveLoggers(extensionHostState))

client/shared/src/api/extension/test/activation.test.ts

@@ -1,4 +1,4 @@
-import { BehaviorSubject, firstValueFrom, of } from 'rxjs'
+import { BehaviorSubject, firstValueFrom } from 'rxjs'
 import { filter, first } from 'rxjs/operators'
 import sinon from 'sinon'
 import type sourcegraph from 'sourcegraph'
@@ -18,9 +18,9 @@ describe('Extension activation', () => {
         it('logs events for activated extensions', async () => {
             const logEvent = sinon.spy()
 
-            const mockMain = pretendRemote<Pick<MainThreadAPI, 'logEvent' | 'getTelemetryRecorder'>>({
+            const mockMain = pretendRemote<Pick<MainThreadAPI, 'logEvent' | 'recordEvent'>>({
                 logEvent,
-                getTelemetryRecorder: () => noOpTelemetryRecorder,
+                recordEvent: noOpTelemetryRecorder.recordEvent as MainThreadAPI['recordEvent'],
             })
 
             const FIXTURE_EXTENSION: ExecutableExtension = {
@@ -53,8 +53,6 @@ describe('Extension activation', () => {
                 function createExtensionAPI() {
                     return {} as typeof sourcegraph
                 },
-                of(true),
-                noopPromise,
                 noopPromise
             )