Merge pull request #692 from brizental/1728128-restarted-webext-qt-verify

Bug 1728128 - Ensure events database is initialized at a time when metrics can be recorded

Author: Beatriz Rizental

CHANGELOG.md

@@ -13,8 +13,9 @@
 * [#661](https://github.com/mozilla/glean.js/pull/661): Include unminified version of library on Qt/QML builds.
 * [#647](https://github.com/mozilla/glean.js/pull/647): Implement the Text metric type.
 * [#681](https://github.com/mozilla/glean.js/pull/681): BUGFIX: Fix error in scanning events database upon initialization on Qt/QML.
-  * This bug prevents the changes introduced in [#526](https://github.com/mozilla/glean.js/pull/526) from working properly.
-* [#614](https://github.com/mozilla/glean.js/pull/614): Implement the String List metric type.
+  * This bug prevents the changes introduced in [#526](https://github.com/mozilla/glean.js/pull/526) from working properly in Qt/QML.
+* [#692](https://github.com/mozilla/glean.js/pull/692): BUGFIX: Ensure events database is initialized at a time Glean is already able to record metrics.
+  * This bug also prevents the changes introduced in [#526](https://github.com/mozilla/glean.js/pull/526) from working properly in all platforms.
 
 # v0.18.1 (2021-07-22)
 

glean/src/core/context.ts

@@ -8,6 +8,9 @@ import type EventsDatabase from "./metrics/events_database/index.js";
 import type PingsDatabase from "./pings/database.js";
 import type ErrorManager from "./error/index.js";
 import Dispatcher from "./dispatcher.js";
+import log, { LoggingLevel } from "./log.js";
+
+const LOG_TAG = "core.Context";
 
 /**
  * This class holds all of the Glean singleton's state and internal dependencies.
@@ -22,30 +25,29 @@ import Dispatcher from "./dispatcher.js";
  * which only matter for Typescript and don't cause circular dependency issues.
  */
 export class Context {
-  private static _instance: Context;
+  private static _instance?: Context;
 
-  private _dispatcher!: Dispatcher | null;
+  private _dispatcher: Dispatcher;
 
+  // The following group of properties are all set on Glean.initialize
+  // Attempting to get them before they are set will log an error.
   private _uploadEnabled!: boolean;
   private _metricsDatabase!: MetricsDatabase;
   private _eventsDatabase!: EventsDatabase;
   private _pingsDatabase!: PingsDatabase;
-  // The reason this was added to the Context,
-  // is to avoid a circular dependency between
-  // the ErrorManager's module and the CounterMetricType module.
   private _errorManager!: ErrorManager;
-
   private _applicationId!: string;
-  private _initialized: boolean;
-
   private _debugOptions!: DebugOptions;
 
+  private _initialized: boolean;
+
   // The moment the current Glean.js session started.
   private _startTime: Date;
 
   private constructor() {
     this._initialized = false;
     this._startTime = new Date();
+    this._dispatcher = new Dispatcher();
   }
 
   static get instance(): Context {
@@ -61,39 +63,25 @@ export class Context {
    *
    * Resets the Context to an uninitialized state.
    */
-  static async testUninitialize(): Promise<void> {
-    // Clear the dispatcher queue
-    // and return the dispatcher back to an uninitialized state.
-    if (Context.instance._dispatcher) {
-      await Context.instance._dispatcher.testUninitialize();
-    }
-
-    // Due to the test requirement of keeping storage in place,
-    // we can't simply wipe out the full `Context` instance.
-    // The closest thing we can do is making the dispatcher `null`.
-    Context.instance._dispatcher = null;
-
-    Context.initialized = false;
-    Context._instance._startTime = new Date();
+  static testUninitialize(): void {
+    Context._instance = undefined;
   }
 
   static get dispatcher(): Dispatcher {
-    // Create a dispatcher if one isn't available already.
-    // This is required since the dispatcher may be used
-    // earlier than Glean initialization, so we can't rely
-    // on `Glean.initialize` to set it.
-    if (!Context.instance._dispatcher) {
-      Context.instance._dispatcher = new Dispatcher();
-    }
-
     return Context.instance._dispatcher;
   }
 
-  static set dispatcher(dispatcher: Dispatcher) {
-    Context.instance._dispatcher = dispatcher;
-  }
-
   static get uploadEnabled(): boolean {
+    if (typeof Context.instance._uploadEnabled === "undefined") {
+      log(
+        LOG_TAG,
+        [
+          "Attempted to access Context.uploadEnabled before it was set. This may cause unexpected behaviour.",
+        ],
+        LoggingLevel.Error
+      );
+    }
+
     return Context.instance._uploadEnabled;
   }
 
@@ -102,6 +90,16 @@ export class Context {
   }
 
   static get metricsDatabase(): MetricsDatabase {
+    if (typeof Context.instance._metricsDatabase === "undefined") {
+      log(
+        LOG_TAG,
+        [
+          "Attempted to access Context.metricsDatabase before it was set. This may cause unexpected behaviour.",
+        ],
+        LoggingLevel.Error
+      );
+    }
+
     return Context.instance._metricsDatabase;
   }
 
@@ -110,6 +108,16 @@ export class Context {
   }
 
   static get eventsDatabase(): EventsDatabase {
+    if (typeof Context.instance._eventsDatabase === "undefined") {
+      log(
+        LOG_TAG,
+        [
+          "Attempted to access Context.eventsDatabase before it was set. This may cause unexpected behaviour.",
+        ],
+        LoggingLevel.Error
+      );
+    }
+
     return Context.instance._eventsDatabase;
   }
 
@@ -118,6 +126,16 @@ export class Context {
   }
 
   static get pingsDatabase(): PingsDatabase {
+    if (typeof Context.instance._pingsDatabase === "undefined") {
+      log(
+        LOG_TAG,
+        [
+          "Attempted to access Context.pingsDatabase before it was set. This may cause unexpected behaviour.",
+        ],
+        LoggingLevel.Error
+      );
+    }
+
     return Context.instance._pingsDatabase;
   }
 
@@ -126,6 +144,16 @@ export class Context {
   }
 
   static get errorManager(): ErrorManager {
+    if (typeof Context.instance._errorManager === "undefined") {
+      log(
+        LOG_TAG,
+        [
+          "Attempted to access Context.errorManager before it was set. This may cause unexpected behaviour.",
+        ],
+        LoggingLevel.Error
+      );
+    }
+
     return Context.instance._errorManager;
   }
 
@@ -134,6 +162,16 @@ export class Context {
   }
 
   static get applicationId(): string {
+    if (typeof Context.instance._applicationId === "undefined") {
+      log(
+        LOG_TAG,
+        [
+          "Attempted to access Context.applicationId before it was set. This may cause unexpected behaviour.",
+        ],
+        LoggingLevel.Error
+      );
+    }
+
     return Context.instance._applicationId;
   }
 
@@ -150,6 +188,16 @@ export class Context {
   }
 
   static get debugOptions(): DebugOptions {
+    if (typeof Context.instance._debugOptions === "undefined") {
+      log(
+        LOG_TAG,
+        [
+          "Attempted to access Context.debugOptions before it was set. This may cause unexpected behaviour.",
+        ],
+        LoggingLevel.Error
+      );
+    }
+
     return Context.instance._debugOptions;
   }
 
@@ -158,6 +206,6 @@ export class Context {
   }
 
   static get startTime(): Date {
-    return Context._instance._startTime;
+    return Context.instance._startTime;
   }
 }

glean/src/core/glean.ts

@@ -99,12 +99,13 @@ class Glean {
    * Afterward, the upload_enabled flag is set to false.
    */
   private static async onUploadDisabled(): Promise<void> {
+    // It's fine to set this before submitting the deletion request ping,
+    // that ping is still sent even if upload is disabled.
+    Context.uploadEnabled = false;
     // We need to use an undispatched submission to guarantee that the
     // ping is collected before metric are cleared, otherwise we end up
     // with malformed pings.
     await PingType._private_submitUndispatched(Glean.corePings.deletionRequest);
-
-    Context.uploadEnabled = false;
     await Glean.clearMetrics();
   }
 
@@ -210,15 +211,19 @@ class Glean {
       return;
     }
 
+    Context.applicationId = sanitizeApplicationId(applicationId);
+
     // The configuration constructor will throw in case config has any incorrect prop.
     const correctConfig = new Configuration(config);
+    Context.debugOptions = correctConfig.debug;
+    Glean.instance._config = correctConfig;
 
     Context.metricsDatabase = new MetricsDatabase(Glean.platform.Storage);
     Context.eventsDatabase = new EventsDatabase(Glean.platform.Storage);
     Context.pingsDatabase = new PingsDatabase(Glean.platform.Storage);
     Context.errorManager = new ErrorManager();
 
-    Glean.instance._pingUploader = new PingUploader(correctConfig, Glean.platform);
+    Glean.instance._pingUploader = new PingUploader(correctConfig, Glean.platform, Context.pingsDatabase);
 
     Context.pingsDatabase.attachObserver(Glean.pingUploader);
 
@@ -235,26 +240,19 @@ class Glean {
     //
     // The dispatcher will catch and log any exceptions.
     Context.dispatcher.flushInit(async () => {
-      Context.applicationId = sanitizeApplicationId(applicationId);
-      Context.debugOptions = correctConfig.debug;
-      Glean.instance._config = correctConfig;
-
-
-      // IMPORTANT!
-      // Any pings we want to send upon initialization should happen before these two lines.
-      //
-      // Clear application lifetime metrics.
-      await Context.metricsDatabase.clear(Lifetime.Application);
-      // Initialize the events database.
-      await Context.eventsDatabase.initialize();
-
       // We need to mark Glean as initialized before dealing with the upload status,
       // otherwise we will not be able to submit deletion-request pings if necessary.
       //
       // This is fine, we are inside a dispatched task that is guaranteed to run before any
       // other task. No external API call will be executed before we leave this task.
       Context.initialized = true;
 
+      // IMPORTANT!
+      // Any pings we want to send upon initialization should happen before this line.
+      //
+      // Clear application lifetime metrics.
+      await Context.metricsDatabase.clear(Lifetime.Application);
+
       // The upload enabled flag may have changed since the last run, for
       // example by the changing of a config file.
       if (uploadEnabled) {
@@ -285,9 +283,16 @@ class Glean {
         }
       }
 
+      // Initialize the events database.
+      //
+      // It's important this happens _after_ the upload state is dealt with,
+      // because initializing the events database may record the execution_counter and
+      // glean.restarted metrics. If the upload state is not defined these metrics can't be recorded.
+      await Context.eventsDatabase.initialize();
+
       // We only scan the pendings pings **after** dealing with the upload state.
-      // If upload is disabled, we delete all pending pings files
-      // and we need to do that **before** scanning the pending pings
+      // If upload is disabled, pending pings files are deleted
+      // so we need to know that state **before** scanning the pending pings
       // to ensure we don't enqueue pings before their files are deleted.
       await Context.pingsDatabase.scanPendingPings();
     });
@@ -505,22 +510,29 @@ class Glean {
   /**
    * Test-only API**
    *
-   * Resets the Glean to an uninitialized state.
+   * Resets Glean to an uninitialized state.
+   * This is a no-op in case Glean has not been initialized.
    *
    * TODO: Only allow this function to be called on test mode (depends on Bug 1682771).
+   *
+   * @param clearStores Whether or not to clear the events, metrics and pings databases on uninitialize.
    */
-  static async testUninitialize(): Promise<void> {
-    // Get back to an uninitialized state.
-    await Context.testUninitialize();
+  static async testUninitialize(clearStores = true): Promise<void> {
+    if (Context.initialized) {
+      await Glean.shutdown();
 
-    // Shutdown the current uploader.
-    //
-    // This is fine because a new uploader is created on initialize.
-    // It will also guarantee all pings to be sent before uninitializing.
-    await Glean.pingUploader?.shutdown();
+      if (clearStores) {
+        await Context.eventsDatabase.clearAll();
+        await Context.metricsDatabase.clearAll();
+        await Context.pingsDatabase.clearAll();
+      }
+
+      // Get back to an uninitialized state.
+      Context.testUninitialize();
 
-    // Deregister all plugins
-    testResetEvents();
+      // Deregister all plugins
+      testResetEvents();
+    }
   }
 
   /**
@@ -535,25 +547,15 @@ class Glean {
    *        If disabled, all persisted metrics, events and queued pings (except
    *        first_run_date) are cleared. Default to `true`.
    * @param config Glean configuration options.
+   * @param clearStores Whether or not to clear the events, metrics and pings databases on reset.
    */
   static async testResetGlean(
     applicationId: string,
     uploadEnabled = true,
-    config?: ConfigurationInterface
+    config?: ConfigurationInterface,
+    clearStores = true,
   ): Promise<void> {
-    await Glean.testUninitialize();
-
-    // Clear the databases.
-    try {
-      await Context.eventsDatabase.clearAll();
-      await Context.metricsDatabase.clearAll();
-      await Context.pingsDatabase.clearAll();
-    } catch {
-      // Nothing to do here.
-      // It is expected that these will fail in case we are initializing Glean for the first time.
-    }
-
-    // Re-Initialize Glean.
+    await Glean.testUninitialize(clearStores);
     await Glean.testInitialize(applicationId, uploadEnabled, config);
   }
 }

glean/src/core/upload/index.ts

@@ -14,6 +14,7 @@ import type {
   Observer as PingsDatabaseObserver,
   PingInternalRepresentation
 } from "../pings/database.js";
+import type PingsDatabase from "../pings/database.js";
 import {
   isDeletionRequest
 } from "../pings/database.js";
@@ -96,7 +97,7 @@ class PingUploader implements PingsDatabaseObserver {
   constructor(
     config: Configuration,
     platform: Platform,
-    private readonly pingsDatabase = Context.pingsDatabase,
+    private readonly pingsDatabase: PingsDatabase,
     private readonly policy = new Policy(),
     private readonly rateLimiter = new RateLimiter(RATE_LIMITER_INTERVAL_MS, MAX_PINGS_PER_INTERVAL)
   ) {