Skip to content

Commit a9584b9

Browse files
mtsgrdmtsgrd
committed
Refactor metrics reporter
- ignore bad input - improved run schedule
1 parent 7cf283f commit a9584b9

File tree

6 files changed

+233
-39
lines changed

6 files changed

+233
-39
lines changed

‎apps/desktop/src/lib/forge/github/githubListingService.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ describe.concurrent('GitHubListingService', () => {
4848
expect(prs?.length).toEqual(1);
4949
expect(prs?.[0]?.title).toEqual(title);
5050

51-
const metrics = projectMetrics.getMetrics();
51+
const metrics = projectMetrics.getReport();
5252
expect(metrics['pr_count']?.value).toEqual(1);
5353
expect(metrics['pr_count']?.maxValue).toEqual(1);
5454
expect(metrics['pr_count']?.minValue).toEqual(1);

‎apps/desktop/src/lib/metrics/MetricsReporter.svelte‎

Lines changed: 50 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,51 +1,74 @@
1+
<script lang="ts" module>
2+
export const HOUR_MS = 60 * 60 * 1000;
3+
export const INTERVAL_MS = 24 * HOUR_MS;
4+
</script>
5+
16
<script lang="ts">
27
import { ProjectMetrics, type ProjectMetricsReport } from './projectMetrics';
8+
import { PostHogWrapper } from '$lib/analytics/posthog';
9+
import { getContext } from '@gitbutler/shared/context';
310
import { persisted } from '@gitbutler/shared/persisted';
4-
import posthog from 'posthog-js';
511
import { onMount } from 'svelte';
612
713
const { projectMetrics }: { projectMetrics: ProjectMetrics } = $props();
14+
815
const projectId = projectMetrics.projectId;
16+
const posthog = getContext(PostHogWrapper);
917
1018
// Storing the last known values so we don't report same metrics twice
1119
const lastReport = persisted<ProjectMetricsReport>({}, `projectMetrics-${projectId}`);
12-
const hourMs = 60 * 60 * 1000;
20+
const lastReportMs = persisted<number | undefined>(undefined, `lastMetricsTs-${projectId}`);
1321
14-
let lastCapture: { [key: string]: number | undefined } = {};
22+
// Any interval or timeout must be cleared on unmount.
1523
let intervalId: any;
24+
let timeoutId: any;
1625
17-
function sample() {
18-
const metrics = projectMetrics.getMetrics();
19-
if (!metrics) return;
26+
function reportMetrics() {
27+
// So we know if we should run on next onMount.
28+
lastReportMs.set(Date.now());
29+
30+
const report = projectMetrics.getReport();
31+
if (!report) return;
2032
2133
// Capture only individual changes.
22-
for (let [name, metric] of Object.entries(metrics)) {
23-
const lastCaptureMs = lastCapture[name];
24-
if (
25-
// If no previously recorded value.
26-
!$lastReport[name] ||
27-
// Or the value has changed.
28-
$lastReport[name]?.value !== metric?.value ||
29-
// Or 24h have passed since metric was last caprured
30-
(lastCaptureMs && lastCaptureMs - Date.now() > 24 * hourMs)
31-
) {
32-
posthog.capture(`metrics:${name}`, {
33-
project_id: projectId,
34-
...metric
35-
});
36-
lastCapture[name] = Date.now();
37-
projectMetrics.resetMetric(name);
38-
}
34+
for (let [name, metric] of Object.entries(report)) {
35+
posthog.capture(`metrics:${name}`, {
36+
project_id: projectId,
37+
...metric
38+
});
39+
projectMetrics.resetMetric(name);
3940
}
40-
lastReport.set(metrics);
41+
lastReport.set(projectMetrics.getReport());
4142
}
4243
43-
onMount(() => {
44+
function startInterval() {
45+
reportMetrics();
4446
intervalId = setInterval(() => {
45-
sample();
46-
}, 4 * hourMs);
47+
reportMetrics();
48+
}, INTERVAL_MS);
49+
}
50+
51+
function scheduleFirstReport() {
52+
const now = Date.now();
53+
const lastMs = $lastReportMs;
54+
55+
if (!lastMs || now - lastMs > INTERVAL_MS) {
56+
// It's been a while, start immediately.
57+
startInterval();
58+
} else {
59+
// Wait until full interval has passed then start.
60+
const duration = lastMs - now + INTERVAL_MS;
61+
timeoutId = setTimeout(() => {
62+
startInterval();
63+
}, duration);
64+
}
65+
}
66+
67+
onMount(() => {
68+
scheduleFirstReport();
4769
return () => {
4870
if (intervalId) clearInterval(intervalId);
71+
if (timeoutId) clearTimeout(timeoutId);
4972
};
5073
});
5174
</script>

‎apps/desktop/src/lib/metrics/MetricsReporter.test.ts‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
import MetricsReporter, { HOUR_MS, INTERVAL_MS } from './MetricsReporter.svelte';
2+
import { ProjectMetrics } from './projectMetrics';
3+
import { PostHogWrapper } from '$lib/analytics/posthog';
4+
import { render } from '@testing-library/svelte';
5+
import { assert, test, describe, vi, beforeEach, afterEach } from 'vitest';
6+
7+
const PROJECT_ID = 'test-project';
8+
const METRIC_NAME = 'test-metric';
9+
10+
describe('MetricsReporter', () => {
11+
let projectMetrics: ProjectMetrics;
12+
let context: Map<any, any>;
13+
let posthog: PostHogWrapper;
14+
15+
beforeEach(() => {
16+
vi.useFakeTimers();
17+
projectMetrics = new ProjectMetrics(PROJECT_ID);
18+
posthog = new PostHogWrapper();
19+
context = new Map([[PostHogWrapper, posthog]]);
20+
});
21+
22+
afterEach(() => {
23+
vi.restoreAllMocks();
24+
vi.clearAllTimers();
25+
});
26+
27+
test('should report on interval', async () => {
28+
const posthogMock = vi.spyOn(posthog, 'capture').mock;
29+
30+
projectMetrics.setMetric(METRIC_NAME, 1);
31+
render(MetricsReporter, { props: { projectMetrics }, context });
32+
33+
// Ensure first-run capture works.
34+
assert.equal(posthogMock.calls.length, 1);
35+
assert.equal(posthogMock.lastCall?.[0], 'metrics:' + METRIC_NAME);
36+
assert.deepEqual(posthogMock.lastCall?.[1], {
37+
project_id: PROJECT_ID,
38+
value: 1,
39+
minValue: 1,
40+
maxValue: 1
41+
});
42+
43+
// Metrics are reset after they have been reported, so we should expect
44+
// that previous value does not influence next max/min.
45+
projectMetrics.setMetric(METRIC_NAME, -1);
46+
projectMetrics.setMetric(METRIC_NAME, 1);
47+
projectMetrics.setMetric(METRIC_NAME, 0);
48+
49+
// Stop just one millisecond short of the reporting interval, and verify
50+
// it has not run again.
51+
await vi.advanceTimersByTimeAsync(INTERVAL_MS - 1);
52+
assert.equal(posthogMock.calls.length, 1);
53+
54+
// Advance one millisecond and verify newly reported metrics.
55+
await vi.advanceTimersByTimeAsync(1);
56+
assert.equal(posthogMock.calls.length, 2);
57+
assert.deepEqual(posthogMock.lastCall?.[1], {
58+
project_id: PROJECT_ID,
59+
value: 0,
60+
minValue: -1,
61+
maxValue: 1
62+
});
63+
});
64+
65+
test('run based on last timestamp', async () => {
66+
const captureMock = vi.spyOn(posthog, 'capture').mock;
67+
68+
// System time set to 0 plus a full report interval.
69+
vi.setSystemTime(INTERVAL_MS);
70+
// Simulate last report to have been sent at hour 1.
71+
localStorage.setItem('lastMetricsTs-fake-id', HOUR_MS.toString());
72+
73+
projectMetrics.setMetric(METRIC_NAME, 1);
74+
render(MetricsReporter, { props: { projectMetrics }, context });
75+
76+
// Verify it did not fire immediately.
77+
assert.equal(captureMock.calls.length, 0);
78+
79+
// Advance one hour so that a full interval has elapsed.
80+
await vi.advanceTimersByTimeAsync(HOUR_MS);
81+
assert.equal(captureMock.calls.length, 1);
82+
83+
// Set new metric value since last one should have been cleared.
84+
projectMetrics.setMetric(METRIC_NAME, 1);
85+
86+
// Advance by full interval and ensure it fires again.
87+
await vi.advanceTimersByTimeAsync(INTERVAL_MS);
88+
assert.equal(captureMock.calls.length, 2);
89+
});
90+
});

‎apps/desktop/src/lib/metrics/projectMetrics.test.ts‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
import { ProjectMetrics } from './projectMetrics';
2+
import { assert, test, describe } from 'vitest';
3+
4+
describe.concurrent('ProjectMetrics', () => {
5+
test('set max and min correctly', async () => {
6+
const metrics = new ProjectMetrics('fake-id');
7+
const metricLabel = 'test_metric';
8+
9+
metrics.setMetric(metricLabel, 0);
10+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 0, minValue: 0, maxValue: 0 });
11+
12+
metrics.setMetric(metricLabel, 1);
13+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 1, minValue: 0, maxValue: 1 });
14+
15+
metrics.setMetric(metricLabel, -1);
16+
assert.deepEqual(metrics.getReport()[metricLabel], { value: -1, minValue: -1, maxValue: 1 });
17+
18+
metrics.setMetric(metricLabel, 2);
19+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 2, minValue: -1, maxValue: 2 });
20+
21+
metrics.setMetric(metricLabel, -2);
22+
assert.deepEqual(metrics.getReport()[metricLabel], { value: -2, minValue: -2, maxValue: 2 });
23+
});
24+
25+
test('handle malformed input', async () => {
26+
const metrics = new ProjectMetrics('fake-id');
27+
const metricLabel = 'test_metric';
28+
metrics.setMetric(metricLabel, 1);
29+
metrics.setMetric(metricLabel, 2);
30+
31+
// Expected initial condition.
32+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 2, minValue: 1, maxValue: 2 });
33+
34+
// @ts-expect-error since we are intentionally violating the type.
35+
metrics.setMetric(metricLabel, null);
36+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 2, minValue: 1, maxValue: 2 });
37+
38+
// @ts-expect-error since we are intentionally violating the type.
39+
metrics.setMetric(metricLabel, undefined);
40+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 2, minValue: 1, maxValue: 2 });
41+
42+
metrics.setMetric(metricLabel, Infinity);
43+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 2, minValue: 1, maxValue: 2 });
44+
45+
metrics.setMetric(metricLabel, -Infinity);
46+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 2, minValue: 1, maxValue: 2 });
47+
48+
metrics.setMetric(metricLabel, NaN);
49+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 2, minValue: 1, maxValue: 2 });
50+
51+
// Set a new valid value and observe the change.
52+
metrics.setMetric(metricLabel, 3);
53+
assert.deepEqual(metrics.getReport()[metricLabel], { value: 3, minValue: 1, maxValue: 3 });
54+
});
55+
});

