Skip to content

Don't render exceptions flyout if data is loading#181588

Merged
nkhristinin merged 5 commits intoelastic:mainfrom
nkhristinin:exceptions-close-alerts
May 1, 2024
Merged

Don't render exceptions flyout if data is loading#181588
nkhristinin merged 5 commits intoelastic:mainfrom
nkhristinin:exceptions-close-alerts

Conversation

@nkhristinin
Copy link
Copy Markdown
Contributor

@nkhristinin nkhristinin commented Apr 24, 2024

Exceptions not created for mappings with conflicts

related: #177007

How to reproduce

  • Execute request in order, and change timestamps to you actual dates
PUT test-1
{
  "mappings": {
    "properties": {
      "@timestamp": {"type": "date"},
      "host.name": { "type": "keyword" }
    }
  }
}

PUT auditbeat-1
{
  "mappings": {
    "properties": {
      "@timestamp": {"type": "date"},
      "host.name": { "type": "boolean" }
    }
  }
}

POST auditbeat-1/_doc
{
  "@timestamp": 1714121723789,
  "host.name":"host 11",
  "user.name": "123"
}

POST test-1/_doc
{
  "@timestamp": 1714121723789,
  "host.name":"host 11",
  "user.name": "123"
}

  • Create a query rule, with index pattern test-1 and the query: '*'. Wait for the alert to be generated
  • Go to the Alerts page (not from the Rule, but all alerts)
  • Click add exception from the alert and observe that it says that fields have conflict about auditbeat index. Which is incorrect as a rule has only test-1 index pattern.
  • Check all checkboxes about closing alerts and creating exceptions. See that there is no success toast, and the rule also has no exception.
  • Open flyout exception again. Observe that there is no more message about conflicts. If you create an exception it will be successful.
Screen.Recording.2024-04-26.at.11.33.41.mov

Reason

In the AddExceptionFlyout we do have this line

 const { isLoading, indexPatterns, getExtendedFields } = useFetchIndexPatterns(rules);

From AlertContextMenuComponent we are passing try to load the rule info and first past undefined to rules and then some real info.

The problem is that in the useFetchIndexPatterns - if rules not defined, we return getExtendedFields related to the default data view, but no there rule index pattern.

And probably there some race conditions happen, and after we pass rules props, exceptions don't rerender the info.

As a fix for this BC I just wait for rule info to be loaded, and then render exception flyout, but it only masks the problem.

As we want to fix this bug faster, it's probably ok to merge it like that. And there will be an issue to address this later.

@nkhristinin nkhristinin added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor labels Apr 26, 2024
@nkhristinin nkhristinin marked this pull request as ready for review April 26, 2024 09:46
@nkhristinin nkhristinin requested review from a team as code owners April 26, 2024 09:46
@nkhristinin nkhristinin requested a review from maximpn April 26, 2024 09:46
@nkhristinin
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

enrichedAlert == null ||
(memoRuleIndices == null && memoDataViewId == null);

if (isLoading || isRuleLoading) return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wonder if it possible to display here any kind of loader? So if it takes longer time to load users might think nothing is happening

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ideally, the loader should be in the flyout, but we hide the flyout to fix this bug.

@nkhristinin
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

We discussed the changes over Zoom and, as an interim solution, it looks okay since the bug has a high impact. However, a more robust solution would involve rewriting the data fetching hooks to prevent issues related to concurrent use effects.

Copy link
Copy Markdown
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

@xcrzx thanks for the comment here: #181588 . Don't want to block this, but can we also open up an issue for fixing the concurrency issue?

}
: undefined
);
return useFetchRuleByIdQuery(id, fetchRuleOptions);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return useFetchRuleByIdQuery(id, fetchRuleOptions);
return useFetchRuleByIdQuery(
id,
showToast
? {
...options,
onError: (error) => {
addError(error, { title: i18n.RULE_AND_TIMELINE_FETCH_FAILURE });
},
}
: options
);

@nkhristinin
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 13.7MB 13.7MB +59.0B

History

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

@nkhristinin nkhristinin merged commit 4aabce5 into elastic:main May 1, 2024
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 1, 2024
## Exceptions not created for mappings with conflicts

related: elastic#177007

How to reproduce

- Execute request in order, and change timestamps to you actual dates
```
PUT test-1
{
  "mappings": {
    "properties": {
      "@timestamp": {"type": "date"},
      "host.name": { "type": "keyword" }
    }
  }
}

PUT auditbeat-1
{
  "mappings": {
    "properties": {
      "@timestamp": {"type": "date"},
      "host.name": { "type": "boolean" }
    }
  }
}

POST auditbeat-1/_doc
{
  "@timestamp": 1714121723789,
  "host.name":"host 11",
  "user.name": "123"
}

POST test-1/_doc
{
  "@timestamp": 1714121723789,
  "host.name":"host 11",
  "user.name": "123"
}

```

