Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a012543
resolves https://github.com/elastic/kibana/issues/78148
dB2510 Oct 17, 2020
8a50c7f
Merge branch 'master' into master
kibanamachine Oct 19, 2020
c5808a0
required changes done for https://github.com/elastic/kibana/issues/78148
dB2510 Oct 21, 2020
5d3b8be
Merge branch 'master' of https://github.com/dB2510/kibana
dB2510 Oct 21, 2020
10cf1ec
Merge branch 'master' of https://github.com/elastic/kibana
dB2510 Oct 21, 2020
e00826b
requested changes done https://github.com/elastic/kibana/issues/78148
dB2510 Oct 21, 2020
86c0e5b
functional tests for index threshold default action message
pmuellr Oct 22, 2020
4cc33bc
resolved type-check and jest errors
dB2510 Oct 22, 2020
18c0da9
Merge branch 'master' of https://github.com/elastic/kibana
dB2510 Oct 22, 2020
7fe16cd
Merge pull request #1 from pmuellr/alerts/default-action-message-ft
dB2510 Oct 27, 2020
9c931be
Merge branch 'master' of https://github.com/elastic/kibana
dB2510 Oct 27, 2020
eb2b0c7
Merge branch 'master' of https://github.com/elastic/kibana
dB2510 Oct 29, 2020
5830f13
added functional UI test for defaultActionMessage
dB2510 Oct 29, 2020
31f50e6
Merge branch 'master' of https://github.com/elastic/kibana
dB2510 Nov 2, 2020
ebb3fee
added functional UI test to an existing test
dB2510 Nov 2, 2020
a519a6d
remove comments in x-pack/test/alerting_api_integration/spaces_only/t…
dB2510 Nov 3, 2020
b42992a
Merge branch 'master' of https://github.com/elastic/kibana
dB2510 Nov 6, 2020
a137daf
fix: i18n labels corrected to camelcase
dB2510 Nov 6, 2020
40a7093
Merge branch 'master' into master
kibanamachine Nov 9, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ export interface BaseActionContext extends AlertInstanceContext {
date: string;
// the value that met the threshold
value: number;
// the function that is used
function: string;
// the time window for aggregation
window: string;
}

export function addMessages(
Expand All @@ -42,10 +46,6 @@ export function addMessages(
},
});

const agg = params.aggField ? `${params.aggType}(${params.aggField})` : `${params.aggType}`;
const humanFn = `${agg} ${params.thresholdComparator} ${params.threshold.join(',')}`;

const window = `${params.timeWindowSize}${params.timeWindowUnit}`;
const message = i18n.translate(
'xpack.stackAlerts.indexThreshold.alertTypeContextMessageDescription',
{
Expand All @@ -55,8 +55,8 @@ export function addMessages(
name: alertInfo.name,
group: baseContext.group,
value: baseContext.value,
function: humanFn,
window,
function: baseContext.function,
window: baseContext.window,
date: baseContext.date,
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,20 @@ export function getAlertType(service: Service): AlertType<Params, {}, {}, Action
}
);

const actionVariableContextFunctionLabel = i18n.translate(
'xpack.stackAlerts.indexThreshold.actionVariableContextFunctionLabel',
{
defaultMessage: 'A string formatted values combining threshold comparator and threshold',
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
defaultMessage: 'A string formatted values combining threshold comparator and threshold',
defaultMessage: 'A string describing the threshold comparator and threshold',

}
);

const actionVariableContextWindowLabel = i18n.translate(
'xpack.stackAlerts.indexThreshold.actionVariableContextWindowLabel',
{
defaultMessage: 'A string formatted values combining time window size and time window unit',
Copy link
Member

Choose a reason for hiding this comment

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

In looking at this, I just realized we shouldn't need to add window as a separate context variable. There is already a context variable containing the parameters, called alertParams, so these can be referenced in the message string as

{{alertParams.timeWindowSize}}{{alertParams.timeWindowUnit}}

which should yield a string like 5s (5 seconds), which is all we're doing below anyway.

So we shouldn't need the window variable at all.

Note, in theory we might be able to do the function variable in the same way, but it would make the mustache template a lot more complicated, not sure it's worth it, and it feels useful to have a nice version of the "function" available easily anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmuellr Got it!

}
);

const alertParamsVariables = Object.keys(CoreQueryParamsSchemaProperties).map(
(propKey: string) => {
return {
Expand All @@ -107,6 +121,8 @@ export function getAlertType(service: Service): AlertType<Params, {}, {}, Action
{ name: 'group', description: actionVariableContextGroupLabel },
{ name: 'date', description: actionVariableContextDateLabel },
{ name: 'value', description: actionVariableContextValueLabel },
{ name: 'function', description: actionVariableContextFunctionLabel },
{ name: 'window', description: actionVariableContextWindowLabel },
],
params: [
{ name: 'threshold', description: actionVariableContextThresholdLabel },
Expand Down Expand Up @@ -160,10 +176,17 @@ export function getAlertType(service: Service): AlertType<Params, {}, {}, Action

if (!met) continue;

const agg = params.aggField ? `${params.aggType}(${params.aggField})` : `${params.aggType}`;
const humanFn = `${agg} ${params.thresholdComparator} ${params.threshold.join(',')}`;

const window = `${params.timeWindowSize}${params.timeWindowUnit}`;

const baseContext: BaseActionContext = {
date,
group: instanceId,
value,
function: humanFn,
window,
};
const actionContext = addMessages(options, baseContext, params);
const alertInstance = options.services.alertInstanceFactory(instanceId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import { lazy } from 'react';

import { i18n } from '@kbn/i18n';
import { AlertTypeModel } from '../../../../types';
import { validateExpression } from './validation';
import { IndexThresholdAlertParams } from './types';
Expand All @@ -17,6 +17,12 @@ export function getAlertType(): AlertTypeModel<IndexThresholdAlertParams, Alerts
iconClass: 'alert',
alertParamsExpression: lazy(() => import('./expression')),
validate: validateExpression,
defaultActionMessage: i18n.translate(
'xpack.triggersActionsUI.builtin_alert_types.threshold.alertDefaultActionMessage',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be camelCase to be consistent with our other i18n labels? xpack.triggersActionsUI.builtinAlertTypes.threshold.alertDefaultActionMessage

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that matches other ids. I noticed that ids using builtinActionTypes also either have a components or sections "prefix path" in them: eg

xpack.triggersActionsUI.components.builtinActionTypes.warningTitle

So, perhaps it should be:

xpack.triggersActionsUI.components.builtinAlertTypes.threshold.alertDefaultActionMessage

Not sure. Thoughts @YulNaumenko ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ymao1 @pmuellr Should I change that line to xpack.triggersActionsUI.components.builtinAlertTypes.threshold.alertDefaultActionMessage ?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I am good with that.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pmuellr Can I commit this change now?

Copy link
Member

Choose a reason for hiding this comment

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

Ya, go ahead.

{
defaultMessage: `alert \\{\\{alertName\\}\\} group \\{\\{context.group\\}\\} value \\{\\{context.value\\}\\} exceeded threshold \\{\\{context.function\\}\\} over \\{\\{context.window\\}\\} on \\{\\{context.date\\}\\}`,
}
),
requiresAppContext: false,
};
}