‎apps/desktop/src/lib/metrics/projectMetrics.ts‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,25 +8,43 @@ type ProjectMetric = {
88
maxValue: number;
99
};
1010

11+
/**
12+
* Tracks arbitrary metrics and keeps track of min/max values. Please note that
13+
* reporting these numbers to the back end is delegated to the MetricsReporter
14+
* component.
15+
*/
1116
export class ProjectMetrics {
1217
private metrics: { [key: string]: ProjectMetric | undefined } = {};
1318

1419
constructor(readonly projectId?: string) {}
1520

1621
setMetric(key: string, value: number) {
17-
const oldvalue = this.metrics[key];
18-
19-
const maxValue = Math.max(value, oldvalue?.maxValue || value);
20-
const minValue = Math.min(value, oldvalue?.minValue || value);
21-
this.metrics[key] = {
22-
value,
23-
maxValue,
24-
minValue
25-
};
22+
// Guard against upstream bugs feeding bad values.
23+
if (typeof value !== 'number' || !Number.isFinite(value) || Number.isNaN(value)) {
24+
console.warn(`Ignoring ${key} metric, bad value: ${value}`);
25+
return;
26+
}
27+
const oldEntry = this.metrics[key];
28+
if (oldEntry) {
29+
const { maxValue, minValue } = oldEntry;
30+
this.metrics[key] = {
31+
value,
32+
maxValue: Math.max(value, maxValue),
33+
minValue: Math.min(value, minValue)
34+
};
35+
} else {
36+
this.metrics[key] = {
37+
value,
38+
maxValue: value,
39+
minValue: value
40+
};
41+
}
2642
}
2743

28-
getMetrics(): ProjectMetricsReport {
29-
return this.metrics;
44+
getReport(): ProjectMetricsReport {
45+
// Return a copy since we keep mutating the metrics object,
46+
// and a report is specific to a point in time.
47+
return { ...this.metrics };
3048
}
3149

3250
resetMetric(key: string) {

‎apps/desktop/src/routes/[projectId]/board/+page.svelte‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,17 @@
44
import Board from '$lib/components/Board.svelte';
55
import { projectHttpsWarningBannerDismissed } from '$lib/config/config';
66
import { getForge } from '$lib/forge/interface/forge';
7+
import MetricsReporter from '$lib/metrics/MetricsReporter.svelte';
78
import { ModeService } from '$lib/modes/service';
89
import { showToast } from '$lib/notifications/toasts';
910
import Scrollbar from '$lib/scroll/Scrollbar.svelte';
1011
import { getContext } from '@gitbutler/shared/context';
12+
import type { PageData } from './$types';
1113
import { goto } from '$app/navigation';
1214
15+
const { data }: { data: PageData } = $props();
16+
const { projectMetrics } = $derived(data);
17+
1318
const project = getContext(Project);
1419
const forge = getForge();
1520
const baseBranchService = getContext(BaseBranchService);
@@ -61,6 +66,9 @@
6166
</div>
6267
</div>
6368

69+
<!-- Mounting metrics reporter in the board ensures dependent services are subscribed to. -->
70+
<MetricsReporter {projectMetrics} />
71+
6472
<style lang="postcss">
6573
/* BOARD */
6674
.board-wrapper {

0 commit comments

Comments
 (0)