Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1694770 - Implement the timespan metric type #253

Merged
merged 6 commits into from
Apr 28, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
[Full changelog](https://github.com/mozilla/glean.js/compare/v0.10.2...main)

* [#202](https://github.com/mozilla/glean.js/pull/202): Add a testing API for the ping type.
* [#253](https://github.com/mozilla/glean.js/pull/253): Implement the timespan metric type.

# v0.10.2 (2021-04-26)

Expand Down
40 changes: 0 additions & 40 deletions glean/src/core/error_recording.ts

This file was deleted.

19 changes: 2 additions & 17 deletions glean/src/core/metrics/types/event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { CommonMetricData } from "../index.js";
import { MetricType } from "../index.js";
import type { ExtraMap} from "../events_database.js";
import { RecordedEvent } from "../events_database.js";
import { isUndefined } from "../../utils.js";
import { getMonotonicNow } from "../../utils.js";
import { Context } from "../../context.js";

const MAX_LENGTH_EXTRA_KEY_VALUE = 100;
Expand All @@ -22,21 +22,6 @@ class EventMetricType extends MetricType {
this.allowedExtraKeys = allowedExtraKeys;
}

/**
* An helper function to aid mocking the time in tests.
*
* This is only meant to be overridden in tests.
*
* @returns the number of milliseconds since the time origin.
*/
protected getMonotonicNow(): number {
// Sadly, `performance.now` is not available outside of browsers, which
// means we should get creative to find a proper clock. Fall back to `Date.now`
// for now, until bug 1690528 is fixed.
const now = isUndefined(performance) ? Date.now() : performance.now();
return Math.round(now / 1000);
}

/**
* Record an event by using the information
* provided by the instance of this class.
Expand All @@ -52,7 +37,7 @@ class EventMetricType extends MetricType {
return;
}

const timestamp = this.getMonotonicNow();
const timestamp = getMonotonicNow();

// Truncate the extra keys, if needed.
let truncatedExtra: ExtraMap | undefined = undefined;
Expand Down
2 changes: 1 addition & 1 deletion glean/src/core/metrics/types/labeled.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { Metric } from "../metric.js";
* whatever is stored internally, without performing any specific
* validation.
*/
export class LabeledMetric extends Metric<JSONValue, JSONValue> {
export class LabeledMetric extends Metric<JSONValue, JSONValue> {
constructor(v: unknown) {
super(v);
}
Expand Down
203 changes: 203 additions & 0 deletions glean/src/core/metrics/types/timespan.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
/* 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 type { CommonMetricData} from "../index.js";
import type { JSONValue } from "../../utils.js";
import TimeUnit from "../time_unit.js";
import { MetricType } from "../index.js";
import { isString, isObject, isNumber, isUndefined, getMonotonicNow } from "../../utils.js";
import { Metric } from "../metric.js";
import { Context } from "../../context.js";

export type TimespanInternalRepresentation = {
// The time unit of the metric type at the time of recording.
timeUnit: TimeUnit,
// The timespan in milliseconds.
timespan: number,
};
export class TimespanMetric extends Metric<TimespanInternalRepresentation, number> {
constructor(v: unknown) {
super(v);
}

validate(v: unknown): v is TimespanInternalRepresentation {
if (!isObject(v) || Object.keys(v).length !== 2) {
return false;
}

const timeUnitVerification = "timeUnit" in v && isString(v.timeUnit) && Object.values(TimeUnit).includes(v.timeUnit as TimeUnit);
const timespanVerification = "timespan" in v && isNumber(v.timespan) && v.timespan >= 0;
if (!timeUnitVerification || !timespanVerification) {
return false;
}

return true;
}

payload(): number {
switch(this._inner.timeUnit) {
case TimeUnit.Nanosecond:
return this._inner.timespan * 10**6;
case TimeUnit.Microsecond:
return this._inner.timespan * 10**3;
case TimeUnit.Millisecond:
return this._inner.timespan;
case TimeUnit.Second:
return Math.round(this._inner.timespan / 1000);
case TimeUnit.Minute:
return Math.round(this._inner.timespan / 1000 / 60);
case TimeUnit.Hour:
return Math.round(this._inner.timespan / 1000 / 60 / 60);
case TimeUnit.Day:
return Math.round(this._inner.timespan / 1000 / 60 / 60 / 24);
}
}
}

/**
* A timespan metric.
*
* Timespans are used to make a measurement of how much time is spent in a particular task.
*/
class TimespanMetricType extends MetricType {
private timeUnit: TimeUnit;
startTime?: number;

constructor(meta: CommonMetricData, timeUnit: string) {
super("timespan", meta);
this.timeUnit = timeUnit as TimeUnit;
}

/**
* Starts tracking time for the provided metric.
*
* This records an error if it's already tracking time (i.e. start was
* already called with no corresponding `stop()`. In which case the original
* start time will be preserved.
*/
start(): void {
// Get the start time outside of the dispatched task so that
// it is the time this function is called and not the time the task is executed.
const startTime = getMonotonicNow();

Context.dispatcher.launch(async () => {
if (!this.shouldRecord(Context.uploadEnabled)) {
return;
}

if (!isUndefined(this.startTime)) {
// TODO: record error once Bug 1682574 is resolved.
console.error("Timespan already started.");
return;
}

this.startTime = startTime;

return Promise.resolve();
});
}

/**
* Stops tracking time for the provided metric. Sets the metric to the elapsed time.
*
* This will record an error if no `start()` was called.
*/
stop(): void {
// Get the stop time outside of the dispatched task so that
// it is the time this function is called and not the time the task is executed.
const stopTime = getMonotonicNow();

Context.dispatcher.launch(async () => {
if (!this.shouldRecord(Context.uploadEnabled)) {
// Reset timer when disabled, so that we don't record timespans across
// disabled/enabled toggling.
this.startTime = undefined;
return;
}

if (isUndefined(this.startTime)) {
// TODO: record error once Bug 1682574 is resolved.
console.error("Timespan not running.");
return;
}

const elapsed = stopTime - this.startTime;
this.startTime = undefined;

if (elapsed < 0) {
// TODO: record error once Bug 1682574 is resolved.
console.error("Timespan was negative.");
return;
}

let reportValueExists = false;
const transformFn = ((elapsed) => {
return (old?: JSONValue): TimespanMetric => {
let metric: TimespanMetric;
try {
metric = new TimespanMetric(old);
// If creating the metric didn't error,
// there is a valid timespan already recorded for this metric.
reportValueExists = true;
} catch {
metric = new TimespanMetric({
timespan: elapsed,
timeUnit: this.timeUnit,
});
}

return metric;
};
})(elapsed);

await Context.metricsDatabase.transform(this, transformFn);

if (reportValueExists) {
// TODO: record error once Bug 1682574 is resolved.
console.error("Timespan value already recorded. New value discarded.");
}
});
}

/**
* Aborts a previous `start()` call.
*
* No error is recorded if no `start()` was called.
*/
cancel(): void {
Context.dispatcher.launch(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wasn't this failing our tests? did you end up finding out?

Copy link
Contributor Author

@brizental brizental Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dispatched or not, start and cancel when called in sequence seem to run one after the other. Probably because neither have any actual await inside. I tried to make the tests fail, but they never did.

Only when I launch some setTimeout's in between calling them do they fail, but I did not commit such a test because I thought it would be confusing.

this.startTime = undefined;
return Promise.resolve();
});
}

/**
* **Test-only API.**
*
* Gets the currently stored value as a number.
*
* This doesn't clear the stored value.
*
* TODO: Only allow this function to be called on test mode (depends on Bug 1682771).
*
* @param ping the ping from which we want to retrieve this metrics value from.
* Defaults to the first value in `sendInPings`.
*
* @returns The value found in storage or `undefined` if nothing was found.
*/
async testGetValue(ping: string = this.sendInPings[0]): Promise<number | undefined> {
let value: TimespanInternalRepresentation | undefined;
await Context.dispatcher.testLaunch(async () => {
value = await Context.metricsDatabase.getMetric<TimespanInternalRepresentation>(ping, this);
});

if (value) {
// `payload` will truncate to the defined time_unit at the time of recording.
return (new TimespanMetric(value)).payload();
brizental marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

export default TimespanMetricType;

2 changes: 2 additions & 0 deletions glean/src/core/metrics/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { BooleanMetric } from "./types/boolean.js";
import { CounterMetric } from "./types/counter.js";
import { DatetimeMetric } from "./types/datetime.js";
import { StringMetric } from "./types/string.js";
import { TimespanMetric } from "./types/timespan.js";
import { UUIDMetric } from "./types/uuid.js";

/**
Expand All @@ -26,6 +27,7 @@ const METRIC_MAP: {
"labeled_counter": LabeledMetric,
"labeled_string": LabeledMetric,
"string": StringMetric,
"timespan": TimespanMetric,
"uuid": UUIDMetric,
});

Expand Down
14 changes: 14 additions & 0 deletions glean/src/core/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,3 +159,17 @@ export function generateUUIDv4(): string {
});
}
}

/**
* A helper function to aid mocking the time in tests.
*
* This is only meant to be overridden in tests.
*
* @returns The number of milliseconds since the time origin.
*/
export function getMonotonicNow(): number {
// Sadly, `performance.now` is not available on Qt, which
// means we should get creative to find a proper clock for that platform.
// Fall back to `Date.now` for now, until bug 1690528 is fixed.
return typeof performance === "undefined" ? Date.now() : performance.now();
brizental marked this conversation as resolved.
Show resolved Hide resolved
}
Loading