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

add defaultActionMessage to index threshold alert UI type definition #78148

Closed
pmuellr opened this issue Sep 22, 2020 · 23 comments · Fixed by #80936
Closed

add defaultActionMessage to index threshold alert UI type definition #78148

pmuellr opened this issue Sep 22, 2020 · 23 comments · Fixed by #80936
Assignees
Labels
Feature:Alerting good first issue low hanging fruit Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@pmuellr
Copy link
Member

pmuellr commented Sep 22, 2020

It was recently noted that the index threshold alertType does not provide a defaultActionMessage - but it seems like it should. That would go in the object returned here:

export function getAlertType(): AlertTypeModel<IndexThresholdAlertParams, AlertsContextValue> {
return {
id: '.index-threshold',
name: 'Index threshold',
iconClass: 'alert',
alertParamsExpression: lazy(() => import('./expression')),
validate: validateExpression,
requiresAppContext: false,
};
}

@pmuellr pmuellr added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Sep 22, 2020
@elasticmachine
Copy link
Contributor

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

@mikecote mikecote added the good first issue low hanging fruit label Sep 23, 2020
@dB2510
Copy link
Contributor

dB2510 commented Sep 29, 2020

@pmuellr @mikecote I am new to elastic/kibana. Can I take up this issue?

@pmuellr
Copy link
Member Author

pmuellr commented Sep 30, 2020

@dB2510 sure!

Here's what I was thinking. The index threshold alert creates a context variable named message, here:

const message = i18n.translate(
'xpack.alertingBuiltins.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,
},
}
);

Specifically,

alert {name} group {group} value {value} exceeded threshold {function} over {window} on {date}'

The defaultActionMessage property mentioned at the top should, at a minimum, match that. It will be a mustache template using the existing context variables.

My first take would be setting it to this string:

alert {{alertName}} group {{context.group}} value {{context.value}} exceeded threshold {{context.function}} over {{context.window}} on {{context.date}}'

I believe that would be a direct replacement, and would be good enough to ship with.

We should make sure there is a functional test somewhere that will end up using this as the message, so we can validate that it's rendering as we expect. I'm guessing we don't have one now, but will poke around a bit to look for one, or a place to add a new one, more when you get to that point.

If you've not contributed to Kibana before, there's some guidelines here: https://www.elastic.co/guide/en/kibana/master/contributing.html - per that page, it's unlikely we'll need a release note for this change.

@dB2510
Copy link
Contributor

dB2510 commented Oct 1, 2020

@pmuellr I am moving forward with the information you have provided. Can you please guide me on how can I reproduce this issue?

@pmuellr
Copy link
Member Author

pmuellr commented Oct 2, 2020

The repro would be:

  • create an index threshold alert
  • add an action - the server log action is fine
  • the "message" field for the server log action will be blank

Once we implement the defaultActionMessage property on the index threshold alert, the same scenario would have the "message" field of the action parameter filled in with the mustache template (eg, alert {{alertName}} group {{context.group}} ...)

@dB2510
Copy link
Contributor

dB2510 commented Oct 5, 2020

@pmuellr For creating an alert, do we require TLS between elasticsearch and kibana compulsorily?

@pmuellr
Copy link
Member Author

pmuellr commented Oct 5, 2020

TLS between elasticsearch and kibana is compulsorily IF security is enabled. You can run without security enabled (ES + Kibana).

At development time, the easiest way to launch ES and Kibana with security is via the following commands, run in separate terminal windows:

yarn es snapshot --ssl --license trial   # start elasticsearch
yarn start --ssl                         # start Kibana

Also for development time, you should run yarn kbn bootstrap before the two other commands, to get all the core plugins built correctly. The VS Code typescript support will also work correctly after running the bootstrap command (you should restart VS Code after it completes). After running the bootstrap once, you'll only need to run it again after checking out a different git branch, and get everything initialized for that branch.

@dB2510
Copy link
Contributor

dB2510 commented Oct 7, 2020

@pmuellr It worked. You said that message field of server log action should be empty but when I tried that it was saying that message is required. Then I added a 2 word message and saved the alert but was unable to observe that message whenever that alert is running in instances section of that alert.
These were settings that I did when creating an alert

Select an index
INDEX .kibana-event-log-8.0.0
WHEN count()
OVER all documents

Define the condition
IS ABOVE 10
FOR THE LAST 1 minute

And created a server log connector with level info.

@dB2510
Copy link
Contributor

dB2510 commented Oct 7, 2020

@pmuellr Is this issue has been resolved by this #78930 pull request?

@pmuellr
Copy link
Member Author

pmuellr commented Oct 7, 2020

@dB2510: Is this issue has been resolved by this #78930 pull request?

No, the prefix of [APM] is a clue - the APM solution team provides several different alert types, and has been adding the defaultActionMessage to their own alerts. Since they don't "own" the index threshold alert, they didn't change it.

