[Actionable Observability] Fix alerts' blank page in case of invalid query string #145067

maryam-saeidi · 2022-11-13T22:14:25Z

Implements #143641

📝 Summary

Fixes the alerts page crash when a wrong query is entered in the search bar query string.

Note
I am working on tests but I will create a separate PR for that.

🧪 How to test

Go to alerts / rule details page
Enter an invalid query such as {, page should not crash and you should see a toast with a related error message

…rage key in consumer

maryam-saeidi · 2022-11-13T22:16:30Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/types.ts

  appName: string;
  setEsQuery: (query: { bool: BoolQuery }) => void;
  queries?: Query[];
 }

+export interface AlertSearchBarWithUrlSyncProps extends CommonAlertSearchBarProps {
+  urlStorageKey: string;


💬 Allowing consumers to set the storage key.

maryam-saeidi · 2022-11-13T22:17:08Z

x-pack/plugins/observability/public/components/shared/alert_search_bar/alert_search_bar.tsx

 import { AlertStatus } from '../../../../common/typings';

 const getAlertStatusQuery = (status: string): Query[] => {
  return status ? [{ query: ALERT_STATUS_QUERY[status], language: 'kuery' }] : [];
 };

-export function AlertSearchBar({


💬 Renamed it to make it easier to find the usage of this component.

…-fix'

…am-saeidi/kibana into 143641-fix-alert-search-bar

kibana-ci · 2022-11-14T09:59:51Z

💚 Build Succeeded

Buildkite Build
Commit: f7929aa

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
`observability`	487.5KB	487.8KB	+317.0B

Unknown metric groups

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 #87347 failed 4f84167
💔 Build #87346 failed 726a7ec

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

cc @maryam-saeidi

elasticmachine · 2022-11-14T10:29:23Z

Pinging @elastic/actionable-observability (Team: Actionable Observability)

benakansara · 2022-11-14T11:15:15Z

Can we display user-friendly message in place of stack trace? I don't know if this is the common practice across error messages in Kibana. If that is the case, then it's fine.

maryam-saeidi · 2022-11-14T11:25:38Z

Can we display user-friendly message in place of stack trace? I don't know if this is the common practice across error messages in Kibana. If that is the case, then it's fine.

I used a similar pattern as Kerry used here. The error message is coming from buildEsQuery in @kbn/es-query which is a shared function and when I tested a similar scenario in logs, I got the same error message:

In some other cases, the message is a bit more clear, like this one:

I am not aware of any other alternative. I tried to at least have a good title for this error :D

benakansara · 2022-11-14T11:34:24Z

x-pack/plugins/observability/public/pages/alerts/containers/alerts_page/constants.ts

@@ -9,3 +9,4 @@ export const ALERTS_PAGE_ID = 'alerts-o11y';
 export const ALERTS_SEARCH_BAR_ID = 'alerts-search-bar-o11y';
 export const ALERTS_PER_PAGE = 50;
 export const ALERTS_TABLE_ID = 'xpack.observability.alerts.alert.table';
+export const URL_STORAGE_KEY = '_a';


Is there a reason to keep URL_STORAGE_KEY as _a here and searchBarParams in rule_details/constants.ts? The namings do not match.

I wanted to use a more meaningful name for this storage rather than _a, so I changed it on the rule details page.
But on the alerts page, I didn't change it since I wasn't sure if changing the URL structure will break anything. We can discuss this use-case within our team and I can update it in the next PR if that makes sense.

Sounds good! 👍

benakansara

LGTM. Tested locally, works well.
Left a question above.

CoenWarmer

Looks good!!

Closes #143641 ## Summary As a follow-up to this PR #145067, in this PR I've added tests to make sure the same issue will not happen in the future. ### Checklist - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…5259) Closes elastic#143641 ## Summary As a follow-up to this PR elastic#145067, in this PR I've added tests to make sure the same issue will not happen in the future. ### Checklist - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Validate alert query before saving in state and allow setting url sto…

d032a85

…rage key in consumer

maryam-saeidi self-assigned this Nov 13, 2022

maryam-saeidi added Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" bug Fixes for quality problems that affect the customer experience release_note:fix labels Nov 13, 2022

maryam-saeidi changed the title ~~[Actionable Observability] Validate alert query before saving it in state~~ [Actionable Observability] Fix alerts' blank page in case of invalid query string Nov 13, 2022

maryam-saeidi commented Nov 13, 2022

View reviewed changes

maryam-saeidi and others added 4 commits November 13, 2022 23:18

Merge branch 'main' into 143641-fix-alert-search-bar

726a7ec

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

4f84167

…-fix'

Fix types

59007a7

Merge branch '143641-fix-alert-search-bar' of https://github.com/mary…

f7929aa

…am-saeidi/kibana into 143641-fix-alert-search-bar

maryam-saeidi marked this pull request as ready for review November 14, 2022 10:29

maryam-saeidi requested a review from a team as a code owner November 14, 2022 10:29

benakansara reviewed Nov 14, 2022

View reviewed changes

benakansara approved these changes Nov 14, 2022

View reviewed changes

CoenWarmer approved these changes Nov 14, 2022

View reviewed changes

maryam-saeidi merged commit 2d5709f into elastic:main Nov 14, 2022

kibanamachine added v8.6.0 backport:skip This commit does not require backporting labels Nov 14, 2022

maryam-saeidi mentioned this pull request Nov 15, 2022

[Actionable observability] Add tests for alert search bar #145259

Merged

1 task

maryam-saeidi deleted the 143641-fix-alert-search-bar branch December 19, 2022 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Actionable Observability] Fix alerts' blank page in case of invalid query string #145067

[Actionable Observability] Fix alerts' blank page in case of invalid query string #145067

maryam-saeidi commented Nov 13, 2022 •

edited

Loading

maryam-saeidi Nov 13, 2022

maryam-saeidi Nov 13, 2022

kibana-ci commented Nov 14, 2022

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

elasticmachine commented Nov 14, 2022

benakansara commented Nov 14, 2022 •

edited

Loading

maryam-saeidi commented Nov 14, 2022

benakansara Nov 14, 2022

maryam-saeidi Nov 14, 2022

benakansara Nov 14, 2022

benakansara left a comment •

edited

Loading

CoenWarmer left a comment

[Actionable Observability] Fix alerts' blank page in case of invalid query string #145067

[Actionable Observability] Fix alerts' blank page in case of invalid query string #145067

Conversation

maryam-saeidi commented Nov 13, 2022 • edited Loading

📝 Summary

🧪 How to test

maryam-saeidi Nov 13, 2022

Choose a reason for hiding this comment

maryam-saeidi Nov 13, 2022

Choose a reason for hiding this comment

kibana-ci commented Nov 14, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

elasticmachine commented Nov 14, 2022

benakansara commented Nov 14, 2022 • edited Loading

maryam-saeidi commented Nov 14, 2022

benakansara Nov 14, 2022

Choose a reason for hiding this comment

maryam-saeidi Nov 14, 2022

Choose a reason for hiding this comment

benakansara Nov 14, 2022

Choose a reason for hiding this comment

benakansara left a comment • edited Loading

Choose a reason for hiding this comment

CoenWarmer left a comment

Choose a reason for hiding this comment

maryam-saeidi commented Nov 13, 2022 •

edited

Loading

benakansara commented Nov 14, 2022 •

edited

Loading

benakansara left a comment •

edited

Loading