Split metrics from `device` resource #1371

fisehara · 2023-07-04T09:05:46Z

Remodel device resource and split metrics into device metrics record

This reduces the load on the device resource when updating device metrics periodically. device metrics are far less frequently needed than the device resource. Thus writing to the heavily used device resource should be offloaded to the device metrics record resource.

Change-type: major

src/features/device-state/routes/state-patch-v3.ts

Page- · 2023-07-04T14:59:28Z

src/features/device-state/routes/state-patch-v2.ts

+						console.log(
+							`patch metricsBody:${JSON.stringify(metricsBody, null, 2)}`,
+						);


This will need removing

Page- · 2023-07-04T15:08:55Z

src/features/device-state/routes/state-patch-v3.ts

+						const latestDeviceMetricsRecord = await resinApiTx.get({
+							resource: 'device_metrics_record',
+							id: { is_reported_by__device: device.id },
+						});


This can be merged into the fetchData request rather than adding an extra one

Thanks, great hint also explicitly loading with readOnly transcation.

I've added it to the device request by expanding the reports__device_metrics_record

Page- · 2023-07-04T15:10:06Z

src/migrations/00084-split-device-metrics-from-device-schema.sql

+CREATE INDEX IF NOT EXISTS "device_metrics_record_by_device_idx"
+ON "device metrics record" ("is reported by-device");


This needs to be included in balena-init.sql

@Page- @fisehara Afaict we shouldn't need to add a manual index (neither here nor in the balena-init.sql), since the "is reported by-device" INTEGER NOT NULL UNIQUE part of the CREATE TABLE does auto-generate the following index:

CREATE UNIQUE INDEX "device metrics record_is reported by-device_key" ON public."device metrics record" USING ("is reported by-device");

which makes device_metrics_record_by_device_idx redundant/duplicate.

If we still prefer to explicitly be adding index as part of the migration just in case, then it should probably be matching the auto-generated one. (but tbh I don't think we should b/c pg adds it out of the box)

Adding a unique constraint will automatically create a unique btree index on the column or group of columns used in the constraint.

See: https://www.postgresql.org/docs/current/sql-createtable.html

Page- · 2023-07-04T15:11:09Z

src/balena.sbvr

-Fact type: device has memory usage
-	Necessity: each device has at most one memory usage
-Fact type: device has memory total
-	Necessity: each device has at most one memory total
-Fact type: device has storage block device
-	Necessity: each device has at most one storage block device
-Fact type: device has storage usage
-	Necessity: each device has at most one storage usage
-Fact type: device has storage total
-	Necessity: each device has at most one storage total
-Fact type: device has cpu usage
-	Necessity: each device has at most one cpu usage
-Fact type: device has cpu temp
-	Necessity: each device has at most one cpu temp
-Fact type: device is undervolted


I don't think this can be safely removed until the async migration relying on it is finalized

Remodel `device` resource and split metrics into `device metrics record` This reduces the load on the `device` resource when updating device metrics periodically. device metrics are far less frequently needed than the `device` resource. Thus writing to the heavily used `device` resource should be offloaded to the `device metrics record` resource. Change-type: major Signed-off-by: Harald Fischer <harald@balena.io>

fisehara · 2023-07-06T22:07:52Z

@Page- @thgreasi when this is fine I'll follow up with a PR for finalising the async migration and dropping the columns from device resource.

thgreasi · 2023-07-07T06:42:46Z

src/features/device-state/routes/state-patch-v3.ts

+							await resinApiTx.patch({
+								resource: 'device_metrics_record',
+								id: { is_reported_by__device: device.id },
+								body: metricsBody,


How about adding v both here and in the v2 patch?

Suggested change

body: metricsBody,

options: {

$filter: {

$not: metricsBody

}

},

body: metricsBody,

Yeah, this avoids an unnecessary postgres write in the case there are no changes

thgreasi · 2023-07-07T09:18:20Z

src/features/device-state/index.ts

@@ -23,9 +23,11 @@ export {
 	serviceInstallFromImage,
 } from './state-get-utils';
 export {
-	metricsPatchFields,
+	validDeviceMetricsRecordPatchFields,


NIT: Since this field is also exported from index.ts w/ export * as deviceState from './features/device-state';, how about not renaming it so that we don't introduce a breaking change?

thgreasi · 2023-07-07T09:21:41Z

src/balena-init.sql

@@ -188,3 +188,6 @@ ON "application" ("slug" varchar_pattern_ops, "is public", "is host");

 CREATE INDEX IF NOT EXISTS "scheduled_job_run_start_timestamp_idx"
 ON "scheduled job run" (DATE_TRUNC('milliseconds', "start timestamp"));
+
+CREATE INDEX IF NOT EXISTS "device_metrics_record_by_device_idx"


Reminder about the discussion on this at
See: https://github.com/balena-io/open-balena-api/pull/1371/files#r1255500040

thgreasi · 2023-07-07T09:26:29Z

src/migrations/00085-split-device-metrics-from-device.async.ts

+					FROM
+						"device metrics record"
+				)
+			FOR UPDATE SKIP LOCKED


Suggested change

FOR UPDATE SKIP LOCKED

FOR UPDATE

The sync migration one should not leave anything pending.

thgreasi · 2023-07-07T09:32:35Z

src/features/device-state/routes/state-patch-v3.ts

+					reports__device_metrics_record: {},
 				},
 			},
 		})) as Array<
 			Pick<Device, 'id' | 'uuid'> & {
 				belongs_to__application: Array<Pick<Application, 'uuid'>>;
+				reports__device_metrics_record: DeviceMetricsRecord[];


We don't really care to fetch any of the rest columns.

Suggested change

reports__device_metrics_record: {},

},

},

})) as Array<