This PR only needs to cover the index threshold alert, which is the only alert provided by the alerting team.

@pmuellr
Copy link
Member Author

pmuellr commented Oct 7, 2020

@dB2510: You said that message field of server log action should be empty but when I tried that it was saying that message is required.

Sorry, let me clarify - when you create an index threshold action today, and then add the server log action, the "message" field will be blank. We do require the user to enter some message text in there. Sounds like it's all working the way it's supposed to.

Once this PR is working, when you run the same scenario, instead of the "message" field being blank, it will be initialized to the value by defaultActionMessage.

The idea is to provide a default message for users so they don't HAVE to enter one in themselves. The message is editable, so a user can customize it however they want, but we'd like to provide a nice default value for them to start with.

@pmuellr
Copy link
Member Author

pmuellr commented Oct 7, 2020

I've started to think about how we want to test this. I would like to have some kind of test that the defaultActionMessage will end up resulting in a reasonable string, when rendering it with some sample data, which we'd end up testing against in the test. So the test would show the rendered output. I think this would likely just be a jest test (a file with a .test.ts extension, like you'll see in the triggers_actions_ui plugin directories.

It seems like the test should call the exported function here in the transform_action_params.ts module (links to these files below) - that function takes the "action params" for an action - for server log that would be message and level- defined in the server_log.ts module, and returns a copy of it where every string in the object (both message and level, if set (you don't need to test this value though)), has been run through mustache passing a set of "context variables" like alertId, alertName, etc.

So, we'll figure out all the different cases we want to check for - and write a jest test for each of those that sets up all the parameters to that function, calls it, and checks that the result - in this case the message field for server log, is set as we expect.

Those notes are basically for me :-). When you get to the point of testing this, I could probably create the initial test with all the setup and stuff, and then let you fill in the rest.

@dB2510
Copy link
Contributor

dB2510 commented Oct 8, 2020

@dB2510: You said that message field of server log action should be empty but when I tried that it was saying that message is required.

Sorry, let me clarify - when you create an index threshold action today, and then add the server log action, the "message" field will be blank. We do require the user to enter some message text in there. Sounds like it's all working the way it's supposed to.

Once this PR is working, when you run the same scenario, instead of the "message" field being blank, it will be initialized to the value by defaultActionMessage.

The idea is to provide a default message for users so they don't HAVE to enter one in themselves. The message is editable, so a user can customize it however they want, but we'd like to provide a nice default value for them to start with.

@pmuellr What I understood now is that basically we don't want that user should bother about the action message by providing him/her a resonable default action message because action message text field is a required text field.

@pmuellr
Copy link
Member Author

pmuellr commented Oct 8, 2020

Correct!

This defaultActionMessage will end up automatically being used in various actions where a "message" can be used - at least server log, email (the body of the email) and slack (the message posted) - once we provide a string value for that optional defaultActionMessage property.

@dB2510
Copy link
Contributor

dB2510 commented Oct 8, 2020

@pmuellr On which branch should I start contributing?

@pmuellr
Copy link
Member Author

pmuellr commented Oct 8, 2020

Good question - create your branch off master.

@dB2510
Copy link
Contributor

dB2510 commented Oct 8, 2020

@dB2510 sure!

Here's what I was thinking. The index threshold alert creates a context variable named message, here:

const message = i18n.translate(
'xpack.alertingBuiltins.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,
},
}
);

Specifically,

alert {name} group {group} value {value} exceeded threshold {function} over {window} on {date}'

The defaultActionMessage property mentioned at the top should, at a minimum, match that. It will be a mustache template using the existing context variables.

My first take would be setting it to this string:

alert {{alertName}} group {{context.group}} value {{context.value}} exceeded threshold {{context.function}} over {{context.window}} on {{context.date}}'

I believe that would be a direct replacement, and would be good enough to ship with.

We should make sure there is a functional test somewhere that will end up using this as the message, so we can validate that it's rendering as we expect. I'm guessing we don't have one now, but will poke around a bit to look for one, or a place to add a new one, more when you get to that point.

If you've not contributed to Kibana before, there's some guidelines here: https://www.elastic.co/guide/en/kibana/master/contributing.html - per that page, it's unlikely we'll need a release note for this change.

@pmuellr You are expecting something like this with some changes, right?
And one more thing, current master branch doesn't have this piece of code. You have referenced some other branch in this comment.

@pmuellr
Copy link
Member Author

pmuellr commented Oct 8, 2020

@pmuellr You are expecting something like this with some changes, right?

Yes. The PR itself should reference the original issue, GH and other tooling will then associate the issue and PR. We usually add a line in the commit message, which will do this:

resolves https://github.com/elastic/kibana/issues/78148

I see I mentioned adding a "functional" test, but in previous comment #78148 (comment) mentioned that it would probably be hard to do a functional test, and we should try to do a jest test instead.

