Added defaultActionMessage to index threshold alert UI type definition

[dB2510]

Fixes #78148

The above screenshot displays the defaultActionMessage in the text box when an action is created for index threshold alert.

[kibanamachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[cla-checker-service]

💚 CLA has been signed

[elasticmachine]

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

[marius-dr]

Thanks for working on this. Until the Kibana Alerting Services team gets back to you, I've added some version labels to it as it would be good to have it backported to 7.x as well. Also, don't forget to sign the CLA.

[dB2510]

@marius-dr I have signed the contributer agreement but here it is not showing.

[marius-dr]

It probably took a while to update. Thanks!

[marius-dr]

@elasticmachine merge upstream

[mikecote]

@pmuellr PR for #78148.

[pmuellr]

I'm going to do a manual test on this right now, I assume it will work as intended :-)

But wondering if we can add some other useful test. Ideally, a functional test, so we can see this in action. I'll start poking around and what we might want to do. It would probably involve checking the response from invoking the action, and then matching with a regex, as {{context.value}} and {{context.date}} are values we won't be able to pre-determine.

[pmuellr]

is the CI stuck on this? Or maybe the CI doesn't actually run unless we specifically tell it to, for community PRs - I think that's it.

[pmuellr]

jenkins retest this please

[pmuellr]

With a manual test, it looks like this differs from using {{context.message}}. Here are the two - I added spaces with the new default message showing where some values came out blank:

new alert index action name group host-2 value 160 exceeded threshold                                         over    on 2020-10-19T18:25:09.107Z
old alert index action name group host-4 value 160 exceeded threshold avg(system.cpu.total.norm.pct) > 0.8 over 5s on 2020-10-19T18:25:09.080Z

I'm guessing the referenced context variables are not set, maybe we don't even make those available, but then it would be good to add those to the existing context variables ...

[dB2510]

With a manual test, it looks like this differs from using {{context.message}}. Here are the two - I added spaces with the new default message showing where some values came out blank:

new alert index action name group host-2 value 160 exceeded threshold                                         over    on 2020-10-19T18:25:09.107Z
old alert index action name group host-4 value 160 exceeded threshold avg(system.cpu.total.norm.pct) > 0.8 over 5s on 2020-10-19T18:25:09.080Z

I'm guessing the referenced context variables are not set, maybe we don't even make those available, but then it would be good to add those to the existing context variables ...

@pmuellr what changes I have to make here? Is there a need to do a manual check that whether those referenced variables are set or not?

[pmuellr]

Ya, just checked, we don't have those context variables currently. We should add them, they'll be useful to customers in other situations anyway.

Here's where the existing ones are set:

kibana/x-pack/plugins/stack_alerts/server/alert_types/index_threshold/action_context.ts

Lines 49 to 63 in 4c81b1a

	const message = i18n.translate(
	'xpack.stackAlerts.indexThreshold.alertTypeContextMessageDescription',
	{
	defaultMessage:
	'alert {name} group {group} value {value} exceeded threshold {function} over {window} on {date}',
	values: {
	name: alertInfo.name,
	group: baseContext.group,
	value: baseContext.value,
	function: humanFn,
	window,
	date: baseContext.date,
	},
	}
	);

In the definition of BaseActionContext above that section of code, we should add two additional string fields:

• function, whose value would be the same as humanFn defined in that method
• window, whose value would be the same as window defined in that method

Those values need to be set in the alert type definition tho, here:

kibana/x-pack/plugins/stack_alerts/server/alert_types/index_threshold/alert_type.ts

Lines 163 to 167 in 4c81b1a

	const baseContext: BaseActionContext = {
	date,
	group: instanceId,
	value,
	};

That file will also need updates to add the new variables to the list here:

kibana/x-pack/plugins/stack_alerts/server/alert_types/index_threshold/alert_type.ts

Lines 103 to 110 in 4c81b1a

	actionVariables: {
	context: [
	{ name: 'message', description: actionVariableContextMessageLabel },
	{ name: 'title', description: actionVariableContextTitleLabel },
	{ name: 'group', description: actionVariableContextGroupLabel },
	{ name: 'date', description: actionVariableContextDateLabel },
	{ name: 'value', description: actionVariableContextValueLabel },
	],

And then the descriptions needed to be added as i18n strings like the existing ones above that code.

[pmuellr]

The i18n error in the CI seems to be caused by using triggers_actions_ui instead of triggersActionsUI, for the string above.

Suggested change

	'xpack.triggers_actions_ui.builtin_alert_types.threshold.alertDefaultActionMessage',
	'xpack.triggersActionsUI.builtin_alert_types.threshold.alertDefaultActionMessage',

That's a difference from other usages anyway, like this one:

kibana/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/expression.tsx

Line 109 in 4c81b1a

'xpack.triggersActionsUI.sections.alertAdd.threshold.fixErrorInExpressionBelowValidationMessage',

[dB2510]

@pmuellr Please review the changes

[pmuellr]

This is looking good! Noted that I realized we don't actually need a new window variable, as the value it provides should be available via the alertParams variables.

[pmuellr]

Suggested change

	defaultMessage: 'A string formatted values combining threshold comparator and threshold',
	defaultMessage: 'A string describing the threshold comparator and threshold',

[pmuellr]

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.

[dB2510]

@pmuellr Got it!

[dB2510]

@pmuellr No problem. So for now I have to commit the changes to your repo, right?

[dB2510]

@pmuellr I have tried resolving merge conflict. Please review once.

[pmuellr]

@dB2510 the merge looks good! No need to use my copy of the PR/branch if your's is still in good shape.

Again, it's not building, so starting one via a message ...

[pmuellr]

jenkins retest this please

[dB2510]

There's one thing in this PR that still kind of bugs me, and that's the name of the context variable function that got added:

My vote is for condition :)

Ooooh, I like that.
Realized I should poke around and see what other alert types are using for context names; let me do that in a bit and I'll post the results back here.

The log threshold alert is creating a conditions variable that's similar to this:

kibana/x-pack/plugins/infra/server/lib/alerting/log_threshold/log_threshold_executor.ts

Lines 629 to 635 in ef5287f

	const createConditionsMessageForCriteria = (criteria: Criteria) => {
	const parts = criteria.map((criterion, index) => {
	const { field, comparator, value } = criterion;
	return `${index === 0 ? '' : 'and'} ${field} ${comparator} ${value}`;
	});
	return parts.join(' ');
	};

So, seems like a winner, paving the cow paths ...

Now, singular or plural? :-) condtions seems like it still works even if there's only a single condition, and it may well be the case that we add additional filtering capabilities to index threshold anyway, so we may want it to be plural in the future, so thinking we should name it conditions.

