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

Split metrics from device resource #1371

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions src/balena-init.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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

ON "device metrics record" ("is reported by-device");
24 changes: 16 additions & 8 deletions src/balena-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,14 +364,6 @@ export interface Device {
public_address: string | null;
ip_address: string | null;
mac_address: string | null;
memory_usage: number | null;
memory_total: number | null;
storage_block_device: string | null;
storage_usage: number | null;
storage_total: number | null;
cpu_usage: number | null;
cpu_temp: number | null;
is_undervolted: boolean;
cpu_id: string | null;
is_running__release: { __id: number } | [Release?] | null;
download_progress: number | null;
Expand Down Expand Up @@ -403,6 +395,7 @@ export interface Device {
service_install?: ServiceInstall[];
installs__image?: ImageInstall[];
installs__application__has__service_name?: ServiceInstall[];
reports__device_metrics_record?: DeviceMetricsRecord[];
}

export interface DeviceEnvironmentVariable {
Expand Down Expand Up @@ -565,6 +558,21 @@ export interface UserHasPublicKey {
title: string;
}

export interface DeviceMetricsRecord {
created_at: DateString;
modified_at: DateString;
id: number;
is_reported_by__device: { __id: number } | [Device];
memory_usage: number | null;
memory_total: number | null;
storage_block_device: string | null;
storage_usage: number | null;
storage_total: number | null;
cpu_usage: number | null;
cpu_temp: number | null;
is_undervolted: boolean;
}

export interface DeviceTypeAlias {
created_at: DateString;
modified_at: DateString;
Expand Down
25 changes: 25 additions & 0 deletions src/balena.sbvr
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,31 @@ Fact type: device should be managed by release
Necessity: each device should be managed by at most one release


Term: device metrics record

-- device metrics record
Fact type: device metrics record is reported by device
Synonymous Form: device reports device metrics record
Necessity: each device metrics record is reported by exactly one device
Necessity: each device reports exactly one device metrics record

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


-- application config variable

Fact type: application config variable has value
Expand Down
1 change: 1 addition & 0 deletions src/features/cascade-delete/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ setupDeleteCascade('device', {
device_tag: 'device',
image_install: 'device',
service_install: 'device',
device_metrics_record: 'is_reported_by__device',
});

setupDeleteCascade('image', {
Expand Down
4 changes: 3 additions & 1 deletion src/features/device-state/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ export {
serviceInstallFromImage,
} from './state-get-utils';
export {
metricsPatchFields,
validDeviceMetricsRecordPatchFields,
Copy link
Member

Choose a reason for hiding this comment

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

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?

v2ValidPatchFields,
v2ValidDevicePatchFields,
v3ValidPatchFields,
v3ValidDevicePatchFields,
} from './state-patch-utils';

const gracefulGet = resolveOrDenyDevicesWithStatus(304);
Expand Down
67 changes: 38 additions & 29 deletions src/features/device-state/routes/state-patch-v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ import { getIP } from '../../../lib/utils';
import type { ImageInstall, PickDeferred } from '../../../balena-model';
import {
shouldUpdateMetrics,
metricsPatchFields,
v2ValidPatchFields,
upsertImageInstall,
deleteOldImageInstalls,
truncateShortTextFields,
StatePatchDeviceMetricsRecordBody,
validDeviceMetricsRecordPatchFields,
v2ValidDevicePatchFields,
} from '../state-patch-utils';
import type { ResolveDeviceInfoCustomObject } from '../middleware';

Expand All @@ -26,7 +27,7 @@ type LocalBody = NonNullable<StatePatchV2Body['local']>;
* These typings should be used as a guide to what should be sent, but cannot be trusted as what actually *is* sent.
*/
export type StatePatchV2Body = {
local?: {
local?: StatePatchDeviceMetricsRecordBody & {
should_be_running__release?: number;
name?: string;
/**
Expand All @@ -46,15 +47,6 @@ export type StatePatchV2Body = {
download_progress?: number | null;
api_port?: number;
api_secret?: string;
memory_usage?: number;
memory_total?: number;
storage_block_device?: string;
storage_usage?: number;
storage_total?: number;
cpu_temp?: number;
cpu_usage?: number;
cpu_id?: string;
is_undervolted?: boolean;
is_on__commit?: string | null;
apps?: Array<{
services?: {
Expand Down Expand Up @@ -139,20 +131,13 @@ export const statePatchV2: RequestHandler = async (req, res) => {
const { apps } = local;

let deviceBody:
| Pick<LocalBody, (typeof v2ValidPatchFields)[number]> & {
| Pick<LocalBody, (typeof v2ValidDevicePatchFields)[number]> & {
is_running__release?: number | null;
} = _.pick(local, v2ValidPatchFields);
let metricsBody: Pick<LocalBody, (typeof metricsPatchFields)[number]> =
_.pick(local, metricsPatchFields);
if (
Object.keys(metricsBody).length > 0 &&
(await shouldUpdateMetrics(uuid))
) {
// If we should force a metrics update then merge the two together and clear `metricsBody` so
// that we don't try to merge it again later
deviceBody = { ...deviceBody, ...metricsBody };
metricsBody = {};
}
} = _.pick(local, v2ValidDevicePatchFields);
const metricsBody: Pick<
LocalBody,
(typeof validDeviceMetricsRecordPatchFields)[number]
> = _.pick(local, validDeviceMetricsRecordPatchFields);

if (local.name != null) {
deviceBody.device_name = local.name;
Expand Down Expand Up @@ -191,15 +176,13 @@ export const statePatchV2: RequestHandler = async (req, res) => {
}
}
}

updateFns.push(async (resinApiTx) => {
if (Object.keys(deviceBody).length > 0) {
if (Object.keys(deviceBody).length > 0) {
updateFns.push(async (resinApiTx) => {
// truncate for resilient legacy compatible device state patch so that supervisors don't fail
// to update b/c of length violation of 255 (SBVR SHORT TEXT type) for ip and mac address.
// sbvr-types does not export SHORT TEXT VARCHAR length 255 to import.
deviceBody = truncateShortTextFields(deviceBody);
// If we're updating anyway then ensure the metrics data is included
deviceBody = { ...deviceBody, ...metricsBody };
await resinApiTx.patch({
resource: 'device',
id: deviceId,
Expand All @@ -208,6 +191,32 @@ export const statePatchV2: RequestHandler = async (req, res) => {
},
body: deviceBody,
});
});
}
}
if (
Object.keys(metricsBody).length > 0 &&
(await shouldUpdateMetrics(uuid))
) {
updateFns.push(async (resinApiTx) => {
const latestDeviceMetricsRecord = await resinApiTx.get({
resource: 'device_metrics_record',
id: { is_reported_by__device: deviceId },
});
Comment on lines +202 to +205
Copy link
Member

Choose a reason for hiding this comment

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

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'
}
});

if (latestDeviceMetricsRecord == null) {
await resinApiTx.post({
resource: 'device_metrics_record',
id: { is_reported_by__device: deviceId },
body: {
...metricsBody,
},
});
Comment on lines +207 to +213
Copy link
Member

Choose a reason for hiding this comment

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

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,
},
});

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

Choose a reason for hiding this comment

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

Suggested change
id: { is_reported_by__device: deviceId },
id: { is_reported_by__device: deviceId },
options: {
$filter: {
$not: metricsBody
}
},

body: metricsBody,
});
}
});
}
Expand Down
74 changes: 47 additions & 27 deletions src/features/device-state/routes/state-patch-v3.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,21 @@ import { getIP } from '../../../lib/utils';
import {
Application,
Device,
DeviceMetricsRecord,
Image,
ImageInstall,
PickDeferred,
Release,
} from '../../../balena-model';
import type { Filter } from 'pinejs-client-core';
import { metricsPatchFields, v3ValidPatchFields } from '..';
import { v3ValidDevicePatchFields } from '..';
import {
deleteOldImageInstalls,
upsertImageInstall,
shouldUpdateMetrics,
truncateShortTextFields,
StatePatchDeviceMetricsRecordBody,
validDeviceMetricsRecordPatchFields,
} from '../state-patch-utils';

const { BadRequestError, UnauthorizedError, InternalRequestError } = errors;
Expand All @@ -31,7 +34,7 @@ const { api } = sbvrUtils;
* These typings should be used as a guide to what should be sent, but cannot be trusted as what actually *is* sent.
*/
export type StatePatchV3Body = {
[uuid: string]: {
[uuid: string]: StatePatchDeviceMetricsRecordBody & {
status?: string;
os_version?: string;
os_variant?: string;
Expand All @@ -42,15 +45,7 @@ export type StatePatchV3Body = {
mac_address?: string;
api_port?: number;
api_secret?: string;
memory_usage?: number;
memory_total?: number;
storage_block_device?: string;
storage_usage?: number;
storage_total?: number;
cpu_temp?: number;
cpu_usage?: number;
cpu_id?: string;
is_undervolted?: boolean;

/**
* Used for setting dependent devices as online
*/
Expand Down Expand Up @@ -103,11 +98,13 @@ const fetchData = async (
belongs_to__application: {
$select: 'uuid',
},
reports__device_metrics_record: {},
},
},
})) as Array<
Pick<Device, 'id' | 'uuid'> & {
belongs_to__application: Array<Pick<Application, 'uuid'>>;
reports__device_metrics_record: DeviceMetricsRecord[];
Comment on lines +101 to +107
Copy link
Member

Choose a reason for hiding this comment

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

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'>?];

}
>;
if (devices.length !== uuids.length) {
Expand Down Expand Up @@ -269,23 +266,14 @@ export const statePatchV3: RequestHandler = async (req, res) => {
let deviceBody:
| Pick<
StatePatchV3Body[string],
(typeof v3ValidPatchFields)[number]
(typeof v3ValidDevicePatchFields)[number]
> & {
is_running__release?: number | null;
} = _.pick(state, v3ValidPatchFields);
} = _.pick(state, v3ValidDevicePatchFields);
let metricsBody: Pick<
StatePatchV3Body[string],
(typeof metricsPatchFields)[number]
> = _.pick(state, metricsPatchFields);
if (
Object.keys(metricsBody).length > 0 &&
(await shouldUpdateMetrics(uuid))
) {
// If we should force a metrics update then merge the two together and clear `metricsBody` so
// that we don't try to merge it again later
deviceBody = { ...deviceBody, ...metricsBody };
metricsBody = {};
}
(typeof validDeviceMetricsRecordPatchFields)[number]
> = _.pick(state, validDeviceMetricsRecordPatchFields);

if (deviceBody.cpu_id != null) {
if (/[^\x20-\x7E]/.test(deviceBody.cpu_id)) {
Expand All @@ -297,12 +285,25 @@ export const statePatchV3: RequestHandler = async (req, res) => {
}
}

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

Choose a reason for hiding this comment

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

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

) {
// If we don't want to update the metrics then clear them
metricsBody = {};
}

if (
apps != null ||
Object.keys(deviceBody).length > 0 ||
Object.keys(metricsBody).length > 0
Page- marked this conversation as resolved.
Show resolved Hide resolved
) {
// We lazily fetch the necessary data only if we absolutely must to avoid unnecessary work if it turns out we don't need it
data ??= await fetchData(req, custom, uuids, appReleasesCriteria);
const { images, releasesByAppUuid } = data;
const device = data.devicesByUuid[uuid];

const [latestDeviceMetricsRecord] =
device.reports__device_metrics_record;
if (apps != null) {
const userAppUuid = device.belongs_to__application[0].uuid;
if (releasesByAppUuid[userAppUuid] != null) {
Expand All @@ -321,7 +322,6 @@ export const statePatchV3: RequestHandler = async (req, res) => {
// sbvr-types does not export SHORT TEXT VARCHAR length 255 to import.
deviceBody = truncateShortTextFields(deviceBody);
// If we're updating anyway then ensure the metrics data is included
deviceBody = { ...deviceBody, ...metricsBody };
updateFns.push(async (resinApiTx) => {
await resinApiTx.patch({
resource: 'device',
Expand All @@ -334,6 +334,26 @@ export const statePatchV3: RequestHandler = async (req, res) => {
});
}

if (Object.keys(metricsBody).length > 0) {
updateFns.push(async (resinApiTx) => {
if (latestDeviceMetricsRecord == null) {
await resinApiTx.post({
resource: 'device_metrics_record',
body: {
...metricsBody,
...{ is_reported_by__device: device.id },
Copy link
Member

@thgreasi thgreasi Jul 7, 2023

Choose a reason for hiding this comment

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

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

I less object to GC :)

},
});
} else {
await resinApiTx.patch({
resource: 'device_metrics_record',
id: { is_reported_by__device: device.id },
body: metricsBody,
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
body: metricsBody,
options: {
$filter: {
$not: metricsBody
}
},
body: metricsBody,

Copy link
Contributor

Choose a reason for hiding this comment

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

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

});
}
});
}

if (apps != null) {
const imgInstalls: Array<{
imageId: number;
Expand Down
Loading