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

[Alerting] formalize alert status and add status fields to alert saved object #75553

Merged
merged 9 commits into from
Oct 1, 2020
23 changes: 23 additions & 0 deletions x-pack/plugins/alerts/common/alert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,28 @@ export interface IntervalSchedule extends SavedObjectAttributes {
interval: string;
}

// for the `typeof ThingValues[number]` types below, become string types that
// only accept the values in the associated string arrays
export const AlertExecutionStatusValues = ['ok', 'active', 'error', 'unknown'] as const;
export type AlertExecutionStatuses = typeof AlertExecutionStatusValues[number];

export const AlertExecutionStatusErrorReasonValues = [
'read',
'decrypt',
'execute',
'unknown',
] as const;
export type AlertExecutionStatusErrorReasons = typeof AlertExecutionStatusErrorReasonValues[number];

export interface AlertExecutionStatus {
status: AlertExecutionStatuses;
date: Date;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we rename date to something that is explicit about what date it is?
lastExecution? lastUpdate? etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

it does feel kind of "context free", viewed here, but it is a property of the executionStatus object, so I thought using date by itself would be fine; ie, you're always be referencing this piece of data as executionStatus.date. Adding additional context on date itself seemed like overkill and overly wordy. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

well, it isn't obvious to me what this date actually is 🤷
I won't block on this, but my feeling is that we're adding to the cognitive load by not being clear what his date actually means.
I'm guessing it means "lastUpdate" of the status, but I'm still not 100% sure and to me that's a reason to clarify.

Copy link
Member Author

Choose a reason for hiding this comment

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

But you know what the status is? Should we change it to lastExecutionStatus as well? :-) I think that was my thinking in trying to not add more context here to the prop names, when it feels like it's implied by it's containing property &| type.

Contextually, how people would end up accessing this, would look like the following for the two variants:

  • alert.executionStatus.date
  • alert.executionStatus.lastExecutionDate

I don't have really strong feels, just trying to cut down verbosity / ceremony / overkill where not actually needed.

But I just thought of a decent reason to do this - if for some reason we add some other date to this structure later, THEN it will certainly be confusing what the un-prefixed date would be.

So, I think lastExecutionDate prolly works best for me, since that exactly describes it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Current branch is using lastExecutionDate - thanks for prodding on this Gidi!

error?: {
reason: AlertExecutionStatusErrorReasons;
message: string;
};
}

export type AlertActionParams = SavedObjectAttributes;

