[ResponseOps][Rules] Adding rule.url variable #145035

jonathan-buttner · 2022-11-10T21:45:39Z

This PR adds a new actions variable for linking back the stack management rule page. In a future PR we will require the rule type to specify the plugin's path when registering the rule type that way we can link back to the specific plugin that created the rule.

Issue: #145132

Mustache variable

Constructed URL

elasticmachine · 2022-11-14T16:23:34Z

Pinging @elastic/response-ops (Team:ResponseOps)

pmuellr

LGTM; made a comment about a comment and suggestion to add a debug log ...

pmuellr · 2022-11-14T21:12:12Z

x-pack/plugins/alerting/server/task_runner/execution_handler.test.ts

+      `);
+    });
+
+    it('does not populate the rule.url when the alert id is set to undefined', async () => {


Suggested change

it('does not populate the rule.url when the alert id is set to undefined', async () => {

it('does not populate the rule.url when the rule id is set to undefined', async () => {

Just to make it a little clearer :-)

pmuellr · 2022-11-14T21:13:55Z

x-pack/plugins/alerting/server/task_runner/execution_handler.test.ts

+      `);
+    });
+
+    it('does not populate the rule.url when the alert id is a number', async () => {


I'm curious why we're even handling this case. Not something I think we usually test, so curious ...

Yeah the reason I thought this was worth covering is because the way we've typed the task manager params is Record<string, any>. So when we destructure taskInstance.params it's showing alertId: ruleId as type any. So I figured it's probably worth skipping the rule building if the type is anything but a string 🤷‍♂️

Ah! Looking closer, I missed the buildRuleUrl() method is referencing this.taskInstance.params.alertId - I missed that before, and that does seem a little brittle. Looking at it's call site, it looks like you could instead pass ruleId as a parameter.

Yeah I can pass it as a parameter, but just to make sure I'm understanding, that doesn't address it being typed as any right? I still need to ensure it is a string.

Ah, good catch - ya, that variable is extracted from the same place 🤦🏻‍♂️ .

However, I noticed a this.rule available, so I think you can leave it parameterless and change

this.taskInstance.params.alertId -> this.rule.id

pmuellr · 2022-11-14T21:17:43Z

x-pack/plugins/alerting/server/task_runner/execution_handler.ts

+
+      return ruleUrl.toString();
+    } catch (error) {
+      return;


feels worthy of a log.debug(), just in case this case gets caught, but we're confused why the url didn't get set

pmuellr

LGTM; made a comment about a comment and suggestion to add a debug log ...

Also a comment about using the getRuleDetailsRoute() somewhere it's not currently being used. I'm sure this would end up needing to be fixed when this code gets more plugin-specific, so doesn't seem like a biggie.

pmuellr · 2022-11-14T21:48:50Z

...plugins/triggers_actions_ui/public/application/sections/rules_list/components/rules_list.tsx

@@ -886,7 +887,7 @@ export const RulesList = ({
        onPage={setPage}
        onRuleChanged={() => refreshRules()}
        onRuleClick={(rule) => {
-          const detailsRoute = ruleDetailsRoute ? ruleDetailsRoute : routeToRuleDetails;
+          const detailsRoute = ruleDetailsRoute ? ruleDetailsRoute : commonRuleDetailsRoute;
          history.push(detailsRoute.replace(`:ruleId`, rule.id));


Is there some reason we don't use getRuleDetailsRoute(rule.id) here, as was done in rule_details.tsx above?

The ruleDetailsRoute is passed in as a prop to this component and it wasn't clear to me what the value would be. So I don't think we can rely on it equaling commonRuleDetailsRoute aka '/rule/:ruleId'. So if we called getRuleDetailsRoute here it could change the initial part of the path (/rule may not exist in the ruleDetailsRoute passed in as a prop).

Another option is that we change getRuleDetailsRoute to take the path portion but that's pretty close to just calling .replace directly 🤷‍♂️

thx

Feels like this is going to change with the rule-type-specific route stuff in a subsequent PR anyway, so feels like we should just leave it alone for now :-)

ok sounds good!

benakansara

AO changes LGTM.

jonathan-buttner · 2022-11-15T20:00:53Z

When I run this PR with an empty ES (no rules) and go to Stack Management, I see this error:

with a network error for failing to resolve https://localhost:5601/internal/alerting/rule/undefined/_resolve. I did not see this on main.

Following up as we've continued this discussion offline. Christos and I tried to reproduce this and we weren't able to. Ying also checked on the cloud deployment and wasn't seeing it there. Seems like this is a race condition somewhere.

kibana-ci · 2022-11-15T21:55:29Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1323	1324	+1
`cases`	511	512	+1
`infra`	1146	1147	+1
`observability`	431	432	+1
`securitySolution`	3294	3295	+1
`synthetics`	1076	1077	+1
`timelines`	191	192	+1
`triggersActionsUi`	461	462	+1
total			+8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/rule-data-utils`	83	87	+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`triggersActionsUi`	663.6KB	655.5KB	-8.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`triggersActionsUi`	104.0KB	108.1KB	+4.1KB

Unknown metric groups

API count

id	before	after	diff
`@kbn/rule-data-utils`	86	90	+4

async chunk count

id	before	after	diff
`triggersActionsUi`	53	52	-1

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	59	65	+6
`osquery`	108	113	+5
`securitySolution`	441	447	+6
total			+19

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	67	73	+6
`osquery`	109	115	+6
`securitySolution`	518	524	+6
total			+20

History

💔 Build #88223 failed b0a1c95
💚 Build #88169 succeeded d2d1394
💔 Build #88144 failed ae4a93e
💚 Build #87837 succeeded c34a153
💔 Build #87790 failed 5425543

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

Working rule.url need to write tests

52d78c0

jonathan-buttner added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework v8.6.0 labels Nov 10, 2022

jonathan-buttner added 2 commits November 14, 2022 11:13

Adding tests for rule.url

86c6000

Merge branch 'main' of github.com:elastic/kibana into rule-url-variable

f42ffd8

jonathan-buttner mentioned this pull request Nov 14, 2022

[ResponseOps][Actions] Add mustache variable to link back to the rule that created the alert #145132

Closed

2 tasks

jonathan-buttner marked this pull request as ready for review November 14, 2022 16:23

jonathan-buttner requested review from a team as code owners November 14, 2022 16:23

jonathan-buttner and others added 3 commits November 14, 2022 12:58

Fixing tests

d7edbb8

Merge branch 'main' into rule-url-variable

3dd3236

Merge branch 'main' into rule-url-variable

b51f3f5

pmuellr approved these changes Nov 14, 2022

View reviewed changes

jonathan-buttner mentioned this pull request Nov 14, 2022

[ResponseOps][Actions] Add rule callback for each rule type #145190

Closed

jonathan-buttner and others added 2 commits November 14, 2022 16:59

Addressing feedback

5425543

Merge branch 'main' into rule-url-variable

c34a153

benakansara approved these changes Nov 15, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

jonathan-buttner added the ci:cloud-deploy Create or update a Cloud deployment label Nov 15, 2022

jonathan-buttner added 2 commits November 15, 2022 12:16

Removing unneeded tests

ae4a93e

Merge branch 'main' of github.com:elastic/kibana into rule-url-variable

d2d1394

spaces are back

b0a1c95

XavierM approved these changes Nov 15, 2022

View reviewed changes

jonathan-buttner enabled auto-merge (squash) November 15, 2022 20:58

jonathan-buttner mentioned this pull request Nov 15, 2022

[ResponseOps][Connectors] Opsgenie Connector #142776

Closed

10 tasks

Fixing tests

bccf5ae

jonathan-buttner merged commit 2180a8a into elastic:main Nov 15, 2022

kibanamachine added the backport:skip This commit does not require backporting label Nov 15, 2022

jonathan-buttner deleted the rule-url-variable branch November 16, 2022 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResponseOps][Rules] Adding rule.url variable #145035

[ResponseOps][Rules] Adding rule.url variable #145035

jonathan-buttner commented Nov 10, 2022 •

edited

Loading

elasticmachine commented Nov 14, 2022

pmuellr left a comment

pmuellr Nov 14, 2022

pmuellr Nov 14, 2022

jonathan-buttner Nov 14, 2022

pmuellr Nov 14, 2022

jonathan-buttner Nov 15, 2022

pmuellr Nov 15, 2022

pmuellr Nov 14, 2022

pmuellr left a comment

pmuellr Nov 14, 2022

jonathan-buttner Nov 15, 2022

pmuellr Nov 15, 2022

jonathan-buttner Nov 15, 2022

benakansara left a comment

This comment was marked as resolved.

jonathan-buttner commented Nov 15, 2022

kibana-ci commented Nov 15, 2022 •

edited

Loading

API count

async chunk count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

	it('does not populate the rule.url when the alert id is set to undefined', async () => {
	it('does not populate the rule.url when the rule id is set to undefined', async () => {

[ResponseOps][Rules] Adding rule.url variable #145035

[ResponseOps][Rules] Adding rule.url variable #145035

Conversation

jonathan-buttner commented Nov 10, 2022 • edited Loading

elasticmachine commented Nov 14, 2022

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benakansara left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

jonathan-buttner commented Nov 15, 2022

kibana-ci commented Nov 15, 2022 • edited Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

async chunk count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

jonathan-buttner commented Nov 10, 2022 •

edited

Loading

kibana-ci commented Nov 15, 2022 •

edited

Loading