From 51d7652b01d333fb8e0007279a084776111defcd Mon Sep 17 00:00:00 2001 From: brizental Date: Wed, 3 Feb 2021 17:47:13 +0100 Subject: [PATCH 01/11] Dispatch all external API tasks This commit makes multiple tests fail, that is because by dispatching tasks we cannot await on them anymore and this makes tests assertion run before tasks are completed. --- src/dispatcher.ts | 11 ++- src/glean.ts | 127 +++++++++++++++++++--------------- src/index.ts | 23 ++---- src/metrics/types/boolean.ts | 16 +++-- src/metrics/types/counter.ts | 76 ++++++++++---------- src/metrics/types/datetime.ts | 70 ++++++++++--------- src/metrics/types/event.ts | 54 ++++++++------- src/metrics/types/string.ts | 26 +++---- src/metrics/types/uuid.ts | 38 +++++----- src/pings/index.ts | 44 ++++++------ 10 files changed, 254 insertions(+), 231 deletions(-) diff --git a/src/dispatcher.ts b/src/dispatcher.ts index 014a040a5..e6b07a31d 100644 --- a/src/dispatcher.ts +++ b/src/dispatcher.ts @@ -180,13 +180,22 @@ class Dispatcher { * Flushes the tasks enqueued while the dispatcher was uninitialized. * * This is a no-op in case the dispatcher is not in an uninitialized state. + * + * @param task Optional task to execute before any of the tasks enqueued prior to init. */ - flushInit(): void { + flushInit(task?: Task): void { if (this.state !== DispatcherState.Uninitialized) { console.warn("Attempted to initialize the Dispatcher, but it is already initialized. Ignoring."); return; } + if (task) { + this.launchInternal({ + task, + command: Commands.Task + }, true); + } + this.state = DispatcherState.Idle; this.triggerExecution(); } diff --git a/src/glean.ts b/src/glean.ts index 19b6faa83..75b75361a 100644 --- a/src/glean.ts +++ b/src/glean.ts @@ -203,54 +203,63 @@ class Glean { if (applicationId.length === 0) { throw new Error("Unable to initialize Glean, applicationId cannot be an empty string."); } - Glean.instance._applicationId = sanitizeApplicationId(applicationId); - Glean.instance._config = new Configuration(config); - // Clear application lifetime metrics. + // The configuration constructor will throw in case config hany any incorrect prop. + const correctConfig = new Configuration(config); + + // Initialize the dispatcher and execute init before any other enqueued task. // - // IMPORTANT! - // Any pings we want to send upon initialization should happen before this. - Glean.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) { - // If upload is enabled, just follow the normal code path to - // instantiate the core metrics. - await Glean.onUploadEnabled(); - } else { - // If upload is disabled, and we've never run before, only set the - // client_id to KNOWN_CLIENT_ID, but do not send a deletion request - // ping. - // If we have run before, and if the client_id is not equal to - // the KNOWN_CLIENT_ID, do the full upload disabled operations to - // clear metrics, set the client_id to KNOWN_CLIENT_ID, and send a - // deletion request ping. - const clientId = await Glean.metricsDatabase.getMetric( - CLIENT_INFO_STORAGE, - Glean.coreMetrics.clientId - ); - - if (clientId) { - if (clientId !== KNOWN_CLIENT_ID) { - // Temporarily enable uploading so we can submit a - // deletion request ping. - Glean.uploadEnabled = true; - await Glean.onUploadDisabled(); - } + // Note: We decide to execute the above tasks outside of the dispatcher task, + // because they will throw if configuration is incorrect and we want them to throw. + // + // The dispatcher will catch and log any exceptions. + Glean.dispatcher.flushInit(async () => { + Glean.instance._applicationId = sanitizeApplicationId(applicationId); + Glean.instance._config = correctConfig; + + // Clear application lifetime metrics. + // + // IMPORTANT! + // Any pings we want to send upon initialization should happen before this. + Glean.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) { + // If upload is enabled, just follow the normal code path to + // instantiate the core metrics. + await Glean.onUploadEnabled(); } else { - await Glean.clearMetrics(); + // If upload is disabled, and we've never run before, only set the + // client_id to KNOWN_CLIENT_ID, but do not send a deletion request + // ping. + // If we have run before, and if the client_id is not equal to + // the KNOWN_CLIENT_ID, do the full upload disabled operations to + // clear metrics, set the client_id to KNOWN_CLIENT_ID, and send a + // deletion request ping. + const clientId = await Glean.metricsDatabase.getMetric( + CLIENT_INFO_STORAGE, + Glean.coreMetrics.clientId + ); + + if (clientId) { + if (clientId !== KNOWN_CLIENT_ID) { + // Temporarily enable uploading so we can submit a + // deletion request ping. + Glean.uploadEnabled = true; + await Glean.onUploadDisabled(); + } + } else { + await Glean.clearMetrics(); + } } - } - - Glean.instance._initialized = true; - - await Glean.pingUploader.scanPendingPings(); - // Even though this returns a promise, there is no need to block on it returning. - Glean.pingUploader.triggerUpload(); - - // Signal to the dispatcher that init is complete. - Glean.dispatcher.flushInit(); + + Glean.instance._initialized = true; + + await Glean.pingUploader.scanPendingPings(); + // Even though this returns a promise, there is no need to block on it returning. + Glean.pingUploader.triggerUpload(); + }); } /** @@ -341,20 +350,26 @@ class Glean { * If the value of this flag is not actually changed, this is a no-op. * * @param flag When true, enable metric collection. - * - * @returns Whether the flag was different from the current value, - * and actual work was done to clear or reinstate metrics. */ - static async setUploadEnabled(flag: boolean): Promise { - if (Glean.uploadEnabled !== flag) { - if (flag) { - await Glean.onUploadEnabled(); - } else { - await Glean.onUploadDisabled(); + static setUploadEnabled(flag: boolean): void { + Glean.dispatcher.launch(async () => { + if (!Glean.initialized) { + console.error( + `Changing upload enabled before Glean is initialized is not supported. + Pass the correct state into \`Glean.initialize\`. + See documentation at https://mozilla.github.io/glean/book/user/general-api.html#initializing-the-glean-sdk` + ); + return; } - return true; - } - return false; + + if (Glean.uploadEnabled !== flag) { + if (flag) { + await Glean.onUploadEnabled(); + } else { + await Glean.onUploadDisabled(); + } + } + }); } /** diff --git a/src/index.ts b/src/index.ts index 82d92a2b9..78b474bec 100644 --- a/src/index.ts +++ b/src/index.ts @@ -31,15 +31,13 @@ export = { * If disabled, all persisted metrics, events and queued pings (except * first_run_date) are cleared. * @param config Glean configuration options. - * - * @returns A promise that is resolved once initialize is completed. */ - async initialize( + initialize( applicationId: string, uploadEnabled: boolean, config?: ConfigurationInterface - ): Promise { - await Glean.initialize(applicationId, uploadEnabled, config); + ): void { + Glean.initialize(applicationId, uploadEnabled, config); }, /** @@ -56,20 +54,9 @@ export = { * If Glean has not been initialized yet, this is also a no-op. * * @param flag When true, enable metric collection. - * - * @returns A promise that is resolved once setUploadEnabled is completed. */ - async setUploadEnabled(flag: boolean): Promise { - if (!Glean.initialized) { - console.error( - `Changing upload enabled before Glean is initialized is not supported. - Pass the correct state into \`Glean.initialize\`. - See documentation at https://mozilla.github.io/glean/book/user/general-api.html#initializing-the-glean-sdk` - ); - return; - } - - await Glean.setUploadEnabled(flag); + setUploadEnabled(flag: boolean): void { + Glean.setUploadEnabled(flag); }, _private: { diff --git a/src/metrics/types/boolean.ts b/src/metrics/types/boolean.ts index 0a3614047..e7fbf13ff 100644 --- a/src/metrics/types/boolean.ts +++ b/src/metrics/types/boolean.ts @@ -34,13 +34,15 @@ class BooleanMetricType extends MetricType { * * @param value the value to set. */ - async set(value: boolean): Promise { - if (!this.shouldRecord()) { - return; - } - - const metric = new BooleanMetric(value); - await Glean.metricsDatabase.record(this, metric); + set(value: boolean): void { + Glean.dispatcher.launch(async () => { + if (!this.shouldRecord()) { + return; + } + + const metric = new BooleanMetric(value); + await Glean.metricsDatabase.record(this, metric); + }); } /** diff --git a/src/metrics/types/counter.ts b/src/metrics/types/counter.ts index 93b6aecd7..1193da8c4 100644 --- a/src/metrics/types/counter.ts +++ b/src/metrics/types/counter.ts @@ -51,43 +51,45 @@ class CounterMetricType extends MetricType { * @param amount The amount to increase by. Should be positive. * If not provided will default to `1`. */ - async add(amount?: number): Promise { - if (!this.shouldRecord()) { - return; - } - - if (isUndefined(amount)) { - amount = 1; - } - - if (amount <= 0) { - // TODO: record error once Bug 1682574 is resolved. - console.warn(`Attempted to add an invalid amount ${amount}. Ignoring.`); - return; - } - - const transformFn = ((amount) => { - return (v?: JSONValue): CounterMetric => { - let metric: CounterMetric; - let result: number; - try { - metric = new CounterMetric(v); - result = metric.get() + amount; - } catch { - metric = new CounterMetric(amount); - result = amount; - } - - if (result > Number.MAX_SAFE_INTEGER) { - result = Number.MAX_SAFE_INTEGER; - } - - metric.set(result); - return metric; - }; - })(amount); - - await Glean.metricsDatabase.transform(this, transformFn); + add(amount?: number): void { + Glean.dispatcher.launch(async () => { + if (!this.shouldRecord()) { + return; + } + + if (isUndefined(amount)) { + amount = 1; + } + + if (amount <= 0) { + // TODO: record error once Bug 1682574 is resolved. + console.warn(`Attempted to add an invalid amount ${amount}. Ignoring.`); + return; + } + + const transformFn = ((amount) => { + return (v?: JSONValue): CounterMetric => { + let metric: CounterMetric; + let result: number; + try { + metric = new CounterMetric(v); + result = metric.get() + amount; + } catch { + metric = new CounterMetric(amount); + result = amount; + } + + if (result > Number.MAX_SAFE_INTEGER) { + result = Number.MAX_SAFE_INTEGER; + } + + metric.set(result); + return metric; + }; + })(amount); + + await Glean.metricsDatabase.transform(this, transformFn); + }); } /** diff --git a/src/metrics/types/datetime.ts b/src/metrics/types/datetime.ts index 217cf7079..d47126168 100644 --- a/src/metrics/types/datetime.ts +++ b/src/metrics/types/datetime.ts @@ -174,40 +174,42 @@ class DatetimeMetricType extends MetricType { * * @param value The Date value to set. If not provided, will record the current time. */ - async set(value?: Date): Promise { - if (!this.shouldRecord()) { - return; - } - - if (!value) { - value = new Date(); - } - - // We always save a milliseconds precision ISO string on the database, - // regardless of the time unit. So we zero out information that - // is not necessary for the current time unit of this metric. - const truncatedDate = value; - switch(this.timeUnit) { - case (TimeUnit.Day): - truncatedDate.setMilliseconds(0); - truncatedDate.setSeconds(0); - truncatedDate.setMinutes(0); - truncatedDate.setMilliseconds(0); - case (TimeUnit.Hour): - truncatedDate.setMilliseconds(0); - truncatedDate.setSeconds(0); - truncatedDate.setMinutes(0); - case (TimeUnit.Minute): - truncatedDate.setMilliseconds(0); - truncatedDate.setSeconds(0); - case (TimeUnit.Second): - truncatedDate.setMilliseconds(0); - default: - break; - } - - const metric = DatetimeMetric.fromDate(value, this.timeUnit); - await Glean.metricsDatabase.record(this, metric); + set(value?: Date): void { + Glean.dispatcher.launch(async () => { + if (!this.shouldRecord()) { + return; + } + + if (!value) { + value = new Date(); + } + + // We always save a milliseconds precision ISO string on the database, + // regardless of the time unit. So we zero out information that + // is not necessary for the current time unit of this metric. + const truncatedDate = value; + switch(this.timeUnit) { + case (TimeUnit.Day): + truncatedDate.setMilliseconds(0); + truncatedDate.setSeconds(0); + truncatedDate.setMinutes(0); + truncatedDate.setMilliseconds(0); + case (TimeUnit.Hour): + truncatedDate.setMilliseconds(0); + truncatedDate.setSeconds(0); + truncatedDate.setMinutes(0); + case (TimeUnit.Minute): + truncatedDate.setMilliseconds(0); + truncatedDate.setSeconds(0); + case (TimeUnit.Second): + truncatedDate.setMilliseconds(0); + default: + break; + } + + const metric = DatetimeMetric.fromDate(value, this.timeUnit); + await Glean.metricsDatabase.record(this, metric); + }); } /** diff --git a/src/metrics/types/event.ts b/src/metrics/types/event.ts index b520587b7..ffedaa604 100644 --- a/src/metrics/types/event.ts +++ b/src/metrics/types/event.ts @@ -44,35 +44,37 @@ class EventMetricType extends MetricType { * additional richer context is needed. * The maximum length for values is 100 bytes. */ - async record(extra?: ExtraMap): Promise { - if (!this.shouldRecord()) { - return; - } - - const timestamp = this.getMonotonicNow(); + record(extra?: ExtraMap): void { + Glean.dispatcher.launch(async () => { + if (!this.shouldRecord()) { + return; + } + + const timestamp = this.getMonotonicNow(); - // Truncate the extra keys, if needed. - let truncatedExtra: ExtraMap | undefined = undefined; - if (extra && this.allowedExtraKeys) { - truncatedExtra = {}; - for (const [name, value] of Object.entries(extra)) { - if (this.allowedExtraKeys.includes(name)) { - truncatedExtra[name] = value.substr(0, MAX_LENGTH_EXTRA_KEY_VALUE); - } else { - // TODO: bug 1682574 - record an error. - console.error(`Invalid key index ${name}`); - continue; + // Truncate the extra keys, if needed. + let truncatedExtra: ExtraMap | undefined = undefined; + if (extra && this.allowedExtraKeys) { + truncatedExtra = {}; + for (const [name, value] of Object.entries(extra)) { + if (this.allowedExtraKeys.includes(name)) { + truncatedExtra[name] = value.substr(0, MAX_LENGTH_EXTRA_KEY_VALUE); + } else { + // TODO: bug 1682574 - record an error. + console.error(`Invalid key index ${name}`); + continue; + } } } - } - - const event = new RecordedEvent( - this.category, - this.name, - timestamp, - truncatedExtra, - ); - await Glean.eventsDatabase.record(this, event); + + const event = new RecordedEvent( + this.category, + this.name, + timestamp, + truncatedExtra, + ); + await Glean.eventsDatabase.record(this, event); + }); } /** diff --git a/src/metrics/types/string.ts b/src/metrics/types/string.ts index 40b8c61ab..7ccaa339a 100644 --- a/src/metrics/types/string.ts +++ b/src/metrics/types/string.ts @@ -51,18 +51,20 @@ class StringMetricType extends MetricType { * * @param value the value to set. */ - async set(value: string): Promise { - if (!this.shouldRecord()) { - return; - } - - if (value.length > MAX_LENGTH_VALUE) { - // TODO: record error once Bug 1682574 is resolved. - console.warn(`String ${value} is longer than ${MAX_LENGTH_VALUE} chars. Truncating.`); - } - - const metric = new StringMetric(value.substr(0, MAX_LENGTH_VALUE)); - await Glean.metricsDatabase.record(this, metric); + set(value: string): void { + Glean.dispatcher.launch(async () => { + if (!this.shouldRecord()) { + return; + } + + if (value.length > MAX_LENGTH_VALUE) { + // TODO: record error once Bug 1682574 is resolved. + console.warn(`String ${value} is longer than ${MAX_LENGTH_VALUE} chars. Truncating.`); + } + + const metric = new StringMetric(value.substr(0, MAX_LENGTH_VALUE)); + await Glean.metricsDatabase.record(this, metric); + }); } /** diff --git a/src/metrics/types/uuid.ts b/src/metrics/types/uuid.ts index de0c8fe8f..667a88b86 100644 --- a/src/metrics/types/uuid.ts +++ b/src/metrics/types/uuid.ts @@ -75,24 +75,26 @@ class UUIDMetricType extends MetricType { * @throws In case `value` is not a valid UUID. */ async set(value: string): Promise { - if (!this.shouldRecord()) { - return; - } - - if (!value) { - value = generateUUIDv4(); - } - - let metric: UUIDMetric; - try { - metric = new UUIDMetric(value); - } catch { - // TODO: record error once Bug 1682574 is resolved. - console.warn(`"${value}" is not a valid UUID. Ignoring`); - return; - } - - await Glean.metricsDatabase.record(this, metric); + Glean.dispatcher.launch(async () => { + if (!this.shouldRecord()) { + return; + } + + if (!value) { + value = generateUUIDv4(); + } + + let metric: UUIDMetric; + try { + metric = new UUIDMetric(value); + } catch { + // TODO: record error once Bug 1682574 is resolved. + console.warn(`"${value}" is not a valid UUID. Ignoring`); + return; + } + + await Glean.metricsDatabase.record(this, metric); + }); } /** diff --git a/src/pings/index.ts b/src/pings/index.ts index f616f6ae9..ecd9867ec 100644 --- a/src/pings/index.ts +++ b/src/pings/index.ts @@ -41,29 +41,29 @@ class PingType { * * @param reason The reason the ping was triggered. Included in the * `ping_info.reason` part of the payload. - * - * @returns Whether or not the ping was successfully submitted. */ - async submit(reason?: string): Promise { - if (!Glean.initialized) { - console.info("Glean must be initialized before submitting pings."); - return false; - } - - if (!Glean.isUploadEnabled()) { - console.info("Glean disabled: not submitting any pings."); - return false; - } - - let correctedReason = reason; - if (reason && !this.reasonCodes.includes(reason)) { - console.error(`Invalid reason code ${reason} from ${this.name}. Ignoring.`); - correctedReason = undefined; - } - - const identifier = UUIDv4(); - await collectAndStorePing(identifier, this, correctedReason); - return true; + submit(reason?: string): void { + Glean.dispatcher.launch(async () => { + if (!Glean.initialized) { + console.info("Glean must be initialized before submitting pings."); + return; + } + + if (!Glean.isUploadEnabled()) { + console.info("Glean disabled: not submitting any pings."); + return; + } + + let correctedReason = reason; + if (reason && !this.reasonCodes.includes(reason)) { + console.error(`Invalid reason code ${reason} from ${this.name}. Ignoring.`); + correctedReason = undefined; + } + + const identifier = UUIDv4(); + await collectAndStorePing(identifier, this, correctedReason); + return; + }); } } From 2a58cb9ccaa2c446817493bd9866c87d2e590e85 Mon Sep 17 00:00:00 2001 From: brizental Date: Thu, 4 Feb 2021 13:50:02 +0100 Subject: [PATCH 02/11] Add the recommended-requiring-type-checking rules to ts linter Initially I was simply going to use one specific rule from this set of recommended rules "require-await", which throws a linter error when you `await` on an expression which does not return a promise. That was going to help me remove the `await`s from all the functions that no longer return promises after the dispatcher changes. It's importat to make this a rule, because during the dispatcher devolpment I have run into many hard to find bugs related to awaiting on sync stuff. Anyways, it turned out this small set of linter rules flagged a bunch of bugs which we were not aware, thus I left the whole set there. --- .eslintrc | 8 ++- src/dispatcher.ts | 19 ++++--- src/glean.ts | 29 ++++++----- src/internal_metrics.ts | 16 +++--- src/metrics/database.ts | 2 - src/metrics/index.ts | 2 +- src/metrics/types/uuid.ts | 6 +-- src/pings/maker.ts | 18 ++++--- src/storage/persistent/webext.ts | 4 +- src/storage/utils.ts | 2 +- src/storage/weak.ts | 32 +++++++----- src/upload/index.ts | 13 +++-- tests/dispatcher.spec.ts | 13 ++--- tests/glean.spec.ts | 71 +++++++++++++-------------- tests/metrics/boolean.spec.ts | 8 +-- tests/metrics/counter.spec.ts | 24 ++++----- tests/metrics/database.spec.ts | 10 ++-- tests/metrics/datetime.spec.ts | 14 +++--- tests/metrics/events_database.spec.ts | 4 +- tests/metrics/string.spec.ts | 12 ++--- tests/metrics/uuid.spec.ts | 14 +++--- tests/pings/database.spec.ts | 32 ++++++------ tests/pings/index.spec.ts | 14 +++--- tests/pings/maker.spec.ts | 2 +- tests/storage.spec.ts | 4 +- tests/upload/index.spec.ts | 10 ++-- tests/upload/uploader.spec.ts | 2 +- tests/utils/webext/index.ts | 3 +- 28 files changed, 210 insertions(+), 178 deletions(-) diff --git a/.eslintrc b/.eslintrc index c2ad5e188..8bf334650 100644 --- a/.eslintrc +++ b/.eslintrc @@ -67,8 +67,12 @@ "files": "*.ts", "extends": [ "plugin:@typescript-eslint/eslint-recommended", - "plugin:@typescript-eslint/recommended" - ] + "plugin:@typescript-eslint/recommended", + "plugin:@typescript-eslint/recommended-requiring-type-checking" + ], + "parserOptions": { + "project": ["./tsconfig.json"] + } } ] } diff --git a/src/dispatcher.ts b/src/dispatcher.ts index e6b07a31d..a01bb04c2 100644 --- a/src/dispatcher.ts +++ b/src/dispatcher.ts @@ -118,12 +118,19 @@ class Dispatcher { // That should be avoided as much as possible, // because it will cause a deadlock in case you wait inside a task // that was launched inside another task. - this.currentJob.then(() => { - this.currentJob = undefined; - if (this.state === DispatcherState.Processing) { - this.state = DispatcherState.Idle; - } - }); + this.currentJob + .then(() => { + this.currentJob = undefined; + if (this.state === DispatcherState.Processing) { + this.state = DispatcherState.Idle; + } + }) + .catch(error => { + console.error( + "IMPOSSIBLE: Something went wrong while the dispatcher was executing the tasks queue.", + error + ); + }); } } diff --git a/src/glean.ts b/src/glean.ts index 75b75361a..33b74ae7f 100644 --- a/src/glean.ts +++ b/src/glean.ts @@ -119,7 +119,7 @@ class Glean { // Stops any task execution on the dispatcher. // // While stopped, the dispatcher will enqueue but won't execute any tasks it receives. - await Glean.dispatcher.stop(); + Glean.dispatcher.stop(); // Stop ongoing upload jobs and clear pending pings queue. await Glean.pingUploader.clearPendingPingsQueue(); @@ -158,15 +158,15 @@ class Glean { // Store a "dummy" KNOWN_CLIENT_ID in the client_id metric. This will // make it easier to detect if pings were unintentionally sent after // uploading is disabled. - await Glean.coreMetrics.clientId.set(KNOWN_CLIENT_ID); + Glean.coreMetrics.clientId.set(KNOWN_CLIENT_ID); // Restore the first_run_date. - await Glean.coreMetrics.firstRunDate.set(firstRunDate); + Glean.coreMetrics.firstRunDate.set(firstRunDate); Glean.uploadEnabled = false; // Clear the dispatcher queue. - await Glean.dispatcher.clear(); + Glean.dispatcher.clear(); } /** @@ -190,11 +190,11 @@ class Glean { * - If config.serverEndpoint is an invalid URL; * - If the application if is an empty string. */ - static async initialize( + static initialize( applicationId: string, uploadEnabled: boolean, config?: ConfigurationInterface - ): Promise { + ): void { if (Glean.instance._initialized) { console.warn("Attempted to initialize Glean, but it has already been initialized. Ignoring."); return; @@ -216,12 +216,12 @@ class Glean { Glean.dispatcher.flushInit(async () => { Glean.instance._applicationId = sanitizeApplicationId(applicationId); Glean.instance._config = correctConfig; - + // Clear application lifetime metrics. // // IMPORTANT! // Any pings we want to send upon initialization should happen before this. - Glean.metricsDatabase.clear(Lifetime.Application); + await Glean.metricsDatabase.clear(Lifetime.Application); // The upload enabled flag may have changed since the last run, for // example by the changing of a config file. @@ -253,12 +253,15 @@ class Glean { await Glean.clearMetrics(); } } - + Glean.instance._initialized = true; - + await Glean.pingUploader.scanPendingPings(); + // Even though this returns a promise, there is no need to block on it returning. - Glean.pingUploader.triggerUpload(); + // + // On the contrary we _want_ the dispatcher to execute tasks async. + void Glean.pingUploader.triggerUpload(); }); } @@ -412,7 +415,9 @@ class Glean { await Glean.pingsDatabase.clearAll(); // Re-Initialize Glean. - await Glean.initialize(applicationId, uploadEnabled, config); + Glean.initialize(applicationId, uploadEnabled, config); + + await Glean.dispatcher.testBlockOnQueue(); } } diff --git a/src/internal_metrics.ts b/src/internal_metrics.ts index f8339461a..f1943e7a2 100644 --- a/src/internal_metrics.ts +++ b/src/internal_metrics.ts @@ -96,8 +96,8 @@ export class CoreMetrics { await this.initializeOsVersion(); await this.initializeArchitecture(); await this.initializeLocale(); - await this.appBuild.set(appBuild || "Unknown"); - await this.appDisplayVersion.set(appDisplayVersion || "Unkown"); + this.appBuild.set(appBuild || "Unknown"); + this.appDisplayVersion.set(appDisplayVersion || "Unkown"); } /** @@ -122,7 +122,7 @@ export class CoreMetrics { } if (needNewClientId) { - await this.clientId.generateAndSet(); + this.clientId.generateAndSet(); } } @@ -136,7 +136,7 @@ export class CoreMetrics { ); if (!firstRunDate) { - await this.firstRunDate.set(); + this.firstRunDate.set(); } } @@ -144,27 +144,27 @@ export class CoreMetrics { * Gets and sets the os. */ async initializeOs(): Promise { - await this.os.set(await PlatformInfo.os()); + this.os.set(await PlatformInfo.os()); } /** * Gets and sets the os. */ async initializeOsVersion(): Promise { - await this.osVersion.set(await PlatformInfo.osVersion()); + this.osVersion.set(await PlatformInfo.osVersion()); } /** * Gets and sets the system architecture. */ async initializeArchitecture(): Promise { - await this.architecture.set(await PlatformInfo.arch()); + this.architecture.set(await PlatformInfo.arch()); } /** * Gets and sets the system / browser locale. */ async initializeLocale(): Promise { - await this.locale.set(await PlatformInfo.locale()); + this.locale.set(await PlatformInfo.locale()); } } diff --git a/src/metrics/database.ts b/src/metrics/database.ts index 337a3459d..af2e79cc6 100644 --- a/src/metrics/database.ts +++ b/src/metrics/database.ts @@ -106,8 +106,6 @@ class MetricsDatabase { return this.pingStore; case Lifetime.Application: return this.appStore; - default: - throw Error(`Attempted to retrive a store for an unknown lifetime: ${lifetime}`); } } diff --git a/src/metrics/index.ts b/src/metrics/index.ts index 9dd1700d6..7580e9399 100644 --- a/src/metrics/index.ts +++ b/src/metrics/index.ts @@ -49,7 +49,7 @@ export abstract class Metric< */ set(v: unknown): void { if (!this.validate(v)) { - console.error(`Unable to set metric to ${v}. Value is in unexpected format. Ignoring.`); + console.error(`Unable to set metric to ${JSON.stringify(v)}. Value is in unexpected format. Ignoring.`); return; } diff --git a/src/metrics/types/uuid.ts b/src/metrics/types/uuid.ts index 667a88b86..c0717547a 100644 --- a/src/metrics/types/uuid.ts +++ b/src/metrics/types/uuid.ts @@ -74,7 +74,7 @@ class UUIDMetricType extends MetricType { * * @throws In case `value` is not a valid UUID. */ - async set(value: string): Promise { + set(value: string): void { Glean.dispatcher.launch(async () => { if (!this.shouldRecord()) { return; @@ -102,13 +102,13 @@ class UUIDMetricType extends MetricType { * * @returns The generated value or `undefined` in case this metric shouldn't be recorded. */ - async generateAndSet(): Promise { + generateAndSet(): string | undefined { if (!this.shouldRecord()) { return; } const value = generateUUIDv4(); - await this.set(value); + this.set(value); return value; } diff --git a/src/pings/maker.ts b/src/pings/maker.ts index 2246a3ee4..09786e40c 100644 --- a/src/pings/maker.ts +++ b/src/pings/maker.ts @@ -31,7 +31,7 @@ export async function getSequenceNumber(ping: PingType): Promise { }); const currentSeqData = await Glean.metricsDatabase.getMetric(PING_INFO_STORAGE, seq); - await seq.add(1); + seq.add(1); if (currentSeqData) { // Creating a new counter metric validates that the metric stored is actually a number. @@ -77,7 +77,7 @@ export async function getStartEndTimes(ping: PingType): Promise<{ startTime: str // Update the start time with the current time. const endTimeData = new Date(); - await start.set(endTimeData); + start.set(endTimeData); const endTime = DatetimeMetric.fromDate(endTimeData, TimeUnit.Minute); return { @@ -149,11 +149,8 @@ export async function buildClientInfoSection(ping: PingType): Promise | undefined> { +export function getPingHeaders(): Record | undefined { // TODO: Returning nothing for now until Bug 1685718 is resolved. return; } @@ -198,7 +195,12 @@ export async function collectPing(ping: PingType, reason?: string): Promise { - return this.store; + _getWholeStore(): Promise { + return Promise.resolve(this.store); } - async get(index: StorageIndex): Promise { - return getValueFromNestedObject(this.store, index); + get(index: StorageIndex): Promise { + try { + const value = getValueFromNestedObject(this.store, index); + return Promise.resolve(value); + } catch(e) { + return Promise.reject(e); + } } - async update( + update( index: StorageIndex, transformFn: (v?: JSONValue) => JSONValue ): Promise { - this.store = updateNestedObject(this.store, index, transformFn); + try { + this.store = updateNestedObject(this.store, index, transformFn); + return Promise.resolve(); + } catch(e) { + return Promise.reject(e); + } } - async delete(index: StorageIndex): Promise { + delete(index: StorageIndex): Promise { try { this.store = deleteKeyFromNestedObject(this.store, index); } catch (e) { - console.warn(e.message, "Ignoring."); + console.warn((e as Error).message, "Ignoring."); } + return Promise.resolve(); } } - + export default WeakStore; diff --git a/src/upload/index.ts b/src/upload/index.ts index c2a840fcc..709a43e3f 100644 --- a/src/upload/index.ts +++ b/src/upload/index.ts @@ -155,7 +155,12 @@ class PingUploader implements PingsDatabaseObserver { const finalPing = this.preparePingForUpload(ping); const result = await Uploader.post( - `${Glean.serverEndpoint}${ping.path}`, + // We are sure that the applicationId is not `undefined` at this point, + // this function is only called when submitting a ping + // and that function return early when Glean is not initialized. + // + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + `${Glean.serverEndpoint!}${ping.path}`, finalPing.payload, finalPing.headers ); @@ -210,13 +215,13 @@ class PingUploader implements PingsDatabaseObserver { if (result === UploadResultStatus.UnrecoverableFailure || (status && status >= 400 && status < 500)) { console.warn( - `Unrecoverable upload failure while attempting to send ping ${identifier}. Error was ${status}.`); + `Unrecoverable upload failure while attempting to send ping ${identifier}. Error was ${status ?? "no status"}.`); await Glean.pingsDatabase.deletePing(identifier); return false; } console.warn( - `Recoverable upload failure while attempting to send ping ${identifier}, will retry. Error was ${status}.`); + `Recoverable upload failure while attempting to send ping ${identifier}, will retry. Error was ${status ?? "no status"}.`); return true; } @@ -314,7 +319,7 @@ class PingUploader implements PingsDatabaseObserver { */ update(identifier: string, ping: PingInternalRepresentation): void { this.enqueuePing({ identifier, ...ping }); - this.triggerUpload(); + void this.triggerUpload(); } } diff --git a/tests/dispatcher.spec.ts b/tests/dispatcher.spec.ts index 94232ee8e..1f48880d6 100644 --- a/tests/dispatcher.spec.ts +++ b/tests/dispatcher.spec.ts @@ -27,7 +27,7 @@ describe("Dispatcher", function() { dispatcher && await dispatcher.testBlockOnQueue(); }); - it("launch correctly adds tasks to the queue", async function () { + it("launch correctly adds tasks to the queue", function () { dispatcher = new Dispatcher(); for (let i = 0; i < 10; i++) { dispatcher.launch(sampleTask); @@ -80,9 +80,10 @@ describe("Dispatcher", function() { // eslint-disable-next-line @typescript-eslint/no-explicit-any const stubs: sinon.SinonStub[] = []; for (let i = 0; i < 10; i++) { - stubs.push(sandbox.stub().callsFake(async (): Promise => { + stubs.push(sandbox.stub().callsFake((): Promise => { counts[i] = counter; counter++; + return Promise.resolve(); })); } @@ -113,7 +114,7 @@ describe("Dispatcher", function() { } }); - it("exceptions thrown by enqueued tasks are caught", async function () { + it("exceptions thrown by enqueued tasks are caught", function () { dispatcher = new Dispatcher(); const stub = sandbox.stub().callsFake(() => Promise.reject()); for (let i = 0; i < 10; i++) { @@ -124,7 +125,7 @@ describe("Dispatcher", function() { assert.doesNotThrow(async () => await dispatcher.testBlockOnQueue()); }); - it("queue is bounded before flushInit", async function () { + it("queue is bounded before flushInit", function () { dispatcher = new Dispatcher(5); for (let i = 0; i < 10; i++) { dispatcher.launch(sampleTask); @@ -224,7 +225,7 @@ describe("Dispatcher", function() { dispatcher.launch(stub); } - dispatcher.launch(async () => dispatcher.clear()); + dispatcher.launch(() => Promise.resolve(dispatcher.clear())); for (let i = 0; i < 10; i++) { dispatcher.launch(stub); } @@ -260,7 +261,7 @@ describe("Dispatcher", function() { dispatcher.launch(stub); } - dispatcher.launch(async () => dispatcher.stop()); + dispatcher.launch(() => Promise.resolve(dispatcher.stop())); await dispatcher.testBlockOnQueue(); assert.strictEqual(stub.callCount, 10); diff --git a/tests/glean.spec.ts b/tests/glean.spec.ts index fa3432498..6320e3e2c 100644 --- a/tests/glean.spec.ts +++ b/tests/glean.spec.ts @@ -30,7 +30,7 @@ describe("Glean", function() { await Glean["coreMetrics"]["firstRunDate"].testGetValue(CLIENT_INFO_STORAGE), undefined); await Glean.testUninitialize(); - await Glean.initialize(GLOBAL_APPLICATION_ID, true); + Glean.initialize(GLOBAL_APPLICATION_ID, true); assert.ok(await Glean["coreMetrics"]["clientId"].testGetValue(CLIENT_INFO_STORAGE)); assert.ok(await Glean["coreMetrics"]["firstRunDate"].testGetValue(CLIENT_INFO_STORAGE)); }); @@ -46,42 +46,42 @@ describe("Glean", function() { }); metric.set("TEST VALUE"); - pings.forEach(async (ping) => { - assert.ok(await Glean["coreMetrics"]["clientId"].testGetValue(ping)); - }); + for (const ping of pings) { + assert.strictEqual(await metric.testGetValue(ping), "TEST VALUE"); + } - await Glean.setUploadEnabled(false); - pings.forEach(async (ping) => { + Glean.setUploadEnabled(false); + for (const ping of pings) { assert.strictEqual(await metric.testGetValue(ping), undefined); - }); + } metric.set("TEST VALUE"); - pings.forEach(async (ping) => { + for (const ping of pings) { assert.strictEqual(await metric.testGetValue(ping), undefined); - }); + } - await Glean.setUploadEnabled(true); - pings.forEach(async (ping) => { + Glean.setUploadEnabled(true); + for (const ping of pings) { assert.strictEqual(await metric.testGetValue(ping), undefined); - }); + } metric.set("TEST VALUE"); - pings.forEach(async (ping) => { - assert.ok(await Glean["coreMetrics"]["clientId"].testGetValue(ping)); - }); + for (const ping of pings) { + assert.strictEqual(await metric.testGetValue(ping), "TEST VALUE"); + } }); it("first_run_date is managed correctly when toggling uploading", async function() { const originalFirstRunDate = await Glean["coreMetrics"]["firstRunDate"] .testGetValueAsString(CLIENT_INFO_STORAGE); - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); assert.strictEqual( await Glean["coreMetrics"]["firstRunDate"].testGetValueAsString(CLIENT_INFO_STORAGE), originalFirstRunDate ); - await Glean.setUploadEnabled(true); + Glean.setUploadEnabled(true); assert.strictEqual( await Glean["coreMetrics"]["firstRunDate"].testGetValueAsString(CLIENT_INFO_STORAGE), originalFirstRunDate @@ -94,13 +94,13 @@ describe("Glean", function() { assert.ok(originalClientId); assert.ok(originalClientId !== KNOWN_CLIENT_ID); - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); assert.strictEqual( await Glean["coreMetrics"]["clientId"].testGetValue(CLIENT_INFO_STORAGE), KNOWN_CLIENT_ID ); - await Glean.setUploadEnabled(true); + Glean.setUploadEnabled(true); const newClientId = await Glean["coreMetrics"]["clientId"].testGetValue(CLIENT_INFO_STORAGE); assert.ok(newClientId !== originalClientId); assert.ok(newClientId !== KNOWN_CLIENT_ID); @@ -108,7 +108,7 @@ describe("Glean", function() { it("client_id is set to known value when uploading disabled at start", async function() { await Glean.testUninitialize(); - await Glean.initialize(GLOBAL_APPLICATION_ID, false); + Glean.initialize(GLOBAL_APPLICATION_ID, false); assert.strictEqual( await Glean["coreMetrics"]["clientId"].testGetValue(CLIENT_INFO_STORAGE), KNOWN_CLIENT_ID @@ -116,9 +116,9 @@ describe("Glean", function() { }); it("client_id is set to random value when uploading enabled at start", async function() { - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); await Glean.testUninitialize(); - await Glean.initialize(GLOBAL_APPLICATION_ID, true); + Glean.initialize(GLOBAL_APPLICATION_ID, true); const clientId = await Glean["coreMetrics"]["clientId"] .testGetValue(CLIENT_INFO_STORAGE); assert.ok(clientId); @@ -126,18 +126,17 @@ describe("Glean", function() { }); it("enabling when already enabled is a no-op", async function() { - assert.ok(!(await Glean.setUploadEnabled(true))); + // TODO: Re-implement this now that this function doesn't return a boolean anymore. }); it("disabling when already disabled is a no-op", async function() { - await Glean.setUploadEnabled(false); - assert.ok(!(await Glean.setUploadEnabled(false))); + // TODO: Re-implement this now that this function doesn't return a boolean anymore. }); it("initialization throws if applicationId is an empty string", async function() { await Glean.testUninitialize(); try { - await Glean.initialize("", true); + Glean.initialize("", true); assert.ok(false); } catch { assert.ok(true); @@ -147,7 +146,7 @@ describe("Glean", function() { it("initialization throws if serverEndpoint is an invalida URL", async function() { await Glean.testUninitialize(); try { - await Glean.initialize(GLOBAL_APPLICATION_ID, true, { serverEndpoint: "" }); + Glean.initialize(GLOBAL_APPLICATION_ID, true, { serverEndpoint: "" }); assert.ok(false); } catch { assert.ok(true); @@ -165,22 +164,22 @@ describe("Glean", function() { }); metric.set("TEST VALUE"); - pings.forEach(async (ping) => { - assert.ok(await Glean["coreMetrics"]["clientId"].testGetValue(ping)); - }); + for (const ping of pings) { + assert.strictEqual(await metric.testGetValue(ping), "TEST VALUE"); + } - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); metric.set("TEST VALUE"); - pings.forEach(async (ping) => { - assert.strictEqual(await Glean["coreMetrics"]["clientId"].testGetValue(ping), undefined); - }); + for (const ping of pings) { + assert.strictEqual(await metric.testGetValue(ping), undefined); + } }); it("initializing twice is a no-op", async function() { await Glean.testUninitialize(); - await Glean.initialize(GLOBAL_APPLICATION_ID, true); + Glean.initialize(GLOBAL_APPLICATION_ID, true); // This time it should not be called, which means upload should not be switched to `false`. - await Glean.initialize(GLOBAL_APPLICATION_ID, false); + Glean.initialize(GLOBAL_APPLICATION_ID, false); assert.ok(Glean.isUploadEnabled()); }); }); diff --git a/tests/metrics/boolean.spec.ts b/tests/metrics/boolean.spec.ts index cfc7d18aa..357697f35 100644 --- a/tests/metrics/boolean.spec.ts +++ b/tests/metrics/boolean.spec.ts @@ -26,7 +26,7 @@ describe("BooleanMetric", function() { }); it("attemping to set when glean upload is disabled is a no-op", async function() { - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); const metric = new BooleanMetricType({ category: "aCategory", @@ -36,7 +36,7 @@ describe("BooleanMetric", function() { disabled: false }); - await metric.set(true); + metric.set(true); assert.strictEqual(await metric.testGetValue("aPing"), undefined); }); @@ -49,7 +49,7 @@ describe("BooleanMetric", function() { disabled: false }); - await metric.set(true); + metric.set(true); assert.strictEqual(await metric.testGetValue("aPing"), true); const snapshot = await Glean.metricsDatabase.getPingMetrics("aPing", true); @@ -69,7 +69,7 @@ describe("BooleanMetric", function() { disabled: false }); - await metric.set(true); + metric.set(true); assert.strictEqual(await metric.testGetValue("aPing"), true); assert.strictEqual(await metric.testGetValue("twoPing"), true); assert.strictEqual(await metric.testGetValue("threePing"), true); diff --git a/tests/metrics/counter.spec.ts b/tests/metrics/counter.spec.ts index 2be2c7817..6cbb5c8aa 100644 --- a/tests/metrics/counter.spec.ts +++ b/tests/metrics/counter.spec.ts @@ -26,7 +26,7 @@ describe("CounterMetric", function() { }); it("attemping to add when glean upload is disabled is a no-op", async function() { - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); const metric = new CounterMetricType({ category: "aCategory", @@ -36,7 +36,7 @@ describe("CounterMetric", function() { disabled: false }); - await metric.add(10); + metric.add(10); assert.strictEqual(await metric.testGetValue("aPing"), undefined); }); @@ -49,7 +49,7 @@ describe("CounterMetric", function() { disabled: false }); - await metric.add(10); + metric.add(10); assert.strictEqual(await metric.testGetValue("aPing"), 10); const snapshot = await Glean.metricsDatabase.getPingMetrics("aPing", true); @@ -69,7 +69,7 @@ describe("CounterMetric", function() { disabled: false }); - await metric.add(10); + metric.add(10); assert.strictEqual(await metric.testGetValue("aPing"), 10); assert.strictEqual(await metric.testGetValue("twoPing"), 10); assert.strictEqual(await metric.testGetValue("threePing"), 10); @@ -84,13 +84,13 @@ describe("CounterMetric", function() { disabled: false }); - await metric.add(0); + metric.add(0); assert.strictEqual(await metric.testGetValue("aPing"), undefined); - await metric.add(-1); + metric.add(-1); assert.strictEqual(await metric.testGetValue("aPing"), undefined); - await metric.add(1); + metric.add(1); assert.strictEqual(await metric.testGetValue("aPing"), 1); }); @@ -103,17 +103,17 @@ describe("CounterMetric", function() { disabled: false }); - await metric.add(2); + metric.add(2); assert.strictEqual(await metric.testGetValue("aPing"), 2); assert.strictEqual(await metric.testGetValue("twoPing"), 2); assert.strictEqual(await metric.testGetValue("threePing"), 2); - await metric.add(2); + metric.add(2); assert.strictEqual(await metric.testGetValue("aPing"), 4); assert.strictEqual(await metric.testGetValue("twoPing"), 4); assert.strictEqual(await metric.testGetValue("threePing"), 4); - await metric.add(2); + metric.add(2); assert.strictEqual(await metric.testGetValue("aPing"), 6); assert.strictEqual(await metric.testGetValue("twoPing"), 6); assert.strictEqual(await metric.testGetValue("threePing"), 6); @@ -128,9 +128,9 @@ describe("CounterMetric", function() { disabled: false }); - await metric.add(2); + metric.add(2); assert.strictEqual(await metric.testGetValue("aPing"), 2); - await metric.add(Number.MAX_SAFE_INTEGER); + metric.add(Number.MAX_SAFE_INTEGER); assert.strictEqual(await metric.testGetValue("aPing"), Number.MAX_SAFE_INTEGER); }); }); diff --git a/tests/metrics/database.spec.ts b/tests/metrics/database.spec.ts index adf4b4eb9..3d0b6f20b 100644 --- a/tests/metrics/database.spec.ts +++ b/tests/metrics/database.spec.ts @@ -115,7 +115,7 @@ describe("MetricsDatabase", function() { disabled: false }); await db.transform(userMetric, (v?: JSONValue) => ( - v ? new StringMetric(`USER_${v}`) : new StringMetric("USER") + v ? new StringMetric(`USER_${JSON.stringify(v)}`) : new StringMetric("USER") )); assert.strictEqual( await db["userStore"].get(["aPing", "string", "user.aMetric"]), @@ -130,7 +130,7 @@ describe("MetricsDatabase", function() { disabled: false }); await db.transform(pingMetric,(v?: JSONValue) => ( - v ? new StringMetric(`PING_${v}`) : new StringMetric("PING") + v ? new StringMetric(`PING_${JSON.stringify(v)}`) : new StringMetric("PING") )); assert.strictEqual( await db["pingStore"].get(["aPing", "string", "ping.aMetric"]), @@ -145,7 +145,7 @@ describe("MetricsDatabase", function() { disabled: false }); await db.transform(appMetric, (v?: JSONValue) => ( - v ? new StringMetric(`APP_${v}`) : new StringMetric("APP") + v ? new StringMetric(`APP_${JSON.stringify(v)}`) : new StringMetric("APP") )); assert.strictEqual( await db["appStore"].get(["aPing", "string", "app.aMetric"]), @@ -164,7 +164,7 @@ describe("MetricsDatabase", function() { }); await db.transform(metric, (v?: JSONValue) => ( - v ? new StringMetric(`EXTRA_${v}`) : new StringMetric("EXTRA") + v ? new StringMetric(`EXTRA_${JSON.stringify(v)}`) : new StringMetric("EXTRA") )); const recorded1 = await db["appStore"].get(["aPing", "string", "aMetric"]); assert.strictEqual(recorded1, "EXTRA"); @@ -185,7 +185,7 @@ describe("MetricsDatabase", function() { }); await db.transform(metric, (v?: JSONValue) => ( - v ? new StringMetric(`EXTRA_${v}`) : new StringMetric("EXTRA") + v ? new StringMetric(`EXTRA_${JSON.stringify(v)}`) : new StringMetric("EXTRA") )); const recorded = await db["appStore"].get(["aPing", "string", "aMetric"]); assert.strictEqual(recorded, undefined); diff --git a/tests/metrics/datetime.spec.ts b/tests/metrics/datetime.spec.ts index 77f72dfcc..7c5ae23d2 100644 --- a/tests/metrics/datetime.spec.ts +++ b/tests/metrics/datetime.spec.ts @@ -63,7 +63,7 @@ describe("DatetimeMetric", function() { }); it("attemping to set when glean upload is disabled is a no-op", async function() { - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); const metric = new DatetimeMetricType({ category: "aCategory", @@ -73,7 +73,7 @@ describe("DatetimeMetric", function() { disabled: false }, "millisecond"); - await metric.set(); + metric.set(); assert.strictEqual(await metric.testGetValue("aPing"), undefined); }); @@ -90,7 +90,7 @@ describe("DatetimeMetric", function() { disabled: false }, "minute"); - await metric.set(new Date(1995, 4, 25, 8, 15, 45, 385)); + metric.set(new Date(1995, 4, 25, 8, 15, 45, 385)); assert.strictEqual(await metric.testGetValueAsString("aPing"), "1995-05-25T08:15+05:00"); const snapshot = await Glean.metricsDatabase.getPingMetrics("aPing", true); @@ -117,19 +117,19 @@ describe("DatetimeMetric", function() { disabled: false }, "millisecond"); - await metric.set(new Date(1995, 4, 25, 8, 15, 45, 385)); + metric.set(new Date(1995, 4, 25, 8, 15, 45, 385)); assert.strictEqual(await metric.testGetValueAsString("aPing"), "1995-05-25T08:15:45.385+05:00"); assert.strictEqual(await metric.testGetValueAsString("twoPing"), "1995-05-25T08:15:45.385+05:00"); assert.strictEqual(await metric.testGetValueAsString("threePing"), "1995-05-25T08:15:45.385+05:00"); // A date prior to the UNIX epoch - await metric.set(new Date(1895, 4, 25, 8, 15, 45, 385)); + metric.set(new Date(1895, 4, 25, 8, 15, 45, 385)); assert.strictEqual(await metric.testGetValueAsString("aPing"), "1895-05-25T08:15:45.385+05:00"); assert.strictEqual(await metric.testGetValueAsString("twoPing"), "1895-05-25T08:15:45.385+05:00"); assert.strictEqual(await metric.testGetValueAsString("threePing"), "1895-05-25T08:15:45.385+05:00"); // A date following 2038 (the extent of signed 32-bits after UNIX epoch) - await metric.set(new Date(2995, 4, 25, 8, 15, 45, 385)); + metric.set(new Date(2995, 4, 25, 8, 15, 45, 385)); assert.strictEqual(await metric.testGetValueAsString("aPing"), "2995-05-25T08:15:45.385+05:00"); assert.strictEqual(await metric.testGetValueAsString("twoPing"), "2995-05-25T08:15:45.385+05:00"); assert.strictEqual(await metric.testGetValueAsString("threePing"), "2995-05-25T08:15:45.385+05:00"); @@ -181,7 +181,7 @@ describe("DatetimeMetric", function() { disabled: false }, testCase.unit); - await metric.set(date); + metric.set(date); assert.strictEqual(await metric.testGetValueAsString("aPing"), testCase.expected); } }); diff --git a/tests/metrics/events_database.spec.ts b/tests/metrics/events_database.spec.ts index 4c776ed4b..ad506070c 100644 --- a/tests/metrics/events_database.spec.ts +++ b/tests/metrics/events_database.spec.ts @@ -10,7 +10,7 @@ import EventMetricType from "metrics/types/event"; import { JSONObject } from "utils"; describe("EventsDatabase", function() { - it("stable serialization", async function () { + it("stable serialization", function () { const event_empty = new RecordedEvent( "cat", "name", @@ -34,7 +34,7 @@ describe("EventsDatabase", function() { assert.deepStrictEqual(event_data, RecordedEvent.fromJSONObject(event_data_json)); }); - it("deserialize existing data", async function () { + it("deserialize existing data", function () { const event_empty_json = { "category": "cat", "extra": undefined, diff --git a/tests/metrics/string.spec.ts b/tests/metrics/string.spec.ts index 4c906e533..0aafdbbcf 100644 --- a/tests/metrics/string.spec.ts +++ b/tests/metrics/string.spec.ts @@ -26,7 +26,7 @@ describe("StringMetric", function() { }); it("attemping to set when glean upload is disabled is a no-op", async function() { - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); const metric = new StringMetricType({ category: "aCategory", @@ -36,7 +36,7 @@ describe("StringMetric", function() { disabled: false }); - await metric.set("test_string_value"); + metric.set("test_string_value"); assert.strictEqual(await metric.testGetValue("aPing"), undefined); }); @@ -49,9 +49,9 @@ describe("StringMetric", function() { disabled: false }); - await metric.set("test_string_value"); + metric.set("test_string_value"); assert.strictEqual(await metric.testGetValue("aPing"), "test_string_value"); - + const snapshot = await Glean.metricsDatabase.getPingMetrics("aPing", true); assert.deepStrictEqual(snapshot, { "string": { @@ -69,7 +69,7 @@ describe("StringMetric", function() { disabled: false }); - await metric.set("test_string_value"); + metric.set("test_string_value"); assert.strictEqual(await metric.testGetValue("aPing"), "test_string_value"); assert.strictEqual(await metric.testGetValue("twoPing"), "test_string_value"); assert.strictEqual(await metric.testGetValue("threePing"), "test_string_value"); @@ -85,7 +85,7 @@ describe("StringMetric", function() { }); const testString = "01234567890".repeat(20); - await metric.set(testString); + metric.set(testString); assert.strictEqual( await metric.testGetValue("aPing"), diff --git a/tests/metrics/uuid.spec.ts b/tests/metrics/uuid.spec.ts index 24578032c..56f0504d8 100644 --- a/tests/metrics/uuid.spec.ts +++ b/tests/metrics/uuid.spec.ts @@ -27,7 +27,7 @@ describe("UUIDMetric", function() { }); it("attemping to set when glean upload is disabled is a no-op", async function() { - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); const metric = new UUIDMetricType({ category: "aCategory", @@ -37,12 +37,12 @@ describe("UUIDMetric", function() { disabled: false }); - await metric.generateAndSet(); + metric.generateAndSet(); assert.strictEqual(await metric.testGetValue("aPing"), undefined); }); it("attemping to set an invalid uuid is a no-op", async function() { - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); const metric = new UUIDMetricType({ category: "aCategory", @@ -52,7 +52,7 @@ describe("UUIDMetric", function() { disabled: false }); - await metric.set("not valid"); + metric.set("not valid"); assert.strictEqual(await metric.testGetValue("aPing"), undefined); }); @@ -66,7 +66,7 @@ describe("UUIDMetric", function() { }); const expected = UUIDv4(); - await metric.set(expected); + metric.set(expected); assert.strictEqual(await metric.testGetValue("aPing"), expected); const snapshot = await Glean.metricsDatabase.getPingMetrics("aPing", true); @@ -87,7 +87,7 @@ describe("UUIDMetric", function() { }); const expected = UUIDv4(); - await metric.set(expected); + metric.set(expected); assert.strictEqual(await metric.testGetValue("aPing"), expected); assert.strictEqual(await metric.testGetValue("twoPing"), expected); assert.strictEqual(await metric.testGetValue("threePing"), expected); @@ -102,7 +102,7 @@ describe("UUIDMetric", function() { disabled: false }); - const value = await metric.generateAndSet(); + const value = metric.generateAndSet(); assert.strictEqual(value, await metric.testGetValue("aPing")); }); }); diff --git a/tests/pings/database.spec.ts b/tests/pings/database.spec.ts index 749a730e9..c79b01b2c 100644 --- a/tests/pings/database.spec.ts +++ b/tests/pings/database.spec.ts @@ -124,15 +124,15 @@ describe("PingsDatabase", function() { }; const identifiers = ["foo", "bar", "baz", "qux", "etc"]; - identifiers.forEach(async (identifier) => { + for (const identifier of identifiers) { await db.recordPing(path, identifier, payload); - }); + } const allPings = await db.getAllPings(); assert.strictEqual(Object.keys(allPings).length, 5); - identifiers.forEach(async (identifier, index) => { + for (const [index, identifier] of identifiers.entries()) { assert.strictEqual(identifier, identifiers[index]); - }); + } }); it("getAllPings dosen't error when there are no pings stored", async function () { @@ -157,23 +157,23 @@ describe("PingsDatabase", function() { }; const identifiers = ["foo", "bar", "baz", "qux", "etc"]; - identifiers.forEach(async (identifier) => { + for (const identifier of identifiers) { await db.recordPing(path, identifier, payload); - }); + } await db.deletePing("foo"); const allPings = await db.getAllPings(); assert.strictEqual(Object.keys(allPings).length, 4); - ["bar", "baz", "qux", "etc"].forEach(async (identifier, index) => { + for (const [index, identifier] of identifiers.entries()) { assert.strictEqual(identifier, identifiers[index]); - }); + } await db.deletePing("bar"); const allPings2 = await db.getAllPings(); assert.strictEqual(Object.keys(allPings2).length, 3); - ["baz", "qux", "etc"].forEach(async (identifier, index) => { + for (const [index, identifier] of identifiers.entries()) { assert.strictEqual(identifier, identifiers[index]); - }); + } }); it("deleting a ping that is not in the db doesn't error", async function() { @@ -191,16 +191,16 @@ describe("PingsDatabase", function() { }; const identifiers = ["foo", "bar", "baz", "qux", "etc"]; - identifiers.forEach(async (identifier) => { + for (const identifier of identifiers) { await db.recordPing(path, identifier, payload); - }); + } await db.deletePing("no existo"); const allPings = await db.getAllPings(); assert.strictEqual(Object.keys(allPings).length, 5); - identifiers.forEach(async (identifier, index) => { + for (const [index, identifier] of identifiers.entries()) { assert.strictEqual(identifier, identifiers[index]); - }); + } }); }); @@ -220,9 +220,9 @@ describe("PingsDatabase", function() { }; const identifiers = ["foo", "bar", "baz", "qux", "etc"]; - identifiers.forEach(async (identifier) => { + for (const identifier of identifiers) { await db.recordPing(path, identifier, payload); - }); + } await db.clearAll(); assert.strictEqual(Object.keys(await db["store"]._getWholeStore()).length, 0); diff --git a/tests/pings/index.spec.ts b/tests/pings/index.spec.ts index 5111216ac..8644e5ff0 100644 --- a/tests/pings/index.spec.ts +++ b/tests/pings/index.spec.ts @@ -33,9 +33,9 @@ describe("PingType", function() { lifetime: Lifetime.Ping, disabled: false }); - await counter.add(); + counter.add(); - assert.ok(await ping.submit()); + assert.ok(ping.submit()); // TODO: Make this nicer once we have a nice way to check if pings are enqueued, // possibly once Bug 1677440 is resolved. const storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); @@ -51,20 +51,20 @@ describe("PingType", function() { // TODO: Make this nicer once we have a nice way to check if pings are enqueued, // possibly once Bug 1677440 is resolved. - assert.ok(await ping1.submit()); + assert.ok(ping1.submit()); let storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); assert.strictEqual(Object.keys(storedPings).length, 0); - assert.ok(await ping2.submit()); + assert.ok(ping2.submit()); storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); assert.strictEqual(Object.keys(storedPings).length, 1); }); it("no pings are submitted if upload is disabled", async function() { - await Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); const ping = new PingType("custom", true, false, []); - assert.strictEqual(await ping.submit(), false); + assert.strictEqual(ping.submit(), false); // TODO: Make this nicer once we have a nice way to check if pings are enqueued, // possibly once Bug 1677440 is resolved. const storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); @@ -75,7 +75,7 @@ describe("PingType", function() { await Glean.testUninitialize(); const ping = new PingType("custom", true, false, []); - assert.strictEqual(await ping.submit(), false); + assert.strictEqual(ping.submit(), false); // TODO: Make this nicer once we have a nice way to check if pings are enqueued, // possibly once Bug 1677440 is resolved. const storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); diff --git a/tests/pings/maker.spec.ts b/tests/pings/maker.spec.ts index c53bd6dd1..d993f7de7 100644 --- a/tests/pings/maker.spec.ts +++ b/tests/pings/maker.spec.ts @@ -39,7 +39,7 @@ describe("PingMaker", function() { assert.ok("telemetry_sdk_build" in clientInfo1); // Initialize will also initialize core metrics that are part of the client info. - await Glean.initialize("something something", true, { + Glean.initialize("something something", true, { appBuild:"build", appDisplayVersion: "display version", serverEndpoint: "http://localhost:8080" diff --git a/tests/storage.spec.ts b/tests/storage.spec.ts index 8ad884394..aa2b47066 100644 --- a/tests/storage.spec.ts +++ b/tests/storage.spec.ts @@ -24,7 +24,7 @@ const stores: { } } = { "WeakStore": { - initializeStore: (): WeakStore => new WeakStore() + initializeStore: (): WeakStore => new WeakStore("unused") }, "WebExtStore": { initializeStore: (): WebExtStore => new WebExtStore("test"), @@ -151,7 +151,7 @@ for (const store in stores) { }); it("Attempting to update an existing entry doesn't error ", async function () { - const updater = (v?: JSONValue): string => `${v} new and improved!`; + const updater = (v?: JSONValue): string => `${JSON.stringify(v)} new and improved!`; await store.update(index, updater); assert.strictEqual(updater(value), await store.get(index)); }); diff --git a/tests/upload/index.spec.ts b/tests/upload/index.spec.ts index d056b2628..0ebb61bdd 100644 --- a/tests/upload/index.spec.ts +++ b/tests/upload/index.spec.ts @@ -33,9 +33,9 @@ async function fillUpPingsDatabase(numPings: number): Promise { }; const identifiers = Array.from({ length: numPings }, () => UUIDv4()); - identifiers.forEach(async (identifier) => { + for (const identifier of identifiers) { await Glean.pingsDatabase.recordPing(path, identifier, payload); - }); + } return identifiers; } @@ -60,7 +60,7 @@ function disableGleanUploader(): void { } describe("PingUploader", function() { - afterEach(async function () { + afterEach(function () { sandbox.restore(); }); @@ -112,7 +112,7 @@ describe("PingUploader", function() { // Trigger uploading, but don't wait for it to finish, // so that it is ongoing when we cancel. - uploader.triggerUpload(); + void uploader.triggerUpload(); await uploader.cancelUpload(); // There is really no way to know how many pings Glean will be able to upload @@ -158,7 +158,7 @@ describe("PingUploader", function() { assert.strictEqual(Glean["pingUploader"]["queue"].length, 1); }); - it("duplicates are not enqueued", async function() { + it("duplicates are not enqueued", function() { const uploader = new PingUploader(); for (let i = 0; i < 10; i++) { uploader["enqueuePing"]({ diff --git a/tests/upload/uploader.spec.ts b/tests/upload/uploader.spec.ts index b279fae85..3fced926e 100644 --- a/tests/upload/uploader.spec.ts +++ b/tests/upload/uploader.spec.ts @@ -57,7 +57,7 @@ function createResponse(status: number): Response { } describe("Uploader/browser", function () { - afterEach(async function () { + afterEach(function () { sandbox.restore(); }); diff --git a/tests/utils/webext/index.ts b/tests/utils/webext/index.ts index 17ac4acad..ea42fe241 100644 --- a/tests/utils/webext/index.ts +++ b/tests/utils/webext/index.ts @@ -43,6 +43,7 @@ export async function setupFirefox(headless: boolean): Promise { // https://github.com/SeleniumHQ/selenium/blob/trunk/javascript/node/selenium-webdriver/firefox.js#L632-L652 // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore + // eslint-disable-next-line @typescript-eslint/no-unsafe-call browser.installAddon( path.resolve(__dirname, "sample/web-ext-artifacts/gleanjs-test-addon-0.0.1.zip"), true @@ -71,7 +72,7 @@ export async function setupFirefox(headless: boolean): Promise { export function webExtensionAPIProxyBuilder(browser: WebDriver, method: string[]) { return async function (...args: never[]): Promise { return browser.executeAsyncScript((args: never[], method: string[], callback: (arg: string) => void) => { - console.log(`Executing proxy ${method} with the following args:`, args); + console.log(`Executing proxy ${JSON.stringify(method)} with the following args:`, args); console.log("Dispatching a new test event!"); // Send a new test event to be run on the background script of the sample web extension. From b38029e556b97c9020fe73858c27189499a604c0 Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 5 Feb 2021 10:59:27 +0100 Subject: [PATCH 03/11] Provide a testLaunch command to launch waitable tasks ...and `testLaunch` all `testGetValue` functions. I considered simply using testBlockOnQueue for waiting on tasks. But that has two main issues: - It may hang for a long time if tasks keep being enqueued; - It does not guarantee that we continue on the expected moment. For example, imagine we launch multiple tasks for setting the same string. When we await on `testBlockOnQueue` we will wait for all the setting tasks and not necessarily on the one we are expecting. This will result in assertions at the unexpected time. --- src/dispatcher.ts | 143 +++++++++++++++++++++++++++++++++- src/metrics/types/boolean.ts | 8 +- src/metrics/types/counter.ts | 6 +- src/metrics/types/datetime.ts | 5 +- src/metrics/types/event.ts | 9 +-- src/metrics/types/string.ts | 6 +- src/metrics/types/uuid.ts | 36 ++------- src/utils.ts | 28 +++++++ tests/dispatcher.spec.ts | 129 ++++++++++++++++++++++++++++++ 9 files changed, 328 insertions(+), 42 deletions(-) diff --git a/src/dispatcher.ts b/src/dispatcher.ts index a01bb04c2..5ce425710 100644 --- a/src/dispatcher.ts +++ b/src/dispatcher.ts @@ -2,6 +2,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +import { generateUUIDv4 } from "utils"; + // The possible states a dispatcher instance can be in. export const enum DispatcherState { // The dispatcher has not been initialized yet. @@ -34,12 +36,29 @@ type Task = () => Promise; // An executable command. type Command = { + testId?: string, task: Task, command: Commands.Task, } | { command: Exclude, }; +/** + * An observer of the commands being executed by the dispatcher. + */ +class DispatcherObserver { + constructor(private cb: (command: Command) => void) {} + + /** + * Updates an observer when the dispatcher finishes executing a command. + * + * @param command The command that was just executed by the dispatcher. + */ + update(command: Command): void { + this.cb(command); + } +} + /** * A task dispatcher for async tasks. * @@ -55,11 +74,56 @@ class Dispatcher { // This is `undefined` in case there is no ongoing execution of tasks. private currentJob?: Promise; + // Observers that are notified about every executed command in this dispacther. + // + // This is private, because we only expect `testLaunch` to attach observers as of yet. + private observers: DispatcherObserver[]; + constructor(readonly maxPreInitQueueSize = 100) { + this.observers = []; this.queue = []; this.state = DispatcherState.Uninitialized; } + /** + * Attaches an observer that will be notified about state changes on the Dispatcher. + * + * # Note + * + * This is a private function because only the `testLaunch` function + * is expected to watch the state of the Dispatcher as of yet. + * + * @param observer The observer to attach. + */ + private attachObserver(observer: DispatcherObserver): void { + this.observers.push(observer); + } + + /** + * Un-attaches an observer that will be notified about state changes on the Dispatcher. + * + * # Note + * + * This is a private function because only the `testLaunch` function + * is expected to watch the state of the Dispatcher as of yet. + * + * @param observer The observer to attach. + */ + private unattachObserver(observer: DispatcherObserver): void { + this.observers = this.observers.filter(curr => observer !== curr); + } + + /** + * Notify any currently attached observer that a new command was executed. + * + * @param command The command to notify about + */ + private notifyObservers(command: Command): void { + for (const observer of this.observers) { + observer.update(command); + } + } + /** * Gets the oldest command added to the queue. * @@ -91,13 +155,16 @@ class Dispatcher { switch(nextCommand.command) { case(Commands.Stop): this.state = DispatcherState.Stopped; + this.notifyObservers(nextCommand); return; case(Commands.Clear): this.queue = []; this.state = DispatcherState.Stopped; + this.notifyObservers(nextCommand); return; case(Commands.Task): await this.executeTask(nextCommand.task); + this.notifyObservers(nextCommand); nextCommand = this.getNextCommand(); } } @@ -146,12 +213,14 @@ class Dispatcher { * @param command The command to enqueue. * @param priorityTask Whether or not this task is a priority task * and should be enqueued at the front of the queue. + * + * @returns Wheter or not the task was queued. */ - private launchInternal(command: Command, priorityTask = false): void { + private launchInternal(command: Command, priorityTask = false): boolean { if (!priorityTask && this.state === DispatcherState.Uninitialized) { if (this.queue.length >= this.maxPreInitQueueSize) { console.warn("Unable to enqueue task, pre init queue is full."); - return; + return false; } } @@ -162,6 +231,8 @@ class Dispatcher { } this.triggerExecution(); + + return true; } /** @@ -254,7 +325,7 @@ class Dispatcher { * * Returns a promise that resolves once the current task execution in finished. * - * Use this with caution. + * Use this with caution. * If called inside a task launched inside another task, it will cause a deadlock. * * @returns The promise. @@ -276,6 +347,72 @@ class Dispatcher { await this.testBlockOnQueue(); this.state = DispatcherState.Uninitialized; } + + /** + * Launches a task in test mode. + * + * # Note + * + * This function will resume the execution of tasks if the dispatcher was stopped + * and return the dispatcher back to an idle state. + * + * This is important in order not to hang forever in case the dispatcher is stopped. + * + * @param task The task to launch. + * + * @returns A promise which only resolves once the task is done being executed + * or is guaranteed to not be executed ever i.e. if the queue gets cleared. + * This promise is rejected if the dispatcher takes more than 1s + * to execute the current task i.e. if the dispatcher is uninitialized. + */ + testLaunch(task: Task): Promise { + const testId = generateUUIDv4(); + console.info("Launching a test task.", testId); + + this.resume(); + const wasLaunched = this.launchInternal({ + testId, + task, + command: Commands.Task + }); + + if (!wasLaunched) { + return Promise.reject(); + } + + // This promise will resolve: + // + // - If the dispatcher gets a Clear command; + // - If a task with `testId` is executed; + // + // This promise will reject if: + // + // - If we wait for this task to be execute for more than 1s. + // This is to attend to the case where the dispatcher is Stopped + // and the task takes to long to be executed. + return new Promise((resolve, reject) => { + const observer = new DispatcherObserver((command: Command) => { + const isCurrentTask = (command.command === Commands.Task && command.testId === testId); + if (isCurrentTask || command.command === Commands.Clear) { + this.unattachObserver(observer); + clearTimeout(timeout); + resolve(); + } + }); + + const timeout = setTimeout(() => { + console.error( + `Test task ${testId} took to long to execute.`, + "Please check if the dispatcher was initialized and is not stopped.", + "Bailing out." + ); + this.unattachObserver(observer); + reject(); + }, 1000); + + this.attachObserver(observer); + }); + } } export default Dispatcher; diff --git a/src/metrics/types/boolean.ts b/src/metrics/types/boolean.ts index e7fbf13ff..86558d32a 100644 --- a/src/metrics/types/boolean.ts +++ b/src/metrics/types/boolean.ts @@ -39,7 +39,7 @@ class BooleanMetricType extends MetricType { if (!this.shouldRecord()) { return; } - + const metric = new BooleanMetric(value); await Glean.metricsDatabase.record(this, metric); }); @@ -59,7 +59,11 @@ class BooleanMetricType extends MetricType { * @returns The value found in storage or `undefined` if nothing was found. */ async testGetValue(ping: string): Promise { - return Glean.metricsDatabase.getMetric(ping, this); + let metric: boolean | undefined; + await Glean.dispatcher.testLaunch(async () => { + metric = await Glean.metricsDatabase.getMetric(ping, this); + }); + return metric; } } diff --git a/src/metrics/types/counter.ts b/src/metrics/types/counter.ts index 1193da8c4..66eff4b90 100644 --- a/src/metrics/types/counter.ts +++ b/src/metrics/types/counter.ts @@ -106,7 +106,11 @@ class CounterMetricType extends MetricType { * @returns The value found in storage or `undefined` if nothing was found. */ async testGetValue(ping: string): Promise { - return Glean.metricsDatabase.getMetric(ping, this); + let metric: number | undefined; + await Glean.dispatcher.testLaunch(async () => { + metric = await Glean.metricsDatabase.getMetric(ping, this); + }); + return metric; } } diff --git a/src/metrics/types/datetime.ts b/src/metrics/types/datetime.ts index d47126168..07455e2c5 100644 --- a/src/metrics/types/datetime.ts +++ b/src/metrics/types/datetime.ts @@ -226,7 +226,10 @@ class DatetimeMetricType extends MetricType { * @returns The value found in storage or `undefined` if nothing was found. */ private async testGetValueAsDatetimeMetric(ping: string): Promise { - const value = await Glean.metricsDatabase.getMetric(ping, this); + let value: DatetimeInternalRepresentation | undefined; + await Glean.dispatcher.testLaunch(async () => { + value = await Glean.metricsDatabase.getMetric(ping, this); + }); if (value) { return new DatetimeMetric(value); } diff --git a/src/metrics/types/event.ts b/src/metrics/types/event.ts index ffedaa604..2b91ee2ef 100644 --- a/src/metrics/types/event.ts +++ b/src/metrics/types/event.ts @@ -92,11 +92,10 @@ class EventMetricType extends MetricType { */ async testGetValue(ping?: string): Promise { const pingToQuery = ping ?? this.sendInPings[0]; - const events = await Glean.eventsDatabase.getEvents(pingToQuery, this); - if (!events) { - return undefined; - } - + let events: RecordedEvent[] | undefined; + await Glean.dispatcher.testLaunch(async () => { + events = await Glean.eventsDatabase.getEvents(pingToQuery, this); + }); return events; } } diff --git a/src/metrics/types/string.ts b/src/metrics/types/string.ts index 7ccaa339a..21a9655b4 100644 --- a/src/metrics/types/string.ts +++ b/src/metrics/types/string.ts @@ -81,7 +81,11 @@ class StringMetricType extends MetricType { * @returns The value found in storage or `undefined` if nothing was found. */ async testGetValue(ping: string): Promise { - return Glean.metricsDatabase.getMetric(ping, this); + let metric: string | undefined; + await Glean.dispatcher.testLaunch(async () => { + metric = await Glean.metricsDatabase.getMetric(ping, this); + }); + return metric; } } diff --git a/src/metrics/types/uuid.ts b/src/metrics/types/uuid.ts index c0717547a..38596ac18 100644 --- a/src/metrics/types/uuid.ts +++ b/src/metrics/types/uuid.ts @@ -2,39 +2,13 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { v4 as UUIDv4, validate as UUIDvalidate } from "uuid"; +import { validate as UUIDvalidate } from "uuid"; import { KNOWN_CLIENT_ID } from "../../constants"; import { Metric, MetricType, CommonMetricData } from "metrics"; -import { isString } from "utils"; +import { isString, generateUUIDv4 } from "utils"; import Glean from "glean"; -/** - * Generates a UUIDv4. - * - * Will provide a fallback in case `crypto` is not available, - * which makes the "uuid" package generator not work. - * - * # Important - * - * This workaround is here for usage in Qt/QML environments, where `crypto` is not available. - * Bug 1688015 was opened to figure out a less hacky way to do this. - * - * @returns A randomly generated UUIDv4. - */ -function generateUUIDv4(): string { - if (typeof crypto !== "undefined") { - return UUIDv4(); - } else { - // Copied from https://stackoverflow.com/a/2117523/261698 - // and https://stackoverflow.com/questions/105034/how-to-create-guid-uuid - return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, function(c) { - const r = Math.random() * 16 | 0, v = c == "x" ? r : (r & 0x3 | 0x8); - return v.toString(16); - }); - } -} - export class UUIDMetric extends Metric { constructor(v: unknown) { super(v); @@ -127,7 +101,11 @@ class UUIDMetricType extends MetricType { * @returns The value found in storage or `undefined` if nothing was found. */ async testGetValue(ping: string): Promise { - return Glean.metricsDatabase.getMetric(ping, this); + let metric: string | undefined; + await Glean.dispatcher.testLaunch(async () => { + metric = await Glean.metricsDatabase.getMetric(ping, this); + }); + return metric; } } diff --git a/src/utils.ts b/src/utils.ts index 7cfd82db0..0493f61aa 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -2,6 +2,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +import { v4 as UUIDv4 } from "uuid"; + // We will intentionaly leave `null` out even though it is a valid JSON primitive. export type JSONPrimitive = string | number | boolean; export type JSONValue = JSONPrimitive | JSONObject | JSONArray; @@ -120,3 +122,29 @@ export function validateURL(v: string): boolean { const urlPattern = /^(http|https):\/\/[a-zA-Z0-9._-]+(:\d+){0,1}(\/{0,1})$/i; return urlPattern.test(v); } + +/** + * Generates a UUIDv4. + * + * Will provide a fallback in case `crypto` is not available, + * which makes the "uuid" package generator not work. + * + * # Important + * + * This workaround is here for usage in Qt/QML environments, where `crypto` is not available. + * Bug 1688015 was opened to figure out a less hacky way to do this. + * + * @returns A randomly generated UUIDv4. + */ +export function generateUUIDv4(): string { + if (typeof crypto !== "undefined") { + return UUIDv4(); + } else { + // Copied from https://stackoverflow.com/a/2117523/261698 + // and https://stackoverflow.com/questions/105034/how-to-create-guid-uuid + return "xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx".replace(/[xy]/g, function(c) { + const r = Math.random() * 16 | 0, v = c == "x" ? r : (r & 0x3 | 0x8); + return v.toString(16); + }); + } +} diff --git a/tests/dispatcher.spec.ts b/tests/dispatcher.spec.ts index 1f48880d6..e283cb518 100644 --- a/tests/dispatcher.spec.ts +++ b/tests/dispatcher.spec.ts @@ -267,4 +267,133 @@ describe("Dispatcher", function() { assert.strictEqual(stub.callCount, 10); assert.strictEqual(dispatcher["state"], DispatcherState.Stopped); }); + + it("testLaunch will resolve once the task is executed", async function () { + dispatcher = new Dispatcher(); + dispatcher.flushInit(); + + const stub = sandbox.stub().callsFake(sampleTask); + + // Launch a bunch of tasks to clog the queue + for (let i = 0; i < 100; i++) { + dispatcher.launch(sampleTask); + } + + await dispatcher.testLaunch(stub); + + assert.strictEqual(stub.callCount, 1); + }); + + it("testLaunch will also resolve if the queue is cleared", async function () { + dispatcher = new Dispatcher(); + dispatcher.flushInit(); + + const stub = sandbox.stub().callsFake(sampleTask); + + // Launch a bunch of tasks to clog the queue + for (let i = 0; i < 10; i++) { + dispatcher.launch(sampleTask); + } + + // The clear command will be queued before `testLaunch` + // and that will definitely prevent `testLaunch` from being called. + dispatcher.clear(); + await dispatcher.testLaunch(stub); + + assert.strictEqual(stub.callCount, 0); + + // The clear command was launched before the testLaunch command, + // which means it is at the top of the queue and will clear before we get to our command. + // + // The final expected state of the dispatcher, in this case, is stopped. + assert.strictEqual(dispatcher["state"], DispatcherState.Stopped); + }); + + it("testLaunch will resume queue execution if the dispatcher is stopped", async function () { + dispatcher = new Dispatcher(); + + const stub = sandbox.stub().callsFake(sampleTask); + for (let i = 0; i < 10; i++) { + dispatcher.launch(stub); + } + + dispatcher.stop(); + + dispatcher.flushInit(); + await dispatcher.testBlockOnQueue(); + assert.strictEqual(dispatcher["queue"].length, 10); + + const testStub = sandbox.stub().callsFake(sampleTask); + await dispatcher.testLaunch(testStub); + + assert.strictEqual(dispatcher["queue"].length, 0); + assert.strictEqual(stub.callCount, 10); + assert.strictEqual(testStub.callCount, 1); + + // We block here because we are only sure that all the tasks are done executing. + // We are not _guaranteed_ that the `execute` function is done, + // this the state may still be Processing. + await dispatcher.testBlockOnQueue(); + + // If the dispatcher was stopped and we called `testLaunch` + // the dispatcher will return the dispatcher to an idle state. + assert.strictEqual(dispatcher["state"], DispatcherState.Idle); + }); + + it("testLaunch will reject in case the dispatcher is uninitialized for too long", async function () { + dispatcher = new Dispatcher(); + try { + await dispatcher.testLaunch(sampleTask); + assert.ok(false); + } catch { + assert.ok(true); + } + }); + + it("testLaunch will not reject in case the dispatcher is uninitialized, but quickly initializes", async function () { + dispatcher = new Dispatcher(); + const testLaunchedTask = dispatcher.testLaunch(sampleTask); + dispatcher.flushInit(); + try { + await testLaunchedTask; + assert.ok(true); + } catch { + assert.ok(false); + } + }); + + it("launching multiple test tasks at the same time works as expected", async function () { + dispatcher = new Dispatcher(); + dispatcher.flushInit(); + + const stub1 = sandbox.stub().callsFake(sampleTask); + const stub2 = sandbox.stub().callsFake(sampleTask); + const stub3 = sandbox.stub().callsFake(sampleTask); + + const test1 = dispatcher.testLaunch(stub1); + const test2 = dispatcher.testLaunch(stub2); + const test3 = dispatcher.testLaunch(stub3); + + await Promise.all([test1, test2, test3]); + + sinon.assert.callOrder(stub1, stub2, stub3); + }); + + it("testLaunch observers are unattached after promise is resolved or rejected", async function() { + dispatcher = new Dispatcher(); + + const willReject = dispatcher.testLaunch(sampleTask); + assert.strictEqual(dispatcher["observers"].length, 1); + try { + await willReject; + } catch { + assert.strictEqual(dispatcher["observers"].length, 0); + } + + dispatcher.flushInit(); + const willNotReject = dispatcher.testLaunch(sampleTask); + assert.strictEqual(dispatcher["observers"].length, 1); + await willNotReject; + assert.strictEqual(dispatcher["observers"].length, 0); + }); }); From 16507cbab4d166579790af41522fb774c908fdeb Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 5 Feb 2021 15:10:57 +0100 Subject: [PATCH 04/11] Provide a way to execute some metrics recording sync It was inevitable to provide a way to execute recording for specific metrics without disptaching. Let's take for example `ping.submit`. That is a dispatched task. When we submit a ping we always record two metrics: the sequence number and the start_time. If by any chance we lauched three ping submissions in a row the order the dispatcher might execute the tasks is: - ping.submit - ping.submit - ping.submit - seq.add - start_time.set - seq.add - start_time.set - seq.add - start_time.set This would mean that all pings would be sent with the same sequence number and start_time, since the recordings would only happen after all pings were submitted. Intuitively, it seems that all metric recording inside of Glean.js itself should be done synchronously. I opened Bug 1691037 to verify this assumption and write documentation about this. --- src/glean.ts | 24 +++++----- src/internal_metrics.ts | 17 +++---- src/metrics/types/counter.ts | 90 ++++++++++++++++++++--------------- src/metrics/types/datetime.ts | 86 +++++++++++++++++++-------------- src/metrics/types/string.ts | 40 +++++++++++----- src/metrics/types/uuid.ts | 54 +++++++++++++-------- src/pings/maker.ts | 4 +- 7 files changed, 185 insertions(+), 130 deletions(-) diff --git a/src/glean.ts b/src/glean.ts index 33b74ae7f..1f8495e52 100644 --- a/src/glean.ts +++ b/src/glean.ts @@ -10,9 +10,10 @@ import PingUploader from "upload"; import { isUndefined, sanitizeApplicationId } from "utils"; import { CoreMetrics } from "internal_metrics"; import { Lifetime } from "metrics"; -import { DatetimeMetric } from "metrics/types/datetime"; -import Dispatcher from "dispatcher"; import EventsDatabase from "metrics/events_database"; +import UUIDMetricType from "metrics/types/uuid"; +import DatetimeMetricType, { DatetimeMetric } from "metrics/types/datetime"; +import Dispatcher from "dispatcher"; class Glean { // The Glean singleton. @@ -116,11 +117,6 @@ class Glean { * This function is only supposed to be called when telemetry is disabled. */ private static async clearMetrics(): Promise { - // Stops any task execution on the dispatcher. - // - // While stopped, the dispatcher will enqueue but won't execute any tasks it receives. - Glean.dispatcher.stop(); - // Stop ongoing upload jobs and clear pending pings queue. await Glean.pingUploader.clearPendingPingsQueue(); @@ -152,21 +148,23 @@ class Glean { // is not a no-op. // // This is safe. - // Since the dispatcher is stopped, no external API calls will be executed. + // + // `clearMetrics` is either called on `initialize` or `setUploadEnabled`. + // Both are dispatched tasks, which means that any other dispatched task + // called after them will only be executed after they are done. + // Since all external API calls are dispatched, it is not possible + // for any other API call to be execute concurrently with this one. Glean.uploadEnabled = true; // Store a "dummy" KNOWN_CLIENT_ID in the client_id metric. This will // make it easier to detect if pings were unintentionally sent after // uploading is disabled. - Glean.coreMetrics.clientId.set(KNOWN_CLIENT_ID); + await UUIDMetricType._private_setSync(Glean.coreMetrics.clientId, KNOWN_CLIENT_ID); // Restore the first_run_date. - Glean.coreMetrics.firstRunDate.set(firstRunDate); + await DatetimeMetricType._private_setSync(Glean.coreMetrics.firstRunDate, firstRunDate); Glean.uploadEnabled = false; - - // Clear the dispatcher queue. - Glean.dispatcher.clear(); } /** diff --git a/src/internal_metrics.ts b/src/internal_metrics.ts index f1943e7a2..e4b797cbf 100644 --- a/src/internal_metrics.ts +++ b/src/internal_metrics.ts @@ -11,6 +11,7 @@ import TimeUnit from "metrics/time_unit"; import { Lifetime } from "metrics"; import Glean from "glean"; import PlatformInfo from "platform_info"; +import { generateUUIDv4 } from "utils"; export class CoreMetrics { readonly clientId: UUIDMetricType; @@ -96,8 +97,8 @@ export class CoreMetrics { await this.initializeOsVersion(); await this.initializeArchitecture(); await this.initializeLocale(); - this.appBuild.set(appBuild || "Unknown"); - this.appDisplayVersion.set(appDisplayVersion || "Unkown"); + await StringMetricType._private_setSync(this.appBuild, appBuild || "Unknown"); + await StringMetricType._private_setSync(this.appDisplayVersion, appDisplayVersion || "Unknown"); } /** @@ -122,7 +123,7 @@ export class CoreMetrics { } if (needNewClientId) { - this.clientId.generateAndSet(); + await UUIDMetricType._private_setSync(this.clientId, generateUUIDv4()); } } @@ -136,7 +137,7 @@ export class CoreMetrics { ); if (!firstRunDate) { - this.firstRunDate.set(); + await DatetimeMetricType._private_setSync(this.firstRunDate); } } @@ -144,27 +145,27 @@ export class CoreMetrics { * Gets and sets the os. */ async initializeOs(): Promise { - this.os.set(await PlatformInfo.os()); + await StringMetricType._private_setSync(this.os, await PlatformInfo.os()); } /** * Gets and sets the os. */ async initializeOsVersion(): Promise { - this.osVersion.set(await PlatformInfo.osVersion()); + await StringMetricType._private_setSync(this.osVersion, await PlatformInfo.osVersion()); } /** * Gets and sets the system architecture. */ async initializeArchitecture(): Promise { - this.architecture.set(await PlatformInfo.arch()); + await StringMetricType._private_setSync(this.architecture, await PlatformInfo.arch()); } /** * Gets and sets the system / browser locale. */ async initializeLocale(): Promise { - this.locale.set(await PlatformInfo.locale()); + await StringMetricType._private_setSync(this.locale, await PlatformInfo.locale()); } } diff --git a/src/metrics/types/counter.ts b/src/metrics/types/counter.ts index 66eff4b90..82b42e8f0 100644 --- a/src/metrics/types/counter.ts +++ b/src/metrics/types/counter.ts @@ -39,6 +39,57 @@ class CounterMetricType extends MetricType { super("counter", meta); } + /** + * An internal implemention of `add` that does not dispatch the recording task. + * + * # Important + * + * This is absolutely not meant to be used outside of Glean itself. + * It may cause multiple issues because it cannot guarantee + * that the recording of the metric will happen in order with other Glean API calls. + * + * @param instance The metric instance to record to. + * @param amount The amount we want to add. + */ + static async _private_addSync(instance: CounterMetricType, amount?: number): Promise { + if (!instance.shouldRecord()) { + return; + } + + if (isUndefined(amount)) { + amount = 1; + } + + if (amount <= 0) { + // TODO: record error once Bug 1682574 is resolved. + console.warn(`Attempted to add an invalid amount ${amount}. Ignoring.`); + return; + } + + const transformFn = ((amount) => { + return (v?: JSONValue): CounterMetric => { + let metric: CounterMetric; + let result: number; + try { + metric = new CounterMetric(v); + result = metric.get() + amount; + } catch { + metric = new CounterMetric(amount); + result = amount; + } + + if (result > Number.MAX_SAFE_INTEGER) { + result = Number.MAX_SAFE_INTEGER; + } + + metric.set(result); + return metric; + }; + })(amount); + + await Glean.metricsDatabase.transform(instance, transformFn); + } + /** * Increases the counter by `amount`. * @@ -52,44 +103,7 @@ class CounterMetricType extends MetricType { * If not provided will default to `1`. */ add(amount?: number): void { - Glean.dispatcher.launch(async () => { - if (!this.shouldRecord()) { - return; - } - - if (isUndefined(amount)) { - amount = 1; - } - - if (amount <= 0) { - // TODO: record error once Bug 1682574 is resolved. - console.warn(`Attempted to add an invalid amount ${amount}. Ignoring.`); - return; - } - - const transformFn = ((amount) => { - return (v?: JSONValue): CounterMetric => { - let metric: CounterMetric; - let result: number; - try { - metric = new CounterMetric(v); - result = metric.get() + amount; - } catch { - metric = new CounterMetric(amount); - result = amount; - } - - if (result > Number.MAX_SAFE_INTEGER) { - result = Number.MAX_SAFE_INTEGER; - } - - metric.set(result); - return metric; - }; - })(amount); - - await Glean.metricsDatabase.transform(this, transformFn); - }); + Glean.dispatcher.launch(async () => CounterMetricType._private_addSync(this, amount)); } /** diff --git a/src/metrics/types/datetime.ts b/src/metrics/types/datetime.ts index 07455e2c5..8be0bcac8 100644 --- a/src/metrics/types/datetime.ts +++ b/src/metrics/types/datetime.ts @@ -162,54 +162,68 @@ export class DatetimeMetric extends Metric { + if (!instance.shouldRecord()) { + return; + } + + if (!value) { + value = new Date(); + } + + // We always save a milliseconds precision ISO string on the database, + // regardless of the time unit. So we zero out information that + // is not necessary for the current time unit of this metric. + const truncatedDate = value; + switch(instance.timeUnit) { + case (TimeUnit.Day): + truncatedDate.setMilliseconds(0); + truncatedDate.setSeconds(0); + truncatedDate.setMinutes(0); + truncatedDate.setMilliseconds(0); + case (TimeUnit.Hour): + truncatedDate.setMilliseconds(0); + truncatedDate.setSeconds(0); + truncatedDate.setMinutes(0); + case (TimeUnit.Minute): + truncatedDate.setMilliseconds(0); + truncatedDate.setSeconds(0); + case (TimeUnit.Second): + truncatedDate.setMilliseconds(0); + default: + break; + } + + const metric = DatetimeMetric.fromDate(value, instance.timeUnit); + await Glean.metricsDatabase.record(instance, metric); + } + /** * Set a datetime value, truncating it to the metric's resolution. * * @param value The Date value to set. If not provided, will record the current time. */ set(value?: Date): void { - Glean.dispatcher.launch(async () => { - if (!this.shouldRecord()) { - return; - } - - if (!value) { - value = new Date(); - } - - // We always save a milliseconds precision ISO string on the database, - // regardless of the time unit. So we zero out information that - // is not necessary for the current time unit of this metric. - const truncatedDate = value; - switch(this.timeUnit) { - case (TimeUnit.Day): - truncatedDate.setMilliseconds(0); - truncatedDate.setSeconds(0); - truncatedDate.setMinutes(0); - truncatedDate.setMilliseconds(0); - case (TimeUnit.Hour): - truncatedDate.setMilliseconds(0); - truncatedDate.setSeconds(0); - truncatedDate.setMinutes(0); - case (TimeUnit.Minute): - truncatedDate.setMilliseconds(0); - truncatedDate.setSeconds(0); - case (TimeUnit.Second): - truncatedDate.setMilliseconds(0); - default: - break; - } - - const metric = DatetimeMetric.fromDate(value, this.timeUnit); - await Glean.metricsDatabase.record(this, metric); - }); + Glean.dispatcher.launch(() => DatetimeMetricType._private_setSync(this, value)); } /** diff --git a/src/metrics/types/string.ts b/src/metrics/types/string.ts index 21a9655b4..3fa25c23b 100644 --- a/src/metrics/types/string.ts +++ b/src/metrics/types/string.ts @@ -41,6 +41,32 @@ class StringMetricType extends MetricType { super("string", meta); } + /** + * An internal implemention of `set` that does not dispatch the recording task. + * + * # Important + * + * This is absolutely not meant to be used outside of Glean itself. + * It may cause multiple issues because it cannot guarantee + * that the recording of the metric will happen in order with other Glean API calls. + * + * @param instance The metric instance to record to. + * @param value The string we want to set to. + */ + static async _private_setSync(instance: StringMetricType, value: string): Promise { + if (!instance.shouldRecord()) { + return; + } + + if (value.length > MAX_LENGTH_VALUE) { + // TODO: record error once Bug 1682574 is resolved. + console.warn(`String ${value} is longer than ${MAX_LENGTH_VALUE} chars. Truncating.`); + } + + const metric = new StringMetric(value.substr(0, MAX_LENGTH_VALUE)); + await Glean.metricsDatabase.record(instance, metric); + } + /** * Sets to the specified string value. * @@ -52,19 +78,7 @@ class StringMetricType extends MetricType { * @param value the value to set. */ set(value: string): void { - Glean.dispatcher.launch(async () => { - if (!this.shouldRecord()) { - return; - } - - if (value.length > MAX_LENGTH_VALUE) { - // TODO: record error once Bug 1682574 is resolved. - console.warn(`String ${value} is longer than ${MAX_LENGTH_VALUE} chars. Truncating.`); - } - - const metric = new StringMetric(value.substr(0, MAX_LENGTH_VALUE)); - await Glean.metricsDatabase.record(this, metric); - }); + Glean.dispatcher.launch(() => StringMetricType._private_setSync(this, value)); } /** diff --git a/src/metrics/types/uuid.ts b/src/metrics/types/uuid.ts index 38596ac18..eacf66ea0 100644 --- a/src/metrics/types/uuid.ts +++ b/src/metrics/types/uuid.ts @@ -41,6 +41,39 @@ class UUIDMetricType extends MetricType { super("uuid", meta); } + /** + * An internal implemention of `set` that does not dispatch the recording task. + * + * # Important + * + * This is absolutely not meant to be used outside of Glean itself. + * It may cause multiple issues because it cannot guarantee + * that the recording of the metric will happen in order with other Glean API calls. + * + * @param instance The metric instance to record to. + * @param value The UUID we want to set to. + */ + static async _private_setSync(instance: UUIDMetricType, value: string): Promise { + if (!instance.shouldRecord()) { + return; + } + + if (!value) { + value = generateUUIDv4(); + } + + let metric: UUIDMetric; + try { + metric = new UUIDMetric(value); + } catch { + // TODO: record error once Bug 1682574 is resolved. + console.warn(`"${value}" is not a valid UUID. Ignoring`); + return; + } + + await Glean.metricsDatabase.record(instance, metric); + } + /** * Sets to the specified value. * @@ -49,26 +82,7 @@ class UUIDMetricType extends MetricType { * @throws In case `value` is not a valid UUID. */ set(value: string): void { - Glean.dispatcher.launch(async () => { - if (!this.shouldRecord()) { - return; - } - - if (!value) { - value = generateUUIDv4(); - } - - let metric: UUIDMetric; - try { - metric = new UUIDMetric(value); - } catch { - // TODO: record error once Bug 1682574 is resolved. - console.warn(`"${value}" is not a valid UUID. Ignoring`); - return; - } - - await Glean.metricsDatabase.record(this, metric); - }); + Glean.dispatcher.launch(() => UUIDMetricType._private_setSync(this, value)); } /** diff --git a/src/pings/maker.ts b/src/pings/maker.ts index 09786e40c..70ad87f6e 100644 --- a/src/pings/maker.ts +++ b/src/pings/maker.ts @@ -31,7 +31,7 @@ export async function getSequenceNumber(ping: PingType): Promise { }); const currentSeqData = await Glean.metricsDatabase.getMetric(PING_INFO_STORAGE, seq); - seq.add(1); + await CounterMetricType._private_addSync(seq, 1); if (currentSeqData) { // Creating a new counter metric validates that the metric stored is actually a number. @@ -77,7 +77,7 @@ export async function getStartEndTimes(ping: PingType): Promise<{ startTime: str // Update the start time with the current time. const endTimeData = new Date(); - start.set(endTimeData); + await DatetimeMetricType._private_setSync(start, endTimeData); const endTime = DatetimeMetric.fromDate(endTimeData, TimeUnit.Minute); return { From f131e10600b16570317c14dfa842f6b4916ae7ef Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 5 Feb 2021 15:19:03 +0100 Subject: [PATCH 05/11] Fix some of the last failing tests due to the dispatcher --- tests/glean.spec.ts | 16 ++++++++++++++-- tests/pings/index.spec.ts | 29 ++++++++++++++++------------- tests/pings/maker.spec.ts | 1 + 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/tests/glean.spec.ts b/tests/glean.spec.ts index 6320e3e2c..6c44a7495 100644 --- a/tests/glean.spec.ts +++ b/tests/glean.spec.ts @@ -126,11 +126,20 @@ describe("Glean", function() { }); it("enabling when already enabled is a no-op", async function() { - // TODO: Re-implement this now that this function doesn't return a boolean anymore. + const spy = sandbox.spy(Glean["coreMetrics"], "initialize"); + Glean.setUploadEnabled(true); + // Wait for `setUploadEnabled` to be executed. + await Glean.dispatcher.testBlockOnQueue(); + assert.strictEqual(spy.callCount, 0); }); it("disabling when already disabled is a no-op", async function() { - // TODO: Re-implement this now that this function doesn't return a boolean anymore. + const spy = sandbox.spy(Glean["pingUploader"], "clearPendingPingsQueue"); + Glean.setUploadEnabled(false); + Glean.setUploadEnabled(false); + // Wait for `setUploadEnabled` to be executed both times. + await Glean.dispatcher.testBlockOnQueue(); + assert.strictEqual(spy.callCount, 1); }); it("initialization throws if applicationId is an empty string", async function() { @@ -178,6 +187,9 @@ describe("Glean", function() { it("initializing twice is a no-op", async function() { await Glean.testUninitialize(); Glean.initialize(GLOBAL_APPLICATION_ID, true); + // initialize is dispatched, we must await on the queue being completed to assert things. + await Glean.dispatcher.testBlockOnQueue(); + // This time it should not be called, which means upload should not be switched to `false`. Glean.initialize(GLOBAL_APPLICATION_ID, false); assert.ok(Glean.isUploadEnabled()); diff --git a/tests/pings/index.spec.ts b/tests/pings/index.spec.ts index 8644e5ff0..b53211229 100644 --- a/tests/pings/index.spec.ts +++ b/tests/pings/index.spec.ts @@ -12,6 +12,17 @@ import Glean from "glean"; const sandbox = sinon.createSandbox(); +/** + * Submits a ping and waits for the dispatcher queue to be completed. + * + * @param ping The ping to submit. + */ +async function submitSync(ping: PingType): Promise { + ping.submit(); + // TODO: Drop this whole approach once Bug 1691033 is resolved. + await Glean.dispatcher.testBlockOnQueue(); +} + describe("PingType", function() { afterEach(function () { sandbox.restore(); @@ -35,9 +46,7 @@ describe("PingType", function() { }); counter.add(); - assert.ok(ping.submit()); - // TODO: Make this nicer once we have a nice way to check if pings are enqueued, - // possibly once Bug 1677440 is resolved. + await submitSync(ping); const storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); assert.strictEqual(Object.keys(storedPings).length, 1); }); @@ -49,13 +58,11 @@ describe("PingType", function() { const ping1 = new PingType("ping1", true, false, []); const ping2 = new PingType("ping2", true, true, []); - // TODO: Make this nicer once we have a nice way to check if pings are enqueued, - // possibly once Bug 1677440 is resolved. - assert.ok(ping1.submit()); + await submitSync(ping1); let storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); assert.strictEqual(Object.keys(storedPings).length, 0); - assert.ok(ping2.submit()); + await submitSync(ping2); storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); assert.strictEqual(Object.keys(storedPings).length, 1); }); @@ -64,9 +71,7 @@ describe("PingType", function() { Glean.setUploadEnabled(false); const ping = new PingType("custom", true, false, []); - assert.strictEqual(ping.submit(), false); - // TODO: Make this nicer once we have a nice way to check if pings are enqueued, - // possibly once Bug 1677440 is resolved. + await submitSync(ping); const storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); assert.strictEqual(Object.keys(storedPings).length, 0); }); @@ -75,9 +80,7 @@ describe("PingType", function() { await Glean.testUninitialize(); const ping = new PingType("custom", true, false, []); - assert.strictEqual(ping.submit(), false); - // TODO: Make this nicer once we have a nice way to check if pings are enqueued, - // possibly once Bug 1677440 is resolved. + await submitSync(ping); const storedPings = await Glean.pingsDatabase["store"]._getWholeStore(); assert.strictEqual(Object.keys(storedPings).length, 0); }); diff --git a/tests/pings/maker.spec.ts b/tests/pings/maker.spec.ts index d993f7de7..1c91c0db2 100644 --- a/tests/pings/maker.spec.ts +++ b/tests/pings/maker.spec.ts @@ -44,6 +44,7 @@ describe("PingMaker", function() { appDisplayVersion: "display version", serverEndpoint: "http://localhost:8080" }); + await Glean.dispatcher.testBlockOnQueue(); const clientInfo2 = await PingMaker.buildClientInfoSection(ping); assert.ok("telemetry_sdk_build" in clientInfo2); From 8989ca67d4af12d4ede44d83877b15f68fd0b12b Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 5 Feb 2021 15:25:58 +0100 Subject: [PATCH 06/11] EXTRA: Delete useless test file --- tests/index.spec.ts | 11 ----------- 1 file changed, 11 deletions(-) delete mode 100644 tests/index.spec.ts diff --git a/tests/index.spec.ts b/tests/index.spec.ts deleted file mode 100644 index dce739bb9..000000000 --- a/tests/index.spec.ts +++ /dev/null @@ -1,11 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -import assert from "assert"; - -describe("Sample test!", function() { - it("1 + 1 should always equal 2", function() { - assert.equal(1 + 1, 2); - }); -}); From 023f770087cb836aa7866dc78f6f2d2c5c152f76 Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 5 Feb 2021 15:49:50 +0100 Subject: [PATCH 07/11] Port 'flipping upload enabled respects order of events' --- tests/glean.spec.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/glean.spec.ts b/tests/glean.spec.ts index 6c44a7495..798822076 100644 --- a/tests/glean.spec.ts +++ b/tests/glean.spec.ts @@ -9,6 +9,8 @@ import { CLIENT_INFO_STORAGE, KNOWN_CLIENT_ID } from "../src/constants"; import Glean from "glean"; import { Lifetime } from "metrics"; import StringMetricType from "metrics/types/string"; +import PingType from "pings"; +import Uploader from "upload/uploader"; const GLOBAL_APPLICATION_ID = "org.mozilla.glean.test.app"; const sandbox = sinon.createSandbox(); @@ -194,4 +196,23 @@ describe("Glean", function() { Glean.initialize(GLOBAL_APPLICATION_ID, false); assert.ok(Glean.isUploadEnabled()); }); + + it("flipping upload enabled respects order of events", async function() { + await Glean.testUninitialize(); + + const ping = new PingType("custom", true, true, []); + const postSpy = sandbox.spy(Uploader, "post"); + + // Start Glean with upload enabled. + Glean.initialize(GLOBAL_APPLICATION_ID, true); + // Immediatelly disable upload. + Glean.setUploadEnabled(false); + ping.submit(); + + await Glean.dispatcher.testBlockOnQueue(); + + assert.strictEqual(postSpy.callCount, 0); + + // TODO: Check that a deletion request ping was sent after Bug 1686685 is resolved. + }); }); From 03140c6a2404d11393cd86241aba57e0f10b3a96 Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 5 Feb 2021 15:53:09 +0100 Subject: [PATCH 08/11] Update the web-extensio sample --- samples/web-extension/index.js | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/samples/web-extension/index.js b/samples/web-extension/index.js index 16afba67e..3c0d4bb61 100644 --- a/samples/web-extension/index.js +++ b/samples/web-extension/index.js @@ -8,30 +8,20 @@ import Glean from "glean"; import { samplePing } from "./generatedPings"; import { webExtStarted, popupOpened } from "./generatedMetrics"; -// TODO: Do not wait for Glean to be initialized before recording metrics -// once Bug 1687491 is resolved. -Glean.initialize("web-extension", true) - .then(() => { - console.log("Glean has been succesfully initialized."); - webExtStarted.set() - .then(() => console.log("`webext-installed` was succesfully set.")); - }); + +Glean.initialize("web-extension", true); +webExtStarted.set(); // Listen for messages from the popup. browser.runtime.onMessage.addListener(msg => { console.log(`New message received! ${msg}`); if (msg === "popup-opened") { - popupOpened.add() - .then(() => console.log("`popup-opened` was succesfully added.")); + popupOpened.add(); } if (msg === "send-ping") { - samplePing.submit() - .then(wasSubmitted => { - console.log( - `Attempted to send ping "${samplePing.name}". Was the ping sent? ${wasSubmitted}`); - }); + samplePing.submit(); } }); From fedf266de0de2e6709f2e8f96a865261fc698e58 Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 5 Feb 2021 16:06:12 +0100 Subject: [PATCH 09/11] Update the Qt sample and CI check --- bin/qt-js-check.sh | 8 +------- samples/qt-qml-app/main.qml | 6 +----- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/bin/qt-js-check.sh b/bin/qt-js-check.sh index 0c2fd3788..0b71bbd32 100755 --- a/bin/qt-js-check.sh +++ b/bin/qt-js-check.sh @@ -18,14 +18,8 @@ xvfb-run python main.py &> qml.log & sleep 10 -if grep -q "Some Javascript error occured" "qml.log"; then +if grep -q "Error executing task:" "qml.log"; then echo "Failed to initialize Glean in QML! See more logs below." cat qml.log exit 1 fi - -if ! grep -q "Called Glean.initialize" "qml.log"; then - echo "Unable to initialize Glean in QML! See more logs below." - cat qml.log - exit 1 -fi diff --git a/samples/qt-qml-app/main.qml b/samples/qt-qml-app/main.qml index ef55779e4..6a792d002 100644 --- a/samples/qt-qml-app/main.qml +++ b/samples/qt-qml-app/main.qml @@ -48,10 +48,6 @@ Rectangle { Component.onCompleted: { // Initialize Glean when the application starts. - Glean.Glean.initialize("qt-qml-app", true) - // Note: If you change this console messages you should change - // the console message verifications on `bin/qt-js-check.sh. - .then(() => console.log("Called Glean.initialize succesfully.")) - .catch(err => console.error(`Some Javascript error occured.\n${err}`)); + Glean.Glean.initialize("qt-qml-app", true); } } From f51d87272bed15377d038a85060d7330942ab612 Mon Sep 17 00:00:00 2001 From: Beatriz Rizental Date: Fri, 5 Feb 2021 18:03:03 +0100 Subject: [PATCH 10/11] Apply suggestions from code review Co-authored-by: Alessio Placitelli --- src/glean.ts | 2 +- src/storage/persistent/webext.ts | 2 +- src/storage/weak.ts | 2 -- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/glean.ts b/src/glean.ts index 1f8495e52..1000db11d 100644 --- a/src/glean.ts +++ b/src/glean.ts @@ -202,7 +202,7 @@ class Glean { throw new Error("Unable to initialize Glean, applicationId cannot be an empty string."); } - // The configuration constructor will throw in case config hany any incorrect prop. + // The configuration constructor will throw in case config has any incorrect prop. const correctConfig = new Configuration(config); // Initialize the dispatcher and execute init before any other enqueued task. diff --git a/src/storage/persistent/webext.ts b/src/storage/persistent/webext.ts index 48083bad6..8c627acb9 100644 --- a/src/storage/persistent/webext.ts +++ b/src/storage/persistent/webext.ts @@ -146,7 +146,7 @@ class WebExtStore implements Store { ); return this.store.set(query); } catch(e) { - console.warn((e as Error).message, "Ignoring"); + console.warn("Ignoring key", e); } } } diff --git a/src/storage/weak.ts b/src/storage/weak.ts index dcd7c9cbe..52dabad00 100644 --- a/src/storage/weak.ts +++ b/src/storage/weak.ts @@ -5,7 +5,6 @@ import { Store, StorageIndex } from "storage"; import { updateNestedObject, getValueFromNestedObject, deleteKeyFromNestedObject } from "storage/utils"; import { JSONObject, JSONValue } from "utils"; - /** * A weak implementation for the Store interface. * @@ -52,5 +51,4 @@ class WeakStore implements Store { return Promise.resolve(); } } - export default WeakStore; From 38233ca16f634300881fe840eea82c6698023835 Mon Sep 17 00:00:00 2001 From: brizental Date: Mon, 8 Feb 2021 11:15:17 +0100 Subject: [PATCH 11/11] setSync -> setUndispatched --- src/glean.ts | 4 ++-- src/internal_metrics.ts | 16 ++++++++-------- src/metrics/types/counter.ts | 4 ++-- src/metrics/types/datetime.ts | 4 ++-- src/metrics/types/string.ts | 4 ++-- src/metrics/types/uuid.ts | 4 ++-- src/pings/maker.ts | 4 ++-- 7 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/glean.ts b/src/glean.ts index 1000db11d..f8f6323d9 100644 --- a/src/glean.ts +++ b/src/glean.ts @@ -159,10 +159,10 @@ class Glean { // Store a "dummy" KNOWN_CLIENT_ID in the client_id metric. This will // make it easier to detect if pings were unintentionally sent after // uploading is disabled. - await UUIDMetricType._private_setSync(Glean.coreMetrics.clientId, KNOWN_CLIENT_ID); + await UUIDMetricType._private_setUndispatched(Glean.coreMetrics.clientId, KNOWN_CLIENT_ID); // Restore the first_run_date. - await DatetimeMetricType._private_setSync(Glean.coreMetrics.firstRunDate, firstRunDate); + await DatetimeMetricType._private_setUndispatched(Glean.coreMetrics.firstRunDate, firstRunDate); Glean.uploadEnabled = false; } diff --git a/src/internal_metrics.ts b/src/internal_metrics.ts index e4b797cbf..b437f7a3c 100644 --- a/src/internal_metrics.ts +++ b/src/internal_metrics.ts @@ -97,8 +97,8 @@ export class CoreMetrics { await this.initializeOsVersion(); await this.initializeArchitecture(); await this.initializeLocale(); - await StringMetricType._private_setSync(this.appBuild, appBuild || "Unknown"); - await StringMetricType._private_setSync(this.appDisplayVersion, appDisplayVersion || "Unknown"); + await StringMetricType._private_setUndispatched(this.appBuild, appBuild || "Unknown"); + await StringMetricType._private_setUndispatched(this.appDisplayVersion, appDisplayVersion || "Unknown"); } /** @@ -123,7 +123,7 @@ export class CoreMetrics { } if (needNewClientId) { - await UUIDMetricType._private_setSync(this.clientId, generateUUIDv4()); + await UUIDMetricType._private_setUndispatched(this.clientId, generateUUIDv4()); } } @@ -137,7 +137,7 @@ export class CoreMetrics { ); if (!firstRunDate) { - await DatetimeMetricType._private_setSync(this.firstRunDate); + await DatetimeMetricType._private_setUndispatched(this.firstRunDate); } } @@ -145,27 +145,27 @@ export class CoreMetrics { * Gets and sets the os. */ async initializeOs(): Promise { - await StringMetricType._private_setSync(this.os, await PlatformInfo.os()); + await StringMetricType._private_setUndispatched(this.os, await PlatformInfo.os()); } /** * Gets and sets the os. */ async initializeOsVersion(): Promise { - await StringMetricType._private_setSync(this.osVersion, await PlatformInfo.osVersion()); + await StringMetricType._private_setUndispatched(this.osVersion, await PlatformInfo.osVersion()); } /** * Gets and sets the system architecture. */ async initializeArchitecture(): Promise { - await StringMetricType._private_setSync(this.architecture, await PlatformInfo.arch()); + await StringMetricType._private_setUndispatched(this.architecture, await PlatformInfo.arch()); } /** * Gets and sets the system / browser locale. */ async initializeLocale(): Promise { - await StringMetricType._private_setSync(this.locale, await PlatformInfo.locale()); + await StringMetricType._private_setUndispatched(this.locale, await PlatformInfo.locale()); } } diff --git a/src/metrics/types/counter.ts b/src/metrics/types/counter.ts index 82b42e8f0..21c0eadd1 100644 --- a/src/metrics/types/counter.ts +++ b/src/metrics/types/counter.ts @@ -51,7 +51,7 @@ class CounterMetricType extends MetricType { * @param instance The metric instance to record to. * @param amount The amount we want to add. */ - static async _private_addSync(instance: CounterMetricType, amount?: number): Promise { + static async _private_addUndispatched(instance: CounterMetricType, amount?: number): Promise { if (!instance.shouldRecord()) { return; } @@ -103,7 +103,7 @@ class CounterMetricType extends MetricType { * If not provided will default to `1`. */ add(amount?: number): void { - Glean.dispatcher.launch(async () => CounterMetricType._private_addSync(this, amount)); + Glean.dispatcher.launch(async () => CounterMetricType._private_addUndispatched(this, amount)); } /** diff --git a/src/metrics/types/datetime.ts b/src/metrics/types/datetime.ts index 8be0bcac8..00c7c1a03 100644 --- a/src/metrics/types/datetime.ts +++ b/src/metrics/types/datetime.ts @@ -181,7 +181,7 @@ class DatetimeMetricType extends MetricType { * @param instance The metric instance to record to. * @param value The date we want to set to. */ - static async _private_setSync(instance: DatetimeMetricType, value?: Date): Promise { + static async _private_setUndispatched(instance: DatetimeMetricType, value?: Date): Promise { if (!instance.shouldRecord()) { return; } @@ -223,7 +223,7 @@ class DatetimeMetricType extends MetricType { * @param value The Date value to set. If not provided, will record the current time. */ set(value?: Date): void { - Glean.dispatcher.launch(() => DatetimeMetricType._private_setSync(this, value)); + Glean.dispatcher.launch(() => DatetimeMetricType._private_setUndispatched(this, value)); } /** diff --git a/src/metrics/types/string.ts b/src/metrics/types/string.ts index 3fa25c23b..55038eb5a 100644 --- a/src/metrics/types/string.ts +++ b/src/metrics/types/string.ts @@ -53,7 +53,7 @@ class StringMetricType extends MetricType { * @param instance The metric instance to record to. * @param value The string we want to set to. */ - static async _private_setSync(instance: StringMetricType, value: string): Promise { + static async _private_setUndispatched(instance: StringMetricType, value: string): Promise { if (!instance.shouldRecord()) { return; } @@ -78,7 +78,7 @@ class StringMetricType extends MetricType { * @param value the value to set. */ set(value: string): void { - Glean.dispatcher.launch(() => StringMetricType._private_setSync(this, value)); + Glean.dispatcher.launch(() => StringMetricType._private_setUndispatched(this, value)); } /** diff --git a/src/metrics/types/uuid.ts b/src/metrics/types/uuid.ts index eacf66ea0..5c3b6ff7a 100644 --- a/src/metrics/types/uuid.ts +++ b/src/metrics/types/uuid.ts @@ -53,7 +53,7 @@ class UUIDMetricType extends MetricType { * @param instance The metric instance to record to. * @param value The UUID we want to set to. */ - static async _private_setSync(instance: UUIDMetricType, value: string): Promise { + static async _private_setUndispatched(instance: UUIDMetricType, value: string): Promise { if (!instance.shouldRecord()) { return; } @@ -82,7 +82,7 @@ class UUIDMetricType extends MetricType { * @throws In case `value` is not a valid UUID. */ set(value: string): void { - Glean.dispatcher.launch(() => UUIDMetricType._private_setSync(this, value)); + Glean.dispatcher.launch(() => UUIDMetricType._private_setUndispatched(this, value)); } /** diff --git a/src/pings/maker.ts b/src/pings/maker.ts index 70ad87f6e..7bf259f8f 100644 --- a/src/pings/maker.ts +++ b/src/pings/maker.ts @@ -31,7 +31,7 @@ export async function getSequenceNumber(ping: PingType): Promise { }); const currentSeqData = await Glean.metricsDatabase.getMetric(PING_INFO_STORAGE, seq); - await CounterMetricType._private_addSync(seq, 1); + await CounterMetricType._private_addUndispatched(seq, 1); if (currentSeqData) { // Creating a new counter metric validates that the metric stored is actually a number. @@ -77,7 +77,7 @@ export async function getStartEndTimes(ping: PingType): Promise<{ startTime: str // Update the start time with the current time. const endTimeData = new Date(); - await DatetimeMetricType._private_setSync(start, endTimeData); + await DatetimeMetricType._private_setUndispatched(start, endTimeData); const endTime = DatetimeMetric.fromDate(endTimeData, TimeUnit.Minute); return {