@pmuellr Should I go for conditions?

[pmuellr]

@dB2510: Should I go for conditions?

Sure! Unless you want to just get this merged. We have some other changes to the defaultActionMessage coming, fairly soon, I'm happy to do that work then. Up to you. Longer we wait, more chances for more conflicts :-)

[dB2510]

@pmuellr Okay then its up to you. Can we merge this now?
One change is required now xpack.triggersActionsUI.components.builtinAlertTypes.threshold.alertDefaultActionMessage one in x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/index.ts file

[pmuellr]

One change is required now xpack.triggersActionsUI.components.builtinAlertTypes.threshold.alertDefaultActionMessage one in x-pack/plugins/triggers_actions_ui/public/application/components/builtin_alert_types/threshold/index.ts file

Ah, for some reason I thought this was already merged. We should go ahead and make this change, because once we create a new i18n key, then it becomes a little messy to rename it later - we should make it right now, since we know what the name will be.

After that, I think will be good to merge!

[dB2510]

@pmuellr Done! Can we merge this now?

[pmuellr]

Let's get the CI to pass, and then should be good to merge!

Thanks for all the effort here Dhruv!

[pmuellr]

jenkins retest this please

[dB2510]

Let's get the CI to pass, and then should be good to merge!

Thanks for all the effort here Dhruv!

@pmuellr Thank you for giving me the opportunity to contribute to this issue.

[pmuellr]

We'll need one more merge from upstream to get the prbot:outdated check to pass. You can try posting the comment it suggests, or do it manually.

[dB2510]

@elasticmachine merge upstream

[pmuellr]

jenkins retest this please

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 40a7093

Metrics [docs]

page load bundle size

id	before	after	diff
triggersActionsUi	132.7KB	133.1KB	+422.0B

History

• 💚 Build #86535 succeeded a137daf
• 💚 Build #86427 succeeded b42992a
• 💚 Build #85531 succeeded a519a6d
• 💔 Build #83258 failed e00826b
• 💔 Build #82560 failed 8a50c7f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[pmuellr]

Whew! Merged! I've started the backport on this, so the feature should be available when Kibana 7.11.0 comes out.

Thanks again @dB2510 !!