- Create a query rule, with index pattern `test-1` and the query: '*'.
Wait for the alert to be generated
- Go to the Alerts page (not from the Rule, but all alerts)
- Click add exception from the alert and observe that it says that
fields have conflict about auditbeat index. Which is incorrect as a rule
has only `test-1` index pattern.
- Check all checkboxes about closing alerts and creating exceptions. See
that there is no success toast, and the rule also has no exception.
- Open flyout exception again. Observe that there is no more message
about conflicts. If you create an exception it will be successful.

https://github.com/elastic/kibana/assets/7609147/d29981f7-5d6f-41b2-af46-2ade884e3563

### Reason

In the `AddExceptionFlyout` we do have this line

```
 const { isLoading, indexPatterns, getExtendedFields } = useFetchIndexPatterns(rules);
```

From `AlertContextMenuComponent` we are passing try to load the rule
info and first past `undefined` to rules and then some real info.

The problem is that in the `useFetchIndexPatterns` - if rules not
defined, we return `getExtendedFields` related to the default data view,
but no there rule index pattern.

And probably there some race conditions happen, and after we pass rules
props, exceptions don't rerender the info.

As a fix for this BC I just wait for rule info to be loaded, and then
render exception flyout, but it only masks the problem.

As we want to fix this bug faster, it's probably ok to merge it like
that. And there will be an issue to address this later.

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 4aabce5)
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 All backports created successfully

Status Branch Result
8.14

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