export interface AlertAction {
Expand Down Expand Up @@ -44,6 +66,7 @@ export interface Alert {
throttle: string | null;
muteAll: boolean;
mutedInstanceIds: string[];
executionStatus: AlertExecutionStatus;
}

export type SanitizedAlert = Omit<Alert, 'apiKey'>;
20 changes: 20 additions & 0 deletions x-pack/plugins/alerts/server/alerts_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,11 @@ describe('create()', () => {
"createdAt": "2019-02-12T21:01:22.479Z",
"createdBy": "elastic",
"enabled": true,
"executionStatus": Object {
"date": "2019-02-12T21:01:22.479Z",
"error": null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right that we set error to null because of the partial update issue?
I just want to double check I understand, as my instinct would have been to just omit it if there is no error... but obviously, being aware of the partial update issues, I'm assuming it's that. is it worth adding a comment in the code for future devs who might not be aware? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, it's a partial update issue; we need to make sure we remove a previous error if we're doing an update and there is no error this time. So it's typed in the raw alert as "null-able".

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a comment in the raw alert definition for this ...

"status": "unknown",
},
"meta": Object {
"versionApiKeyLastmodified": "v7.10.0",
},
Expand Down Expand Up @@ -1029,6 +1034,11 @@ describe('create()', () => {
muteAll: false,
mutedInstanceIds: [],
tags: ['foo'],
executionStatus: {
date: '2019-02-12T21:01:22.479Z',
status: 'unknown',
error: null,
},
},
{
references: [
Expand Down Expand Up @@ -1145,6 +1155,11 @@ describe('create()', () => {
muteAll: false,
mutedInstanceIds: [],
tags: ['foo'],
executionStatus: {
date: '2019-02-12T21:01:22.479Z',
status: 'unknown',
error: null,
},
},
{
references: [
Expand Down Expand Up @@ -2496,6 +2511,11 @@ const BaseAlertInstanceSummarySavedObject: SavedObject<RawAlert> = {
throttle: null,
muteAll: false,
mutedInstanceIds: [],
executionStatus: {
status: 'unknown',
date: '2020-08-20T19:23:38Z',
error: null,
},
},
references: [],
};
Expand Down
16 changes: 14 additions & 2 deletions x-pack/plugins/alerts/server/alerts_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
AlertTaskState,
AlertInstanceSummary,
} from './types';
import { validateAlertTypeParams } from './lib';
import { validateAlertTypeParams, alertExecutionStatusFromRaw } from './lib';
import {
InvalidateAPIKeyParams,
GrantAPIKeyResult as SecurityPluginGrantAPIKeyResult,
Expand Down Expand Up @@ -122,6 +122,7 @@ export interface CreateOptions {
| 'muteAll'
| 'mutedInstanceIds'
| 'actions'
| 'executionStatus'
Comment on lines 122 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

nit.
This list has grown a lot, should we switch it to the inverse (using Pick instead of Omit)?
It would mean, by default, new fields are not part of create... 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

You're not wrong, but I hate to make changes like this, in a PR like this. It's more of a general clean up thing, and this particular item seems hardly worth an issue by itself. Do we have some general "simple tech debt items" issue we could add this to?

> & { actions: NormalizedAlertAction[] };
options?: {
migrationVersion?: Record<string, string>;
Expand Down Expand Up @@ -228,6 +229,11 @@ export class AlertsClient {
params: validatedAlertTypeParams as RawAlert['params'],
muteAll: false,
mutedInstanceIds: [],
executionStatus: {
status: 'unknown',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. It feels like we should be using enums for these rather than hard coded strings, no?
I get that the wrong string won't pass type checking, but we've used enums for equivalent fields so it feels like we should align.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kinda look at the "set of these strings" type as being the ceremony-free version of string enums. Is there an explicit value in using enums instead? The upside to not using them is not having to maintain the duplication of the enum keys / values, and not having to have access to that enum type in code that needs it. ie, less ceremony :-)

Would the enum type be exposed in the Alert objects themselves, or just an internal detail? I think we could type it in the Alert and RawAlert as enum safely, but not quite sure. Another reason I generally avoid enums, because I always have to go read the chapter on them in the TS handbook :-)

date: new Date().toISOString(),
error: null,
},
};
const createdAlert = await this.unsecuredSavedObjectsClient.create(
'alert',
Expand Down Expand Up @@ -961,9 +967,14 @@ export class AlertsClient {
updatedAt: SavedObject['updated_at'] = createdAt,
references: SavedObjectReference[] | undefined
): PartialAlert {
const rawAlertWithoutExecutionStatus: Partial<Omit<RawAlert, 'executionStatus'>> = {
...rawAlert,
};
delete rawAlertWithoutExecutionStatus.executionStatus;
const executionStatus = alertExecutionStatusFromRaw(rawAlert.executionStatus);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit.
Could we just extract executionStatus on line 966 like we do with createdAt, meta and scheduledTaskId?
That avoids the need for the delete and the creation of rawAlertWithoutExecutionStatus.
We could also rename rawAlert to be explicit about it not containing the Execution Status if you feel that's important. 🤷

It just feels more in line with the rest of the code there

Copy link
Member Author

Choose a reason for hiding this comment

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

this was sooooo hard - I tried multiple approaches to this, including doing what you suggested. Not happy with that delete, and I this seemed like the best alternative.

The main problem is the types of the props in the RawAlert and Alert pretty much match up, so that ...rawAlertWithoutExecutionStatus fills in most of the bits without any typing errors. However, the types of the executionStatus in RawAlert and Alert are different. And because we're dealing with partials coming in and going out, it might not be there. So when doing something like what you suggest, you'll end up with an error TS thinks the type of the returned executionStatus is RawAlertExecutionStatus | AlertExecutionStatus, which clearly isn't kosher.

It's worth a comment I think, will help the next person looking at this save a few minutes when they try to fix it. heh

return {
id,
...rawAlert,
...rawAlertWithoutExecutionStatus,
// we currently only support the Interval Schedule type
// Once we support additional types, this type signature will likely change
schedule: rawAlert.schedule as IntervalSchedule,
Expand All @@ -973,6 +984,7 @@ export class AlertsClient {
...(updatedAt ? { updatedAt: new Date(updatedAt) } : {}),
...(createdAt ? { createdAt: new Date(createdAt) } : {}),
...(scheduledTaskId ? { scheduledTaskId } : {}),
...(executionStatus ? { executionStatus } : {}),
};
}

Expand Down
163 changes: 163 additions & 0 deletions x-pack/plugins/alerts/server/lib/alert_execution_status.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { AlertExecutionStatusErrorReasons } from '../types';
import {
executionStatusFromState,
executionStatusFromError,
alertExecutionStatusToRaw,
alertExecutionStatusFromRaw,
} from './alert_execution_status';
import { ErrorWithReason } from './error_with_reason';

describe('AlertExecutionStatus', () => {
describe('executionStatusFromState()', () => {
test('empty task state', () => {
const status = executionStatusFromState({});
checkDateIsNearNow(status.date);
expect(status.status).toBe('ok');
expect(status.error).toBe(undefined);
});

test('task state with no instances', () => {
const status = executionStatusFromState({ alertInstances: {} });
checkDateIsNearNow(status.date);
expect(status.status).toBe('ok');
expect(status.error).toBe(undefined);
});

test('task state with one instance', () => {
const status = executionStatusFromState({ alertInstances: { a: {} } });
checkDateIsNearNow(status.date);
expect(status.status).toBe('active');
expect(status.error).toBe(undefined);
});
});

describe('executionStatusFromError()', () => {
test('error with no reason', () => {
const status = executionStatusFromError(new Error('boo!'));
expect(status.status).toBe('error');
expect(status.error).toMatchInlineSnapshot(`
Object {
"message": "boo!",
"reason": "unknown",
}
`);
});

test('error with a reason', () => {
const status = executionStatusFromError(new ErrorWithReason('execute', new Error('hoo!')));
expect(status.status).toBe('error');
expect(status.error).toMatchInlineSnapshot(`
Object {
"message": "hoo!",
"reason": "execute",
}
`);
});
});

describe('alertExecutionStatusToRaw()', () => {
const date = new Date('2020-09-03T16:26:58Z');
const status = 'ok';
const reason: AlertExecutionStatusErrorReasons = 'decrypt';
const error = { reason, message: 'wops' };

test('status without an error', () => {
expect(alertExecutionStatusToRaw({ date, status })).toMatchInlineSnapshot(`
Object {
"date": "2020-09-03T16:26:58.000Z",
"error": null,
"status": "ok",
}
`);
});

test('status with an error', () => {
expect(alertExecutionStatusToRaw({ date, status, error })).toMatchInlineSnapshot(`
Object {
"date": "2020-09-03T16:26:58.000Z",
"error": Object {
"message": "wops",
"reason": "decrypt",
},
"status": "ok",
}
`);
});
});

describe('alertExecutionStatusFromRaw()', () => {
const date = new Date('2020-09-03T16:26:58Z').toISOString();
const status = 'active';
const reason: AlertExecutionStatusErrorReasons = 'execute';
const error = { reason, message: 'wops' };

test('no input', () => {
const result = alertExecutionStatusFromRaw();
expect(result).toBe(undefined);
});

test('undefined input', () => {
const result = alertExecutionStatusFromRaw(undefined);
expect(result).toBe(undefined);
});

test('null input', () => {
const result = alertExecutionStatusFromRaw(null);
expect(result).toBe(undefined);
});

test('invalid date', () => {
const result = alertExecutionStatusFromRaw({ date: 'an invalid date' })!;
checkDateIsNearNow(result.date);
expect(result.status).toBe('unknown');
expect(result.error).toBe(undefined);
});

test('valid date', () => {
const result = alertExecutionStatusFromRaw({ date });
expect(result).toMatchInlineSnapshot(`
Object {
"date": 2020-09-03T16:26:58.000Z,
"status": "unknown",
}
`);
});

test('valid status and date', () => {
const result = alertExecutionStatusFromRaw({ status, date });
expect(result).toMatchInlineSnapshot(`
Object {
"date": 2020-09-03T16:26:58.000Z,
"status": "active",
}
`);
});

test('valid status, date and error', () => {
const result = alertExecutionStatusFromRaw({ status, date, error });
expect(result).toMatchInlineSnapshot(`
Object {
"date": 2020-09-03T16:26:58.000Z,
"error": Object {
"message": "wops",
"reason": "execute",
},
"status": "active",
}
`);
});
});
});

// eslint-disable-next-line @typescript-eslint/no-explicit-any
function checkDateIsNearNow(date: any) {
expect(date instanceof Date).toBe(true);
// allow for lots of slop in the time difference
expect(Date.now() - date.valueOf()).toBeLessThanOrEqual(10000);
}
Comment on lines +181 to +185
Copy link
Contributor

Choose a reason for hiding this comment

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

nit.
Haha, love it, but might it be easier to maintain if we just use fakeTimers here? 😆

Copy link
Member Author

@pmuellr pmuellr Oct 1, 2020

Choose a reason for hiding this comment

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

Some day I'll have to learn fake timers bits; I'm always confused seeing we're using sinon when I think jest also has fake timers. I'll leave this as is for now, make a mental note to learn fake timers for the next one of these.

60 changes: 60 additions & 0 deletions x-pack/plugins/alerts/server/lib/alert_execution_status.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { AlertTaskState, AlertExecutionStatus, RawAlertExecutionStatus } from '../types';
import { getReasonFromError } from './error_with_reason';

export function executionStatusFromState(state: AlertTaskState): AlertExecutionStatus {
const instanceIds = Object.keys(state.alertInstances ?? {});
return {
date: new Date(),
status: instanceIds.length === 0 ? 'ok' : 'active',
};
}

export function executionStatusFromError(error: Error): AlertExecutionStatus {
return {
date: new Date(),
status: 'error',
error: {
reason: getReasonFromError(error),
message: error.message,
},
};
}

export function alertExecutionStatusToRaw({
date,
status,
error,
}: AlertExecutionStatus): RawAlertExecutionStatus {
return {
date: date.toISOString(),
status,
error: error ?? null,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this is in order to remove existing errors in the SO due to partial updates?
It might be worth adding a comment to clarify that for developers who don't have the context we do.

};
}

export function alertExecutionStatusFromRaw(
rawAlertExecutionStatus?: Partial<RawAlertExecutionStatus> | null | undefined
): AlertExecutionStatus | undefined {
if (!rawAlertExecutionStatus) return undefined;

const { date, status = 'unknown', error } = rawAlertExecutionStatus;

let parsedDateMillis = date ? Date.parse(date) : Date.now();
if (isNaN(parsedDateMillis)) {
// TODO: log a message?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, worth a debug message at least

Copy link
Member Author

Choose a reason for hiding this comment

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

but now I have to thread a Logger through a few methods :-(

Copy link
Member Author

@pmuellr pmuellr Oct 1, 2020

Choose a reason for hiding this comment

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

wow, this is going to get nasty - but TBH, these locations are a great place to have a logger, since we don't do much verification on the raw data anyway ...
not as bad as I originally thought :-) mostly there as an instance var in the callers, didn't happen to notice that at first

parsedDateMillis = Date.now();
}

const parsedDate = new Date(parsedDateMillis);
if (error) {
return { date: parsedDate, status, error };
} else {
return { date: parsedDate, status };
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -511,4 +511,8 @@ const BaseAlert: SanitizedAlert = {
createdAt: new Date(),
updatedAt: new Date(),
apiKeyOwner: null,
executionStatus: {
status: 'unknown',
date: new Date('2020-08-20T19:23:38Z'),
},
};
Loading