Pick<Device, 'id' | 'uuid'> & {

belongs_to__application: Array<Pick<Application, 'uuid'>>;

reports__device_metrics_record: DeviceMetricsRecord[];

reports__device_metrics_record: {

$select: 'id'

},

},

},

})) as Array<

Pick<Device, 'id' | 'uuid'> & {

belongs_to__application: Array<Pick<Application, 'uuid'>>;

reports__device_metrics_record: reports__device_metrics_record: [Pick<DeviceMetricsRecord, 'id'>?];

thgreasi · 2023-07-07T09:34:36Z

src/features/device-state/routes/state-patch-v2.ts

+					const latestDeviceMetricsRecord = await resinApiTx.get({
+						resource: 'device_metrics_record',
+						id: { is_reported_by__device: deviceId },
+					});


Suggested change

const latestDeviceMetricsRecord = await resinApiTx.get({

resource: 'device_metrics_record',

id: { is_reported_by__device: deviceId },

});

const latestDeviceMetricsRecord = await resinApiTx.get({

resource: 'device_metrics_record',

id: { is_reported_by__device: deviceId },

options: {

$select: 'id'

}

});

thgreasi · 2023-07-07T09:36:35Z

src/features/device-state/routes/state-patch-v2.ts

+						await resinApiTx.post({
+							resource: 'device_metrics_record',
+							id: { is_reported_by__device: deviceId },
+							body: {
+								...metricsBody,
+							},
+						});


Does that work? Shouldn't it be v ?

Suggested change

await resinApiTx.post({

resource: 'device_metrics_record',

id: { is_reported_by__device: deviceId },

body: {

...metricsBody,

},

});

await resinApiTx.post({

resource: 'device_metrics_record',

body: {

...metricsBody,

is_reported_by__device: deviceId,

},

});

thgreasi · 2023-07-07T09:38:41Z

src/features/device-state/routes/state-patch-v3.ts

+								resource: 'device_metrics_record',
+								body: {
+									...metricsBody,
+									...{ is_reported_by__device: device.id },


Suggested change

...{ is_reported_by__device: device.id },

is_reported_by__device: device.id,

I less object to GC :)

thgreasi · 2023-07-07T11:43:39Z

src/features/device-state/routes/state-patch-v2.ts

+					} else {
+						await resinApiTx.patch({
+							resource: 'device_metrics_record',
+							id: { is_reported_by__device: deviceId },


Suggested change

id: { is_reported_by__device: deviceId },

id: { is_reported_by__device: deviceId },

options: {

$filter: {

$not: metricsBody

}

},

Page- · 2023-10-19T12:13:11Z

src/features/device-state/routes/state-patch-v3.ts

-			if (apps != null || Object.keys(deviceBody).length > 0) {
+			if (
+				Object.keys(metricsBody).length > 0 &&
+				!(await shouldUpdateMetrics(uuid))


There's a special case I think where we always want to recognize changes to the is_undervolted property which we currently always accept but this PR has moved to being treated like any of the other "metrics" fields

Page- · 2023-10-19T12:14:04Z

src/features/device-state/routes/state-patch-v3.ts

+							await resinApiTx.patch({
+								resource: 'device_metrics_record',
+								id: { is_reported_by__device: device.id },
+								body: metricsBody,


Yeah, this avoids an unnecessary postgres write in the case there are no changes

fisehara marked this pull request as draft July 4, 2023 09:05

fisehara force-pushed the fisehara/split-device-metrics branch 2 times, most recently from 65c5775 to 70730be Compare July 4, 2023 12:53

fisehara requested review from thgreasi and Page- July 4, 2023 12:57

fisehara force-pushed the fisehara/split-device-metrics branch from 70730be to 4b2e937 Compare July 4, 2023 13:26

Page- reviewed Jul 4, 2023

View reviewed changes

fisehara requested review from Page- and thgreasi and removed request for thgreasi July 6, 2023 22:04

fisehara force-pushed the fisehara/split-device-metrics branch from 4b2e937 to 533ab03 Compare July 6, 2023 22:05

thgreasi reviewed Jul 7, 2023

View reviewed changes

Page- reviewed Oct 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split metrics from `device` resource #1371

Split metrics from `device` resource #1371

fisehara commented Jul 4, 2023

Page- Jul 4, 2023

fisehara Jul 6, 2023

Page- Jul 4, 2023

fisehara Jul 6, 2023

Page- Jul 4, 2023

fisehara Jul 6, 2023

thgreasi Jul 7, 2023 •

edited

thgreasi Jul 7, 2023

Page- Jul 4, 2023

fisehara commented Jul 6, 2023

thgreasi Jul 7, 2023

Page- Oct 19, 2023

thgreasi Jul 7, 2023

thgreasi Jul 7, 2023

thgreasi Jul 7, 2023

thgreasi Jul 7, 2023

thgreasi Jul 7, 2023

thgreasi Jul 7, 2023

thgreasi Jul 7, 2023 •

edited

thgreasi Jul 7, 2023

Page- Oct 19, 2023

Page- Oct 19, 2023

		CREATE INDEX IF NOT EXISTS "device_metrics_record_by_device_idx"
		ON "device metrics record" ("is reported by-device");

-								body: metricsBody,
+								options: {
+									$filter: {
+										$not: metricsBody
+									}
+								},
+								body: metricsBody,

	...{ is_reported_by__device: device.id },
	is_reported_by__device: device.id,

Split metrics from device resource #1371

Are you sure you want to change the base?

Split metrics from device resource #1371

Conversation

fisehara commented Jul 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thgreasi Jul 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisehara commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thgreasi Jul 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Split metrics from `device` resource #1371

Split metrics from `device` resource #1371

thgreasi Jul 7, 2023 •

edited

thgreasi Jul 7, 2023 •

edited