diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e9642db6..7aae8b124 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ * [#1233](https://github.com/mozilla/glean.js/pull/1233): Add optional `buildDate` argument to `initialize` configuration. The build date can be generated by glean_parser. * [#1233](https://github.com/mozilla/glean.js/pull/1233): Update glean_parser to version 5.1.0. * [#1217](https://github.com/mozilla/glean.js/pull/1217): Record `InvalidType` error when incorrectly type values are passed to metric recording functions. +* [#1267](https://github.com/mozilla/glean.js/pull/1267): Implement the 'events' ping. # v0.32.0 (2022-03-01) diff --git a/glean/src/core/config.ts b/glean/src/core/config.ts index 28277ca63..71d54a437 100644 --- a/glean/src/core/config.ts +++ b/glean/src/core/config.ts @@ -11,6 +11,10 @@ import { Context } from "./context.js"; const LOG_TAG = "core.Config"; +// The default maximum number of events Glean will store before submitting the events ping. +// If the maximum is hit, the events ping is sent immediately. +const DEFAULT_MAX_EVENTS = 500; + /** * Lists Glean's debug options. */ @@ -35,6 +39,8 @@ export interface ConfigurationInterface { readonly appDisplayVersion?: string, // The server pings are sent to. readonly serverEndpoint?: string, + // The maximum number of events to store before submitting the events ping. + readonly maxEvents?: number, // Optional list of plugins to include in current Glean instance. plugins?: Plugin[], // The HTTP client implementation to use for uploading pings. @@ -61,6 +67,7 @@ export class Configuration implements ConfigurationInterface { readonly architecture?: string; readonly osVersion?: string; readonly buildDate?: Date; + readonly maxEvents: number; // Debug configuration. debug: DebugOptions; @@ -74,6 +81,7 @@ export class Configuration implements ConfigurationInterface { this.architecture = config?.architecture; this.osVersion = config?.osVersion; this.buildDate = config?.buildDate; + this.maxEvents = config?.maxEvents || DEFAULT_MAX_EVENTS; this.debug = {}; diff --git a/glean/src/core/constants.ts b/glean/src/core/constants.ts index 6e3b3a7ee..2dd9b480f 100644 --- a/glean/src/core/constants.ts +++ b/glean/src/core/constants.ts @@ -37,6 +37,9 @@ export const DEFAULT_TELEMETRY_ENDPOINT = "https://incoming.telemetry.mozilla.or // The name of the deletion-request ping. export const DELETION_REQUEST_PING_NAME = "deletion-request"; +// The name of the events ping. +export const EVENTS_PING_NAME = "events"; + // The maximum amount of source tags a user can set. export const GLEAN_MAX_SOURCE_TAGS = 5; diff --git a/glean/src/core/context.ts b/glean/src/core/context.ts index 1a38495f2..4b65c366c 100644 --- a/glean/src/core/context.ts +++ b/glean/src/core/context.ts @@ -12,6 +12,8 @@ import type { JSONValue } from "./utils.js"; import Dispatcher from "./dispatcher.js"; import log, { LoggingLevel } from "./log.js"; import type { Configuration } from "./config.js"; +import type CorePings from "./internal_pings.js"; +import type { CoreMetrics } from "./internal_metrics.js"; const LOG_TAG = "core.Context"; @@ -32,6 +34,8 @@ export class Context { private _dispatcher: Dispatcher; private _platform!: Platform; + private _corePings!: CorePings; + private _coreMetrics!: CoreMetrics; // The following group of properties are all set on Glean.initialize // Attempting to get them before they are set will log an error. @@ -230,6 +234,22 @@ export class Context { Context.instance._testing = flag; } + static get corePings(): CorePings { + return Context.instance._corePings; + } + + static set corePings(pings: CorePings) { + Context.instance._corePings = pings; + } + + static get coreMetrics(): CoreMetrics { + return Context.instance._coreMetrics; + } + + static set coreMetrics(metrics: CoreMetrics) { + Context.instance._coreMetrics = metrics; + } + static set platform(platform: Platform) { Context.instance._platform = platform; } diff --git a/glean/src/core/glean.ts b/glean/src/core/glean.ts index 14dff1f40..1bd00c5ca 100644 --- a/glean/src/core/glean.ts +++ b/glean/src/core/glean.ts @@ -23,15 +23,6 @@ import log, { LoggingLevel } from "./log.js"; const LOG_TAG = "core.Glean"; namespace Glean { - // The below properties are exported for testing purposes. - // - // Instances of Glean's core metrics. - // - // Disabling the lint, because we will actually re-assign this variable in the testInitializeGlean API. - // eslint-disable-next-line prefer-const - export let coreMetrics = new CoreMetrics(); - // Instances of Glean's core pings. - export const corePings = new CorePings(); // An instance of the ping uploader. export let pingUploader: PingUploadManager; @@ -51,7 +42,7 @@ namespace Glean { */ async function onUploadEnabled(): Promise { Context.uploadEnabled = true; - await coreMetrics.initialize(); + await Context.coreMetrics.initialize(); } /** @@ -70,7 +61,7 @@ namespace Glean { // We need to use an undispatched submission to guarantee that the // ping is collected before metric are cleared, otherwise we end up // with malformed pings. - await corePings.deletionRequest.submitUndispatched(); + await Context.corePings.deletionRequest.submitUndispatched(); await clearMetrics(); } @@ -97,7 +88,7 @@ namespace Glean { firstRunDate = new DatetimeMetric( await Context.metricsDatabase.getMetric( CLIENT_INFO_STORAGE, - coreMetrics.firstRunDate + Context.coreMetrics.firstRunDate ) ).date; } catch { @@ -124,10 +115,10 @@ namespace 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 coreMetrics.clientId.setUndispatched(KNOWN_CLIENT_ID); + await Context.coreMetrics.clientId.setUndispatched(KNOWN_CLIENT_ID); // Restore the first_run_date. - await coreMetrics.firstRunDate.setUndispatched(firstRunDate); + await Context.coreMetrics.firstRunDate.setUndispatched(firstRunDate); Context.uploadEnabled = false; } @@ -194,6 +185,9 @@ namespace Glean { return; } + Context.coreMetrics = new CoreMetrics(); + Context.corePings = new CorePings(); + Context.applicationId = sanitizeApplicationId(applicationId); // The configuration constructor will throw in case config has any incorrect prop. @@ -224,14 +218,20 @@ namespace Glean { // // The dispatcher will catch and log any exceptions. Context.dispatcher.flushInit(async () => { - // We need to mark Glean as initialized before dealing with the upload status, - // otherwise we will not be able to submit deletion-request pings if necessary. - // - // This is fine, we are inside a dispatched task that is guaranteed to run before any - // other task. No external API call will be executed before we leave this task. Context.initialized = true; Context.uploadEnabled = uploadEnabled; + + // Initialize the events database. + // + // It's important this happens _after_ the upload state is set, + // because initializing the events database may record the execution_counter and + // glean.restarted metrics. If the upload state is not defined these metrics cannot be recorded. + // + // This may also submit an 'events' ping, + // so it also needs to happen before application lifetime metrics are cleared. + await Context.eventsDatabase.initialize(); + // The upload enabled flag may have changed since the last run, for // example by the changing of a config file. if (uploadEnabled) { @@ -259,7 +259,7 @@ namespace Glean { // deletion request ping. const clientId = await Context.metricsDatabase.getMetric( CLIENT_INFO_STORAGE, - coreMetrics.clientId + Context.coreMetrics.clientId ); if (clientId) { @@ -273,13 +273,6 @@ namespace Glean { } } - // Initialize the events database. - // - // It's important this happens _after_ the upload state is dealt with, - // because initializing the events database may record the execution_counter and - // glean.restarted metrics. If the upload state is not defined these metrics can't be recorded. - await Context.eventsDatabase.initialize(); - // We only scan the pendings pings **after** dealing with the upload state. // If upload is disabled, pending pings files are deleted // so we need to know that state **before** scanning the pending pings diff --git a/glean/src/core/internal_pings.ts b/glean/src/core/internal_pings.ts index 9a1479e09..4530afee9 100644 --- a/glean/src/core/internal_pings.ts +++ b/glean/src/core/internal_pings.ts @@ -2,7 +2,7 @@ * 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 { DELETION_REQUEST_PING_NAME } from "./constants.js"; +import { DELETION_REQUEST_PING_NAME, EVENTS_PING_NAME } from "./constants.js"; import { InternalPingType as PingType} from "./pings/ping_type.js"; /** @@ -16,6 +16,8 @@ class CorePings { // that the user wishes to have their reported Telemetry data deleted. // As such it attempts to send itself at the moment the user opts out of data collection. readonly deletionRequest: PingType; + // The events ping's purpose is to transport event metric information. + readonly events: PingType; constructor() { this.deletionRequest = new PingType({ @@ -23,6 +25,13 @@ class CorePings { includeClientId: true, sendIfEmpty: true, }); + + this.events = new PingType({ + name: EVENTS_PING_NAME, + includeClientId: true, + sendIfEmpty: false, + reasonCodes: ["startup", "max_capacity"] + }); } } diff --git a/glean/src/core/metrics/database.ts b/glean/src/core/metrics/database.ts index e367c6659..69e0bca58 100644 --- a/glean/src/core/metrics/database.ts +++ b/glean/src/core/metrics/database.ts @@ -262,7 +262,7 @@ class MetricsDatabase { if (!validateMetricInternalRepresentation(metricType, metrics[metricIdentifier])) { log( LOG_TAG, - `Invalid value found in storage for metric "${metricIdentifier}". Deleting.`, + `Invalid value "${JSON.stringify(metrics[metricIdentifier])}" found in storage for metric "${metricIdentifier}". Deleting.`, LoggingLevel.Debug ); diff --git a/glean/src/core/metrics/events_database/index.ts b/glean/src/core/metrics/events_database/index.ts index 73d4041c6..cedc4db7a 100644 --- a/glean/src/core/metrics/events_database/index.ts +++ b/glean/src/core/metrics/events_database/index.ts @@ -15,6 +15,7 @@ import { generateReservedMetricIdentifiers } from "../database.js"; import type { ExtraValues , Event } from "./recorded_event.js"; import { RecordedEvent } from "./recorded_event.js"; import { + EVENTS_PING_NAME, GLEAN_EXECUTION_COUNTER_EXTRA_KEY, GLEAN_REFERENCE_TIME_EXTRA_KEY } from "../../constants.js"; @@ -47,12 +48,14 @@ function createDateObject(str?: ExtraValues): Date { * Creates an execution counter metric. * * @param sendInPings The list of pings this metric is sent in. + * Note: The 'events' ping should not contain glean.restarted events, + * so this ping will be filtered out from the 'sendInPings' array. * @returns A metric type instance. */ function getExecutionCounterMetric(sendInPings: string[]): CounterMetricType { return new CounterMetricType({ ...generateReservedMetricIdentifiers("execution_counter"), - sendInPings: sendInPings, + sendInPings: sendInPings.filter(name => name !== EVENTS_PING_NAME), lifetime: Lifetime.Ping, disabled: false }); @@ -62,13 +65,15 @@ function getExecutionCounterMetric(sendInPings: string[]): CounterMetricType { * Creates an `glean.restarted` event metric. * * @param sendInPings The list of pings this metric is sent in. + * Note: The 'events' ping should not contain glean.restarted events, + * so this ping will be filtered out from the 'sendInPings' array. * @returns A metric type instance. */ export function getGleanRestartedEventMetric(sendInPings: string[]): EventMetricType { return new EventMetricType({ category: "glean", name: "restarted", - sendInPings: sendInPings, + sendInPings: sendInPings.filter(name => name !== EVENTS_PING_NAME), lifetime: Lifetime.Ping, disabled: false }, [ GLEAN_REFERENCE_TIME_EXTRA_KEY ]); @@ -140,6 +145,14 @@ class EventsDatabase { } const storeNames = await this.getAvailableStoreNames(); + // Submit the events ping in case there are _any_ events unsubmitted from the previous run + if (storeNames.includes(EVENTS_PING_NAME)) { + const storedEvents = (await this.eventsStore.get([EVENTS_PING_NAME]) as JSONArray) ?? []; + if (storedEvents.length > 0) { + await Context.corePings.events.submitUndispatched("startup"); + } + } + // Increment the execution counter for known stores. // !IMPORTANT! This must happen before any event is recorded for this run. await getExecutionCounterMetric(storeNames).addUndispatched(1); @@ -177,12 +190,18 @@ class EventsDatabase { } value.addExtra(GLEAN_EXECUTION_COUNTER_EXTRA_KEY, currentExecutionCount); + let numEvents = 0; const transformFn = (v?: JSONValue): JSONArray => { - const existing: JSONArray = (v as JSONArray) ?? []; - existing.push(value.get()); - return existing; + const events: JSONArray = (v as JSONArray) ?? []; + events.push(value.get()); + numEvents = events.length; + return events; }; + await this.eventsStore.update([ping], transformFn); + if (ping === EVENTS_PING_NAME && numEvents >= Context.config.maxEvents) { + await Context.corePings.events.submitUndispatched("max_capacity"); + } } } diff --git a/glean/src/core/metrics/utils.ts b/glean/src/core/metrics/utils.ts index cb3be4cd1..c4f0c010c 100644 --- a/glean/src/core/metrics/utils.ts +++ b/glean/src/core/metrics/utils.ts @@ -11,8 +11,9 @@ import { isInteger, isString } from "../utils.js"; import { LabeledMetric } from "./types/labeled.js"; import { Context } from "../context.js"; import { ErrorType } from "../error/error_type.js"; +import log, { LoggingLevel } from "../log.js"; - +const LOG_TAG = "Glean.core.Metrics.utils"; /** * A metric factory function. @@ -31,7 +32,7 @@ export function createMetric(type: string, v: unknown): Metric( try { createMetric(type, v); return true; - } catch { + } catch(e) { + log(LOG_TAG, (e as Error).message, LoggingLevel.Error); return false; } } diff --git a/glean/src/core/testing/utils.ts b/glean/src/core/testing/utils.ts index df786ab2c..2cc7ec87f 100644 --- a/glean/src/core/testing/utils.ts +++ b/glean/src/core/testing/utils.ts @@ -7,7 +7,6 @@ import type { ConfigurationInterface } from "../config.js"; import { Context } from "../context.js"; import { testResetEvents } from "../events/utils.js"; import Glean from "../glean.js"; -import { CoreMetrics } from "../internal_metrics.js"; /** * Test-only API @@ -27,10 +26,6 @@ export async function testInitializeGlean( uploadEnabled = true, config?: ConfigurationInterface ): Promise { - // Core metrics need to be re-initialized so that - // the supportedMetrics map is re-created. - Glean.coreMetrics = new CoreMetrics(); - Context.testing = true; Glean.setPlatform(TestPlatform); diff --git a/glean/src/core/upload/manager.ts b/glean/src/core/upload/manager.ts index 06b5db4fd..a38b8b6ab 100644 --- a/glean/src/core/upload/manager.ts +++ b/glean/src/core/upload/manager.ts @@ -35,6 +35,10 @@ export interface QueuedPing extends PingInternalRepresentation { class PingUploadManager implements PingsDatabaseObserver { // A FIFO queue storing a `QueuedPing` for each pending ping. private queue: QueuedPing[]; + // A set of the identifiers of pings being processed + // i.e. pings that were removed from the queue by calling `getUploadTask`, + // but have not yet been deleted from the database / re-enqueued by calling `processPingUploadResponse`. + private processing: Set; // A worker that will take care of actually uploading pings. private worker: PingUploadWorker; @@ -50,6 +54,7 @@ class PingUploadManager implements PingsDatabaseObserver { private readonly rateLimiter = new RateLimiter(), ) { this.queue = []; + this.processing = new Set(); this.worker = new PingUploadWorker( // Initialize the ping upload worker with either the platform defaults or a custom @@ -65,11 +70,16 @@ class PingUploadManager implements PingsDatabaseObserver { /** * Enqueues a new ping at the end of the queue. * - * Will not enqueue if a ping with the same identifier is already enqueued. + * Will not enqueue if a ping with the same identifier is already enqueued + * or is currently being processed. * * @param ping The ping to enqueue. */ private enqueuePing(ping: QueuedPing): void { + if (this.processing.has(ping.identifier)) { + return; + } + for (const queuedPing of this.queue) { if (queuedPing.identifier === ping.identifier) { return; @@ -109,9 +119,11 @@ class PingUploadManager implements PingsDatabaseObserver { return uploadTaskFactory.wait(remainingTime || 0); } - // We are sure this array is not empty, so `shift` will never return an `undefined` value. + // We are sure this array is not empty, so this will never return an `undefined` value. // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const nextPing = this.queue.shift()!; + this.processing.add(nextPing.identifier); + return uploadTaskFactory.upload(nextPing); } @@ -175,8 +187,7 @@ class PingUploadManager implements PingsDatabaseObserver { */ async processPingUploadResponse(ping: QueuedPing, response: UploadResult): Promise { const { identifier } = ping; - // Remove ping from queue list. - this.queue.filter(p => p.identifier !== identifier); + this.processing.delete(identifier); const { status, result } = response; if (status && status >= 200 && status < 300) { diff --git a/glean/src/core/upload/task.ts b/glean/src/core/upload/task.ts index 044e0d488..edd500110 100644 --- a/glean/src/core/upload/task.ts +++ b/glean/src/core/upload/task.ts @@ -22,7 +22,7 @@ export const enum UploadTaskTypes { // An "uploading window" starts when a requester gets a new // `Upload_UploadTask` response and finishes when they // finally get a `Done_UploadTask` or `Wait_UploadTask` response. -type Done_UploadTask = { +export type Done_UploadTask = { type: UploadTaskTypes.Done }; @@ -31,13 +31,13 @@ type Done_UploadTask = { // // Contains the amount of time in milliseconds // the requester should wait before requesting a new task. -type Wait_UploadTask = { +export type Wait_UploadTask = { type: UploadTaskTypes.Wait, remainingTime: number, }; // A flag signaling there are no remaining upload tasks and worker is done (for now). -type Upload_UploadTask = { +export type Upload_UploadTask = { type: UploadTaskTypes.Upload, ping: QueuedPing, }; diff --git a/glean/src/pings.yaml b/glean/src/pings.yaml index 3155aeca9..7232cb301 100644 --- a/glean/src/pings.yaml +++ b/glean/src/pings.yaml @@ -23,3 +23,36 @@ deletion-request: - https://bugzilla.mozilla.org/show_bug.cgi?id=1587095#c6 notification_emails: - glean-team@mozilla.com + +events: + description: | + The events ping's purpose is to transport event metric information. + + This ping is sent at startup if there are any events + from the previous application run in storage. + It is also sent when maximum capacity is hit + i.e. when a given number of events is in storage. + Maximum capacity defaults to 500, + but may be changed through the `maxEvents` configuration option. + include_client_id: true + bugs: + - https://bugzilla.mozilla.org/1512938 + data_reviews: + - https://bugzilla.mozilla.org/show_bug.cgi?id=1512938#c3 + notification_emails: + - glean-team@mozilla.com + reasons: + startup: | + The ping was submitted at startup. + The events ping is always sent if there are any pending events at startup, + because event timestamps are not as reliable across application runs. + max_capacity: | + The maximum number of events was reached (default 500 events). + inactive: | + The ping was submitted when becoming inactive. In earlier versions, this + was called `background`. + + NOTE: It is not possible to find a definition of "inactivity" that spans + all of the platforms served by the Glean JavaScript SDK. + This reason is only listed here for documentation purposes. + It is never reported by the JavaScript SDK. diff --git a/glean/tests/unit/core/glean.spec.ts b/glean/tests/unit/core/glean.spec.ts index 8eaf2ca87..fcc37443d 100644 --- a/glean/tests/unit/core/glean.spec.ts +++ b/glean/tests/unit/core/glean.spec.ts @@ -49,14 +49,14 @@ describe("Glean", function() { it("client_id and first_run_date are regenerated if cleared", async function() { await Context.metricsDatabase.clearAll(); assert.strictEqual( - await Glean.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE), undefined); + await Context.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE), undefined); assert.strictEqual( - await Glean.coreMetrics["firstRunDate"].testGetValue(CLIENT_INFO_STORAGE), undefined); + await Context.coreMetrics["firstRunDate"].testGetValue(CLIENT_INFO_STORAGE), undefined); await testUninitializeGlean(); await testInitializeGlean(testAppId, true); - assert.ok(await Glean.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE)); - assert.ok(await Glean.coreMetrics["firstRunDate"].testGetValue(CLIENT_INFO_STORAGE)); + assert.ok(await Context.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE)); + assert.ok(await Context.coreMetrics["firstRunDate"].testGetValue(CLIENT_INFO_STORAGE)); }); it("basic metrics should be cleared when upload is disabled", async function() { @@ -96,36 +96,36 @@ describe("Glean", function() { }); it("first_run_date is managed correctly when toggling uploading", async function() { - const originalFirstRunDate = await Glean.coreMetrics["firstRunDate"] + const originalFirstRunDate = await Context.coreMetrics["firstRunDate"] .testGetValueAsString(CLIENT_INFO_STORAGE); Glean.setUploadEnabled(false); assert.strictEqual( - await Glean.coreMetrics["firstRunDate"].testGetValueAsString(CLIENT_INFO_STORAGE), + await Context.coreMetrics["firstRunDate"].testGetValueAsString(CLIENT_INFO_STORAGE), originalFirstRunDate ); Glean.setUploadEnabled(true); assert.strictEqual( - await Glean.coreMetrics["firstRunDate"].testGetValueAsString(CLIENT_INFO_STORAGE), + await Context.coreMetrics["firstRunDate"].testGetValueAsString(CLIENT_INFO_STORAGE), originalFirstRunDate ); }); it("client_id is managed correctly when toggling uploading", async function() { - const originalClientId = await Glean.coreMetrics["clientId"] + const originalClientId = await Context.coreMetrics["clientId"] .testGetValue(CLIENT_INFO_STORAGE); assert.ok(originalClientId); assert.ok(originalClientId !== KNOWN_CLIENT_ID); Glean.setUploadEnabled(false); assert.strictEqual( - await Glean.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE), + await Context.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE), KNOWN_CLIENT_ID ); Glean.setUploadEnabled(true); - const newClientId = await Glean.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE); + const newClientId = await Context.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE); assert.ok(newClientId !== originalClientId); assert.ok(newClientId !== KNOWN_CLIENT_ID); }); @@ -134,7 +134,7 @@ describe("Glean", function() { await testUninitializeGlean(); await testInitializeGlean(testAppId, false); assert.strictEqual( - await Glean.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE), + await Context.coreMetrics["clientId"].testGetValue(CLIENT_INFO_STORAGE), KNOWN_CLIENT_ID ); }); @@ -143,14 +143,14 @@ describe("Glean", function() { Glean.setUploadEnabled(false); await testUninitializeGlean(); await testInitializeGlean(testAppId, true); - const clientId = await Glean.coreMetrics["clientId"] + const clientId = await Context.coreMetrics["clientId"] .testGetValue(CLIENT_INFO_STORAGE); assert.ok(clientId); assert.ok(clientId !== KNOWN_CLIENT_ID); }); it("enabling when already enabled is a no-op", async function() { - const spy = sandbox.spy(Glean.coreMetrics, "initialize"); + const spy = sandbox.spy(Context.coreMetrics, "initialize"); Glean.setUploadEnabled(true); // Wait for `setUploadEnabled` to be executed. await Context.dispatcher.testBlockOnQueue(); @@ -488,8 +488,8 @@ describe("Glean", function() { ); await Context.dispatcher.testBlockOnQueue(); - assert.strictEqual(await Glean.coreMetrics.appBuild.testGetValue(), testBuild); - assert.strictEqual(await Glean.coreMetrics.appDisplayVersion.testGetValue(), testDisplayVersion); + assert.strictEqual(await Context.coreMetrics.appBuild.testGetValue(), testBuild); + assert.strictEqual(await Context.coreMetrics.appDisplayVersion.testGetValue(), testDisplayVersion); }); // Verification test, does not test anything the Dispatcher suite doesn't cover, diff --git a/glean/tests/unit/core/metrics/event.spec.ts b/glean/tests/unit/core/metrics/event.spec.ts index d79576866..265b3e6ea 100644 --- a/glean/tests/unit/core/metrics/event.spec.ts +++ b/glean/tests/unit/core/metrics/event.spec.ts @@ -117,9 +117,6 @@ describe("EventMetric", function() { assert.strictEqual(snapshot?.length, 1); }); - it.skip("bug 1690253: flush queued events on startup"); - it.skip("bug 1690253: flush queued events on startup and correctly handle pre init events"); - it("long extra values record an error", async function () { const metric = new EventMetricType({ category: "telemetry", @@ -146,9 +143,6 @@ describe("EventMetric", function() { assert.strictEqual(await metric.testGetNumRecordedErrors(ErrorType.InvalidValue), 1); }); - it.skip("bug 1690301: overdue events are submitted in registered custom pings"); - it.skip("bug 1690301: overdue events are discarded if ping is not registered"); - it("records properly without optional arguments", async function () { const pings = ["store1", "store2"]; diff --git a/glean/tests/unit/core/metrics/events_database.spec.ts b/glean/tests/unit/core/metrics/events_database.spec.ts index 1b62b4a87..2ebabfdc6 100644 --- a/glean/tests/unit/core/metrics/events_database.spec.ts +++ b/glean/tests/unit/core/metrics/events_database.spec.ts @@ -15,10 +15,14 @@ import { generateReservedMetricIdentifiers } from "../../../../src/core/metrics/ import { InternalPingType as PingType} from "../../../../src/core/pings/ping_type"; import { Context } from "../../../../src/core/context"; import { RecordedEvent } from "../../../../src/core/metrics/events_database/recorded_event"; -import { GLEAN_EXECUTION_COUNTER_EXTRA_KEY } from "../../../../src/core/constants"; +import { EVENTS_PING_NAME, GLEAN_EXECUTION_COUNTER_EXTRA_KEY } from "../../../../src/core/constants"; import { collectPing } from "../../../../src/core/pings/maker"; import { ErrorType } from "../../../../src/core/error/error_type"; import { testResetGlean } from "../../../../src/core/testing"; +import type { Event } from "../../../../src/core/metrics/events_database/recorded_event"; +import { testInitializeGlean, testUninitializeGlean } from "../../../../src/core/testing/utils"; +import { WaitableUploader } from "../../../utils"; +import type { PingPayload } from "../../../../src/core/pings/ping_payload"; const sandbox = sinon.createSandbox(); const now = new Date(); @@ -728,4 +732,109 @@ describe("EventsDatabase", function() { assert.strictEqual(secondEvent.timestamp, timestamp2); } }); + + it("send the 'events' ping on initialize when there are remaining events from previous run", async function () { + // Restore timer APIs for WaitableUploader to work + clock.restore(); + + const event = new EventMetricType({ + category: "test", + name: "event", + sendInPings: [EVENTS_PING_NAME], + lifetime: Lifetime.Ping, + disabled: false + }); + for (let i = 0; i < 10; i++) { + await event.recordUndispatched(); + } + + const httpClient = new WaitableUploader(); + const waitForEventsPing = httpClient.waitForPingSubmission(EVENTS_PING_NAME); + // Re-initialize Glean without clearing stores, + // this will trigger initialization of the events database as well + await testResetGlean(testAppId, true, { httpClient }, false); + + const { ping_info: { reason } } = (await waitForEventsPing) as PingPayload; + assert.strictEqual(reason, "startup"); + }); + + it("send the 'events' ping on initialize and correctly handle pre init events", async function () { + // Restore timer APIs for WaitableUploader to work + clock.restore(); + + const previousRunEvent = new EventMetricType({ + category: "test", + name: "previousRun", + sendInPings: [EVENTS_PING_NAME], + lifetime: Lifetime.Ping, + disabled: false + }); + for (let i = 0; i < 5; i++) { + previousRunEvent.record(); + } + + // Uninitialize Glean, but do not clear stores + await testUninitializeGlean(false); + + // Record a bunch of events while Glean is uninitialized + const preInitEvent = new EventMetricType({ + category: "test", + name: "preInit", + sendInPings: [EVENTS_PING_NAME], + lifetime: Lifetime.Ping, + disabled: false + }); + for (let i = 0; i < 5; i++) { + preInitEvent.record(); + } + + const httpClient = new WaitableUploader(); + const waitForEventsPings = httpClient.waitForBatchPingSubmission(EVENTS_PING_NAME, 2); + // Initialization should trigger a startup ping + await testInitializeGlean(testAppId, true, { httpClient }); + // Send another 'events' ping after init, it should contain the preInit events + await Context.corePings.events.submitUndispatched(); + + // First ping is the startup ping, + // second ping is the events ping submitted above. + const [ + { ping_info: { reason }, events: fromPreviousRun }, + { events: fromPreInit } + ] = (await waitForEventsPings) as PingPayload[]; + + assert.strictEqual(reason, "startup"); + assert.strictEqual(fromPreviousRun?.length, 5); + assert.ok(fromPreviousRun.every(event => (event as Event).name === "previousRun")); + + assert.strictEqual(fromPreInit?.length, 5); + assert.ok(fromPreInit?.every(event => (event as Event).name === "preInit")); + }); + + it("send the 'events' ping when max capacity is hit", async function () { + // Restore timer APIs for WaitableUploader to work + clock.restore(); + + const httpClient = new WaitableUploader(); + const waitForEventsPing = httpClient.waitForPingSubmission(EVENTS_PING_NAME); + // Re-initialize Glean with a known max capacity for the events ping + await testResetGlean(testAppId, true, { httpClient, maxEvents: 10 }); + + const event = new EventMetricType({ + category: "test", + name: "event", + sendInPings: [EVENTS_PING_NAME], + lifetime: Lifetime.Ping, + disabled: false + }); + for (let i = 0; i < 15; i++) { + await event.recordUndispatched(); + } + + const { ping_info: { reason }, events } = (await waitForEventsPing) as PingPayload; + assert.strictEqual(reason, "max_capacity"); + assert.strictEqual(events?.length, 10); + + const leftoverEvents = await Context.eventsDatabase.getPingEvents(EVENTS_PING_NAME, false); + assert.strictEqual(leftoverEvents?.length, 5); + }); }); diff --git a/glean/tests/unit/core/upload/manager.spec.ts b/glean/tests/unit/core/upload/manager.spec.ts index 8b5248d60..d2e5a5e35 100644 --- a/glean/tests/unit/core/upload/manager.spec.ts +++ b/glean/tests/unit/core/upload/manager.spec.ts @@ -9,17 +9,28 @@ import { v4 as UUIDv4 } from "uuid"; import { Configuration } from "../../../../src/core/config"; import { Context } from "../../../../src/core/context"; import PingUploadManager from "../../../../src/core/upload/manager"; -import { UploadResultStatus } from "../../../../src/core/upload/uploader"; +import { UploadResult, UploadResultStatus } from "../../../../src/core/upload/uploader"; import { CounterUploader, WaitableUploader } from "../../../utils"; import { DELETION_REQUEST_PING_NAME } from "../../../../src/core/constants"; import PingsDatabase from "../../../../src/core/pings/database"; import { makePath } from "../../../../src/core/pings/maker"; import Policy from "../../../../src/core/upload/policy"; +import type { Upload_UploadTask } from "../../../../src/core/upload/task"; import { UploadTaskTypes } from "../../../../src/core/upload/task"; import { MAX_PINGS_PER_INTERVAL } from "../../../../src/core/upload/rate_limiter"; import { testResetGlean } from "../../../../src/core/testing"; const sandbox = sinon.createSandbox(); +const MOCK_PAYLOAD = { + ping_info: { + seq: 1, + start_time: "2020-01-11+01:00", + end_time: "2020-01-12+01:00", + }, + client_info: { + telemetry_sdk_build: "32.0.0" + } +}; describe("PingUploadManager", function() { const testAppId = `gleanjs.test.${this.title}`; @@ -36,24 +47,13 @@ describe("PingUploadManager", function() { numPings: number, pingName = "ping" ): Promise { - const payload = { - ping_info: { - seq: 1, - start_time: "2020-01-11+01:00", - end_time: "2020-01-12+01:00", - }, - client_info: { - telemetry_sdk_build: "32.0.0" - } - }; - const identifiers = Array.from({ length: numPings }, () => UUIDv4()); for (const identifier of identifiers) { const path = makePath( identifier, { name: pingName, includeClientId: true, sendIfEmpty: true } ); - await pingsDatabase.recordPing(path, identifier, payload); + await pingsDatabase.recordPing(path, identifier, MOCK_PAYLOAD); } return identifiers; @@ -331,4 +331,39 @@ describe("PingUploadManager", function() { assert.strictEqual(uploader.getUploadTask().type, UploadTaskTypes.Done); }); + + it("pings cannot be re-enqueued while they are being processed", async function () { + const uploader = new PingUploadManager(new Configuration(), pingsDatabase); + // Disable the PingUploadWorker so it does not interfere with these tests + uploader["worker"]["workInternal"] = () => Promise.resolve(); + + // Enqueue a ping and start processing it + const [ identifier ] = await fillUpPingsDatabase(1); + const task = uploader.getUploadTask() as Upload_UploadTask; + assert.strictEqual(task.type, UploadTaskTypes.Upload); + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + assert.strictEqual(task.ping.identifier, identifier); + + // Attempt to re-enqueue the same ping by scanning the pings database, + // since the ping has not been deleted yet this will propmt the uploader + // to re-enqueue the ping we just created. + await pingsDatabase.scanPendingPings(); + + // No new pings should have been enqueued so the upload task is Done. + assert.strictEqual(uploader.getUploadTask().type, UploadTaskTypes.Done); + + // Fake process the upload response, only the identifier really matters here + // we expect the ping to be deleted from the queue and the database now. + await uploader.processPingUploadResponse( + { identifier, payload: MOCK_PAYLOAD, collectionDate: "", path: "" }, + new UploadResult(UploadResultStatus.Success, 200) + ); + + // Check that the ping was not re-enqueued + assert.strictEqual(uploader.getUploadTask().type, UploadTaskTypes.Done); + + // Check the ping was deleted from the database + await pingsDatabase.scanPendingPings(); + assert.strictEqual(uploader.getUploadTask().type, UploadTaskTypes.Done); + }); });