Functional tests are where we run an elasticsearch index, and kibana (instrumented with extra test code), and then the tests all run in a separate process accessing kibana via HTTP. They can be complicated to build, and are slow.

Jest tests are stand-alone tests of individual modules, where we create jest mocks for all the things the module depends on. Easier to write, and typically take seconds to run, and you can easily debug them in VS Code, etc. Once you get the PR up, I can add a commit to add the necessary structure for a new test, and you can add the appropriate test body, if needed. I'm not sure where the test would go, there might already be an existing test file that would be appropriate for it.

The plugin name changed (which is rare) from alerting_builtins to stack_alerts - the code now lives here in master:

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,
},
}
);
return { ...baseContext, title, message };
}

@dB2510
Copy link
Contributor

dB2510 commented Oct 13, 2020

@pmuellr Is there any way I can test my changes locally?
defaultMessage: 'alert {name} group {group} value {value} exceeded threshold {function} over {window} on {date}'
I want to replace the contents of defaultMessage with some random text and test that locally.

@pmuellr
Copy link
Member Author

pmuellr commented Oct 14, 2020

Here's how I do a local test of es and kibana; run the following commands to get the packages built - you'll need yarn globally installed:

  • yarn kbn bootstrap
  • node scripts/build_kibana_platform_plugins

Then start elasticsearch via:

yarn es snapshot --ssl --license trial

Then start Kibana via:

yarn start --ssl --no-base-path

That will start with a trial license, and Kibana will be available at https://localhost:5601/

@dB2510
Copy link
Contributor

dB2510 commented Oct 16, 2020

@pmuellr
Screenshot from 2020-10-16 09-02-38

I think when selecting {{context.message}} option from above drop-down it is giving the desired result.

@pmuellr
Copy link
Member Author

pmuellr commented Oct 16, 2020

That's correct - we do provide a context variable which will be set to what we want the default message set to.

But what we want to happen, is when the Create Alert panel appears, and an action that can use a "message" is added, instead of the Message field being blank, it should be filled in with a default mustache template, for example

alert {{alertName}} group {{context.group}} value {{context.value}} exceeded threshold {{context.function}} over {{context.window}} on {{context.date}}'

To make that work, we just need to set defaultActionMessage to that template string, inside the AlertTypeModel, returned here:

export function getAlertType(): AlertTypeModel<IndexThresholdAlertParams, AlertsContextValue> {
return {
id: '.index-threshold',
name: 'Index threshold',
iconClass: 'alert',
alertParamsExpression: lazy(() => import('./expression')),
validate: validateExpression,
requiresAppContext: false,
};
}

Here's an example from another plugin:

export function createInventoryMetricAlertType(): AlertTypeModel {
return {
id: METRIC_INVENTORY_THRESHOLD_ALERT_TYPE_ID,
name: i18n.translate('xpack.infra.metrics.inventory.alertFlyout.alertName', {
defaultMessage: 'Inventory',
}),
iconClass: 'bell',
alertParamsExpression: React.lazy(() => import('./components/expression')),
validate: validateMetricThreshold,
defaultActionMessage: i18n.translate(
'xpack.infra.metrics.alerting.inventory.threshold.defaultActionMessage',
{
defaultMessage: `\\{\\{alertName\\}\\} - \\{\\{context.group\\}\\} is in a state of \\{\\{context.alertState\\}\\}
Reason:
\\{\\{context.reason\\}\\}
`,
}
),
requiresAppContext: false,
};
}

That's the inventory alert. When I create one of those alerts, and then add a Slack action, you can see this string pre-filled in the Message field:

image

@dB2510
Copy link
Contributor

dB2510 commented Oct 17, 2020

@pmuellr I have opened the PR referencing this issue. I didn't know about the contributer-agreement so I have signed the contributer-agreement after opening the PR.

pmuellr pushed a commit that referenced this issue Nov 9, 2020
…ion (#80936)

* resolves #78148

Adds a `defaultActionMessage` to the index threshold alert, so that the `message` parameter for actions will be pre-filled with a useful message
pmuellr pushed a commit to pmuellr/kibana that referenced this issue Nov 9, 2020
…ion (elastic#80936)

* resolves elastic#78148

Adds a `defaultActionMessage` to the index threshold alert, so that the `message` parameter for actions will be pre-filled with a useful message
pmuellr added a commit that referenced this issue Nov 9, 2020
…ion (#80936) (#82950)

* resolves #78148

Adds a `defaultActionMessage` to the index threshold alert, so that the `message` parameter for actions will be pre-filled with a useful message

Co-authored-by: Dhruv Bodani <dhruvbodani2510@gmail.com>
@kobelb kobelb added the needs-team Issues missing a team label label Jan 31, 2022
@botelastic botelastic bot removed the needs-team Issues missing a team label label Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting good first issue low hanging fruit Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
None yet
5 participants