TinLe added a commit to TinLe/kibana that referenced this pull request May 1, 2024
* master: (1654 commits)
  Bump ejs from 3.1.9 to 3.1.10
  Don't render exceptions flyout if data is loading (elastic#181588)
  Enable value list modal (elastic#181593)
  skip flaky suite (elastic#181777)
  skip failing test suite (elastic#182263)
  [Mappings Editor] Disable _source field in serverless (elastic#181712)
  [data.search] Fix unhandled promise rejections (elastic#181785)
  [Fleet] Fix logic for detecting first time Elastic Agent users (elastic#182214)
  [ML] Decouple data_visualizer from MapEmbeddable (elastic#181928)
  [ES|QL] Sorting accepts expressions (elastic#181916)
  [ML] Single Metric Viewer: ensures chart displays correctly when opening from a job annotation (elastic#182176)
  Adding optional Description field to Roles APIs (elastic#182039)
  Upgrade Markdown-it to 14.1.0 (elastic#182244)
  Bump xml-crypto from 5.0.0 to 6.0.0
  [DOCS] Fix docs and screenshots for rule creation changes (elastic#181925)
  Update dependency elastic-apm-node to ^4.5.3 (main) (elastic#182236)
  [Obs AI Assistant] register alert details context in observability plugin (elastic#181501)
  Add `@typescript-eslint/no-floating-promises` (elastic#181456)
  [Playground] Propagate Error message into FE (elastic#182201)
  [ES|QL] Rename the setting to a more generic one and move to the general section (elastic#182074)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 2, 2024
@kibanamachine
Copy link
Copy Markdown
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine added a commit that referenced this pull request May 3, 2024
… (#182286)

# Backport

This will backport the following commits from `main` to `8.14`:
- [Don&#x27;t render exceptions flyout if data is loading
(#181588)](#181588)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Khristinin
Nikita","email":"nikita.khristinin@elastic.co"},"sourceCommit":{"committedDate":"2024-05-01T18:58:38Z","message":"Don't
render exceptions flyout if data is loading (#181588)\n\n## Exceptions
not created for mappings with conflicts\r\n\r\nrelated:
https://github.com/elastic/kibana/pull/177007\r\n\r\n\r\nHow to
reproduce\r\n\r\n- Execute request in order, and change timestamps to
you actual dates\r\n```\r\nPUT test-1\r\n{\r\n \"mappings\": {\r\n
\"properties\": {\r\n \"@timestamp\": {\"type\": \"date\"},\r\n
\"host.name\": { \"type\": \"keyword\" }\r\n }\r\n }\r\n}\r\n\r\nPUT
auditbeat-1\r\n{\r\n \"mappings\": {\r\n \"properties\": {\r\n
\"@timestamp\": {\"type\": \"date\"},\r\n \"host.name\": { \"type\":
\"boolean\" }\r\n }\r\n }\r\n}\r\n\r\nPOST auditbeat-1/_doc\r\n{\r\n
\"@timestamp\": 1714121723789,\r\n \"host.name\":\"host 11\",\r\n
\"user.name\": \"123\"\r\n}\r\n\r\nPOST test-1/_doc\r\n{\r\n
\"@timestamp\": 1714121723789,\r\n \"host.name\":\"host 11\",\r\n
\"user.name\": \"123\"\r\n}\r\n\r\n```\r\n\r\n- Create a query rule,
with index pattern `test-1` and the query: '*'.\r\nWait for the alert to
be generated\r\n- Go to the Alerts page (not from the Rule, but all
alerts)\r\n- Click add exception from the alert and observe that it says
that\r\nfields have conflict about auditbeat index. Which is incorrect
as a rule\r\nhas only `test-1` index pattern.\r\n- Check all checkboxes
about closing alerts and creating exceptions. See\r\nthat there is no
success toast, and the rule also has no exception.\r\n- Open flyout
exception again. Observe that there is no more message\r\nabout
conflicts. If you create an exception it will be
successful.\r\n\r\n\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/7609147/d29981f7-5d6f-41b2-af46-2ade884e3563\r\n\r\n\r\n###
Reason\r\n\r\nIn the `AddExceptionFlyout` we do have this
line\r\n\r\n```\r\n const { isLoading, indexPatterns, getExtendedFields
} = useFetchIndexPatterns(rules);\r\n```\r\n\r\nFrom
`AlertContextMenuComponent` we are passing try to load the rule\r\ninfo
and first past `undefined` to rules and then some real info.\r\n\r\nThe
problem is that in the `useFetchIndexPatterns` - if rules
not\r\ndefined, we return `getExtendedFields` related to the default
data view,\r\nbut no there rule index pattern.\r\n\r\nAnd probably there
some race conditions happen, and after we pass rules\r\nprops,
exceptions don't rerender the info.\r\n\r\nAs a fix for this BC I just
wait for rule info to be loaded, and then\r\nrender exception flyout,
but it only masks the problem.\r\n\r\nAs we want to fix this bug faster,
it's probably ok to merge it like\r\nthat. And there will be an issue to
address this later.\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana
Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"4aabce5bcd9aa625c85af3f7dde9bc3b675d5344","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:prev-minor","v8.15.0"],"title":"Don't
render exceptions flyout if data is
loading","number":181588,"url":"https://github.com/elastic/kibana/pull/181588","mergeCommit":{"message":"Don't
render exceptions flyout if data is loading (#181588)\n\n## Exceptions
not created for mappings with conflicts\r\n\r\nrelated:
https://github.com/elastic/kibana/pull/177007\r\n\r\n\r\nHow to
reproduce\r\n\r\n- Execute request in order, and change timestamps to
you actual dates\r\n```\r\nPUT test-1\r\n{\r\n \"mappings\": {\r\n
\"properties\": {\r\n \"@timestamp\": {\"type\": \"date\"},\r\n
\"host.name\": { \"type\": \"keyword\" }\r\n }\r\n }\r\n}\r\n\r\nPUT
auditbeat-1\r\n{\r\n \"mappings\": {\r\n \"properties\": {\r\n
\"@timestamp\": {\"type\": \"date\"},\r\n \"host.name\": { \"type\":
\"boolean\" }\r\n }\r\n }\r\n}\r\n\r\nPOST auditbeat-1/_doc\r\n{\r\n
\"@timestamp\": 1714121723789,\r\n \"host.name\":\"host 11\",\r\n
\"user.name\": \"123\"\r\n}\r\n\r\nPOST test-1/_doc\r\n{\r\n
\"@timestamp\": 1714121723789,\r\n \"host.name\":\"host 11\",\r\n
\"user.name\": \"123\"\r\n}\r\n\r\n```\r\n\r\n- Create a query rule,
with index pattern `test-1` and the query: '*'.\r\nWait for the alert to
be generated\r\n- Go to the Alerts page (not from the Rule, but all
alerts)\r\n- Click add exception from the alert and observe that it says
that\r\nfields have conflict about auditbeat index. Which is incorrect
as a rule\r\nhas only `test-1` index pattern.\r\n- Check all checkboxes
about closing alerts and creating exceptions. See\r\nthat there is no
success toast, and the rule also has no exception.\r\n- Open flyout
exception again. Observe that there is no more message\r\nabout
conflicts. If you create an exception it will be
successful.\r\n\r\n\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/7609147/d29981f7-5d6f-41b2-af46-2ade884e3563\r\n\r\n\r\n###
Reason\r\n\r\nIn the `AddExceptionFlyout` we do have this
line\r\n\r\n```\r\n const { isLoading, indexPatterns, getExtendedFields
} = useFetchIndexPatterns(rules);\r\n```\r\n\r\nFrom
`AlertContextMenuComponent` we are passing try to load the rule\r\ninfo
and first past `undefined` to rules and then some real info.\r\n\r\nThe
problem is that in the `useFetchIndexPatterns` - if rules
not\r\ndefined, we return `getExtendedFields` related to the default
data view,\r\nbut no there rule index pattern.\r\n\r\nAnd probably there
some race conditions happen, and after we pass rules\r\nprops,
exceptions don't rerender the info.\r\n\r\nAs a fix for this BC I just
wait for rule info to be loaded, and then\r\nrender exception flyout,
but it only masks the problem.\r\n\r\nAs we want to fix this bug faster,
it's probably ok to merge it like\r\nthat. And there will be an issue to
address this later.\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana
Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"4aabce5bcd9aa625c85af3f7dde9bc3b675d5344"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/181588","number":181588,"mergeCommit":{"message":"Don't
render exceptions flyout if data is loading (#181588)\n\n## Exceptions
not created for mappings with conflicts\r\n\r\nrelated:
https://github.com/elastic/kibana/pull/177007\r\n\r\n\r\nHow to
reproduce\r\n\r\n- Execute request in order, and change timestamps to
you actual dates\r\n```\r\nPUT test-1\r\n{\r\n \"mappings\": {\r\n
\"properties\": {\r\n \"@timestamp\": {\"type\": \"date\"},\r\n
\"host.name\": { \"type\": \"keyword\" }\r\n }\r\n }\r\n}\r\n\r\nPUT
auditbeat-1\r\n{\r\n \"mappings\": {\r\n \"properties\": {\r\n
\"@timestamp\": {\"type\": \"date\"},\r\n \"host.name\": { \"type\":
\"boolean\" }\r\n }\r\n }\r\n}\r\n\r\nPOST auditbeat-1/_doc\r\n{\r\n
\"@timestamp\": 1714121723789,\r\n \"host.name\":\"host 11\",\r\n
\"user.name\": \"123\"\r\n}\r\n\r\nPOST test-1/_doc\r\n{\r\n
\"@timestamp\": 1714121723789,\r\n \"host.name\":\"host 11\",\r\n
\"user.name\": \"123\"\r\n}\r\n\r\n```\r\n\r\n- Create a query rule,
with index pattern `test-1` and the query: '*'.\r\nWait for the alert to
be generated\r\n- Go to the Alerts page (not from the Rule, but all
alerts)\r\n- Click add exception from the alert and observe that it says
that\r\nfields have conflict about auditbeat index. Which is incorrect
as a rule\r\nhas only `test-1` index pattern.\r\n- Check all checkboxes
about closing alerts and creating exceptions. See\r\nthat there is no
success toast, and the rule also has no exception.\r\n- Open flyout
exception again. Observe that there is no more message\r\nabout
conflicts. If you create an exception it will be
successful.\r\n\r\n\r\n\r\n\r\nhttps://github.com/elastic/kibana/assets/7609147/d29981f7-5d6f-41b2-af46-2ade884e3563\r\n\r\n\r\n###
Reason\r\n\r\nIn the `AddExceptionFlyout` we do have this
line\r\n\r\n```\r\n const { isLoading, indexPatterns, getExtendedFields
} = useFetchIndexPatterns(rules);\r\n```\r\n\r\nFrom
`AlertContextMenuComponent` we are passing try to load the rule\r\ninfo
and first past `undefined` to rules and then some real info.\r\n\r\nThe
problem is that in the `useFetchIndexPatterns` - if rules
not\r\ndefined, we return `getExtendedFields` related to the default
data view,\r\nbut no there rule index pattern.\r\n\r\nAnd probably there
some race conditions happen, and after we pass rules\r\nprops,
exceptions don't rerender the info.\r\n\r\nAs a fix for this BC I just
wait for rule info to be loaded, and then\r\nrender exception flyout,
but it only masks the problem.\r\n\r\nAs we want to fix this bug faster,
it's probably ok to merge it like\r\nthat. And there will be an issue to
address this later.\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana
Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"4aabce5bcd9aa625c85af3f7dde9bc3b675d5344"}}]}]
BACKPORT-->

Co-authored-by: Khristinin Nikita <nikita.khristinin@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Skip the PR/issue when compiling release notes v8.14.0 v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants