fix: Fix race condition with client registration. (#750)

During initialization the js-client-sdk does some context processing.
This is an async operation which will add keys to anonymous contexts.
Being as this doesn't happen synchronously within the initialization
call there is a possibility that you register the telemetry SDK before
that initialization is complete.

This PR updates the registration to attempt to wait for initialization.
This is a larger window than we need to wait, but ensures that process
will be complete.

Author: kinyoklion

packages/telemetry/browser-telemetry/__tests__/BrowserTelemetryImpl.test.ts

@@ -657,3 +657,79 @@ it('only logs error filter error once', () => {
 
   expect(mockLogger.warn).toHaveBeenCalledTimes(1);
 });
+
+it('waits for client initialization before sending events', async () => {
+  const telemetry = new BrowserTelemetryImpl(defaultOptions);
+  const error = new Error('Test error');
+
+  let resolver;
+
+  const initPromise = new Promise((resolve) => {
+    resolver = resolve;
+  });
+
+  const mockInitClient = {
+    track: jest.fn(),
+    waitForInitialization: jest.fn().mockImplementation(() => initPromise),
+  };
+
+  telemetry.captureError(error);
+  telemetry.register(mockInitClient);
+
+  expect(mockInitClient.track).not.toHaveBeenCalled();
+
+  resolver!();
+
+  await initPromise;
+
+  expect(mockInitClient.track).toHaveBeenCalledWith(
+    '$ld:telemetry:session:init',
+    expect.objectContaining({
+      sessionId: expect.any(String),
+    }),
+  );
+
+  expect(mockInitClient.track).toHaveBeenCalledWith(
+    '$ld:telemetry:error',
+    expect.objectContaining({
+      type: 'Error',
+      message: 'Test error',
+      stack: { frames: expect.any(Array) },
+      breadcrumbs: [],
+      sessionId: expect.any(String),
+    }),
+  );
+});
+
+it('handles client initialization failure gracefully', async () => {
+  const telemetry = new BrowserTelemetryImpl(defaultOptions);
+  const error = new Error('Test error');
+  const mockInitClient = {
+    track: jest.fn(),
+    waitForInitialization: jest.fn().mockRejectedValue(new Error('Init failed')),
+  };
+
+  telemetry.captureError(error);
+  telemetry.register(mockInitClient);
+
+  await expect(mockInitClient.waitForInitialization()).rejects.toThrow('Init failed');
+
+  // Should still send events even if initialization fails
+  expect(mockInitClient.track).toHaveBeenCalledWith(
+    '$ld:telemetry:session:init',
+    expect.objectContaining({
+      sessionId: expect.any(String),
+    }),
+  );
+
+  expect(mockInitClient.track).toHaveBeenCalledWith(
+    '$ld:telemetry:error',
+    expect.objectContaining({
+      type: 'Error',
+      message: 'Test error',
+      stack: { frames: expect.any(Array) },
+      breadcrumbs: [],
+      sessionId: expect.any(String),
+    }),
+  );
+});

packages/telemetry/browser-telemetry/src/BrowserTelemetryImpl.ts

@@ -5,7 +5,7 @@
  */
 import type { LDContext, LDEvaluationDetail } from '@launchdarkly/js-client-sdk';
 
-import { LDClientLogging, LDClientTracking, MinLogger } from './api';
+import { LDClientInitialization, LDClientLogging, LDClientTracking, MinLogger } from './api';
 import { Breadcrumb, FeatureManagementBreadcrumb } from './api/Breadcrumb';
 import { BrowserTelemetry } from './api/BrowserTelemetry';
 import { BrowserTelemetryInspector } from './api/client/BrowserTelemetryInspector';
@@ -34,6 +34,12 @@ const GENERIC_EXCEPTION = 'generic';
 const NULL_EXCEPTION_MESSAGE = 'exception was null or undefined';
 const MISSING_MESSAGE = 'exception had no message';
 
+// Timeout for client initialization. The telemetry SDK doesn't require that the client be initialized, but it does
+// require that the context processing that happens during initialization complete. This is some subset of the total
+// initialization time, but we don't care if initialization actually completes within the, just that the context
+// is available for event sending.
+const INITIALIZATION_TIMEOUT = 5;
+
 /**
  * Given a flag value ensure it is safe for analytics.
  *
@@ -83,6 +89,10 @@ function isLDClientLogging(client: unknown): client is LDClientLogging {
   return (client as any).logger !== undefined;
 }
 
+function isLDClientInitialization(client: unknown): client is LDClientInitialization {
+  return (client as any).waitForInitialization !== undefined;
+}
+
 export default class BrowserTelemetryImpl implements BrowserTelemetry {
   private _maxPendingEvents: number;
   private _maxBreadcrumbs: number;
@@ -98,6 +108,10 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry {
 
   private _logger: MinLogger;
 
+  private _registrationComplete: boolean = false;
+
+  // Used to ensure we only log the event dropped message once.
+  private _clientRegistered: boolean = false;
   // Used to ensure we only log the event dropped message once.
   private _eventsDropped: boolean = false;
   // Used to ensure we only log the breadcrumb filter error once.
@@ -159,17 +173,45 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry {
   }
 
   register(client: LDClientTracking): void {
+    if (this._client !== undefined) {
+      return;
+    }
+
     this._client = client;
+
     // When the client is registered, we need to set the logger again, because we may be able to use the client's
     // logger.
     this._setLogger();
 
-    this._client.track(SESSION_INIT_KEY, { sessionId: this._sessionId });
+    const completeRegistration = () => {
+      this._client?.track(SESSION_INIT_KEY, { sessionId: this._sessionId });
 
-    this._pendingEvents.forEach((event) => {
-      this._client?.track(event.type, event.data);
-    });
-    this._pendingEvents = [];
+      this._pendingEvents.forEach((event) => {
+        this._client?.track(event.type, event.data);
+      });
+      this._pendingEvents = [];
+      this._registrationComplete = true;
+    };
+
+    if (isLDClientInitialization(client)) {
+      // We don't actually need the client initialization to complete, but we do need the context processing that
+      // happens during initialization to complete. This time will be some time greater than that, but we don't
+      // care if initialization actually completes within the timeout.
+
+      // An immediately invoked async function is used to ensure that the registration method can be called synchronously.
+      // Making the `register` method async would increase the complexity for application developers.
+      (async () => {
+        try {
+          await client.waitForInitialization(INITIALIZATION_TIMEOUT);
+        } catch {
+          // We don't care if the initialization fails.
+        }
+        completeRegistration();
+      })();
+    } else {
+      // TODO(EMSR-36): Figure out how to handle the 4.x implementation.
+      completeRegistration();
+    }
   }
 
   private _setLogger() {
@@ -207,7 +249,9 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry {
       return;
     }
 
-    if (this._client === undefined) {
+    if (this._registrationComplete) {
+      this._client?.track(type, filteredEvent);
+    } else {
       this._pendingEvents.push({ type, data: filteredEvent });
       if (this._pendingEvents.length > this._maxPendingEvents) {
         if (!this._eventsDropped) {
@@ -221,7 +265,6 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry {
         this._pendingEvents.shift();
       }
     }
-    this._client?.track(type, filteredEvent);
   }
 
   captureError(exception: Error): void {

packages/telemetry/browser-telemetry/src/api/client/LDClientInitialization.ts

@@ -0,0 +1,6 @@
+/**
+ * Minimal client interface which allows waiting for initialization.
+ */
+export interface LDClientInitialization {
+  waitForInitialization(timeout?: number): Promise<void>;
+}

packages/telemetry/browser-telemetry/src/api/client/index.ts

@@ -1,3 +1,4 @@
 export * from './LDClientTracking';
 export * from './LDClientLogging';
 export * from './BrowserTelemetryInspector';
+export * from './LDClientInitialization';