[Actionable Observability] save alerts config in different localStorage key for overview and ale… #130538

mgiota · 2022-04-19T09:36:06Z

…rts page

fkanout · 2022-04-21T08:26:38Z

...ins/observability/public/pages/alerts/containers/alerts_table_t_grid/alerts_table_t_grid.tsx

@@ -311,7 +310,7 @@ export function AlertsTableTGrid(props: AlertsTableTGridProps) {

  const [flyoutAlert, setFlyoutAlert] = useState<TopAlert | undefined>(undefined);
  const [tGridState, setTGridState] = useState<Partial<TGridModel> | null>(
-    JSON.parse(localStorage.getItem(ALERT_TABLE_STATE_STORAGE_KEY) ?? 'null')
+    JSON.parse(localStorage.getItem(stateStorageKey) ?? 'null')


Isn't unsafe to parse JSON without a try/catch block? I believe there is a helper/util in Kibana that handles that. /src/plugins/kibana_utils/public/storage/storage.ts

Ok let me check it

@fkanout I made a quick search in the whole project for imports of kibana_utils/public/storage, but I didn't find anything. So I was wondering how I should properly use Storage from this file. My guess is that it should be part of a kbn pacakge? Do you have any idea? I can simply try to import this file, but I didn't find any other imports of it in the whole project, that's why I am wondering if there is a proper way to do it.

Ok I found something. Here's how I can import it import { IStorage, Storage } from '@kbn/kibana-utils-plugin/public';. This is a very recent change from kibana-operations team on how we can import stuff. This change would require a bit more refactoring. Should it be part of this PR?

@fkanout In the end I did the refactoring as part of this PR d12eeea.

@estermv Since I had to change one of your files as part of this refactoring, let me know if everything looks fine

fkanout · 2022-04-21T08:34:19Z

x-pack/plugins/observability/public/pages/overview/old_overview_page.tsx

@@ -195,6 +195,7 @@ export function OverviewPage({ routeParams }: Props) {
                    rangeFrom={relativeStart}
                    rangeTo={relativeEnd}
                    indexNames={indexNames}
+                    stateStorageKey="xpack.observability.overview.alert.tableState"


Isn't better to share this hardcoded value in a commonplace then be used here and there https://github.com/elastic/kibana/pull/130538/files#diff-ab4cbe22bf9a1c9bbaf3a3cb32b5aa5b0c8b8e33c34b88ed3c99dbde6fedadb9R333

The values are different (unless they are used somewhere else that I missed) so I wouldn't put them in a commonplace until they are duplicated somewhere. The only thing I would suggest is to put that in a constant at the top of the file.

@fkanout Thanks for the feedback. That's the whole point of this fix. Let me explain more. Before my fix the localStorage key was shared between overview and alerts page (xpack.observability.alert.tableState) and this was causing the alerts table columns bug explained in this issue.

With my current change I introduced two different localStorage keys xpack.observability.alert.tableState and xpack.observability.overview.alert.tableState for alerts page and overview page accordingly.

@estermv I can put the keys in a constant at the top of the files.

fkanout · 2022-04-21T08:39:53Z

...ins/observability/public/pages/alerts/containers/alerts_table_t_grid/alerts_table_t_grid.tsx

-      if (newState !== localStorage.getItem(ALERT_TABLE_STATE_STORAGE_KEY)) {
-        localStorage.setItem(ALERT_TABLE_STATE_STORAGE_KEY, newState);
+      if (newState !== localStorage.getItem(stateStorageKey)) {
+        localStorage.setItem(stateStorageKey, newState);


Could you help me understand why we keep altering the localStorage value for the alert table? Like this aren't we changing the o11y alerts table settings to match the other TGrid settings which we don't want?

@fkanout Sure I'll try to explain. Short answer is we keep altering the localStorage value to reflect the user UI changes (for example visible columns or sorting column).

The only change I did here was to:

not use the common ALERT_TABLE_STATE_STORAGE_KEY that was used in both Overview and Alerts page and was causing this bug Alerts table will reset fields to a different table config that the default #126765,

but use the stateStorageKey which is a new prop I introduced to the AlertsTableTGrid component

So now the two consumers alerts page and old overview page pass the prop to the AlertsTableTGrid component. and a different localStorage key is used for the two pages to keep track of the user specified settings. Nothing else changed in the previous logic, just the parameterization of the localStorage key.

Regarding the existing localStorage functionality (which didn't change in this PR) it works in a way that when the tGridState is changed, it will update the localStorage to the new user settings, if they are of course different from what is currently saved in that localStorage key. tGridState will be changed when any of the following is changed onStateChange (columns, sort, selectedEventIds). You can play around a bit with the UI and change table columns or sorting for example and see what is saved in localStorage.

Does it make sense?

mgiota · 2022-04-21T09:45:07Z

@elasticmachine merge upstream

mgiota · 2022-04-25T07:55:06Z

@elasticmachine merge upstream

kibana-ci · 2022-04-25T09:09:19Z

💚 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
`observability`	427.3KB	427.4KB	+81.0B

History

💚 Build #39794 succeeded d12eeea
💚 Build #39756 succeeded f431476
💚 Build #38979 succeeded 779f3e3

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

cc @mgiota

fkanout

LGTM

…ge key for overview and ale… (elastic#130538) * save alerts config in different localStorage key for overview and alerts page * remove ALERT_TABLE_STATE_STORAGE_KEY

save alerts config in different localStorage key for overview and ale…

5d84e60

…rts page

mgiota requested review from a team as code owners April 19, 2022 09:36

mgiota self-assigned this Apr 19, 2022

mgiota added Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.3.0 release_note:skip Skip the PR/issue when compiling release notes labels Apr 19, 2022

remove ALERT_TABLE_STATE_STORAGE_KEY

779f3e3

mgiota enabled auto-merge (squash) April 19, 2022 12:56

estermv approved these changes Apr 20, 2022

View reviewed changes

fkanout requested changes Apr 21, 2022

View reviewed changes

kibanamachine and others added 2 commits April 21, 2022 04:45

Merge branch 'main' into 126765_reset_fields_fix

02e7f98

use consts for localStorage key

f431476

mgiota requested a review from fkanout April 21, 2022 11:02

use Storage from kibana-utils plugin

d12eeea

Merge branch 'main' into 126765_reset_fields_fix

159bd05

fkanout approved these changes Apr 25, 2022

View reviewed changes

mgiota merged commit 20bba8d into elastic:main Apr 25, 2022

kibanamachine added the backport:skip This commit does not require backporting label Apr 25, 2022

mgiota mentioned this pull request Apr 28, 2022

[Actions Triggers UI] Register initial alerts table configuration reg… #130883

Merged

mgiota deleted the 126765_reset_fields_fix branch May 10, 2022 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Actionable Observability] save alerts config in different localStorage key for overview and ale… #130538

[Actionable Observability] save alerts config in different localStorage key for overview and ale… #130538

mgiota commented Apr 19, 2022

fkanout Apr 21, 2022

mgiota Apr 21, 2022

mgiota Apr 21, 2022 •

edited

Loading

mgiota Apr 21, 2022 •

edited

Loading

mgiota Apr 21, 2022 •

edited

Loading

fkanout Apr 21, 2022

estermv Apr 21, 2022

mgiota Apr 21, 2022

fkanout Apr 21, 2022

mgiota Apr 21, 2022 •

edited

Loading

mgiota commented Apr 21, 2022

mgiota commented Apr 25, 2022

kibana-ci commented Apr 25, 2022

fkanout left a comment

[Actionable Observability] save alerts config in different localStorage key for overview and ale… #130538

[Actionable Observability] save alerts config in different localStorage key for overview and ale… #130538

Conversation

mgiota commented Apr 19, 2022

fkanout Apr 21, 2022

Choose a reason for hiding this comment

mgiota Apr 21, 2022

Choose a reason for hiding this comment

mgiota Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

mgiota Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

mgiota Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

fkanout Apr 21, 2022

Choose a reason for hiding this comment

estermv Apr 21, 2022

Choose a reason for hiding this comment

mgiota Apr 21, 2022

Choose a reason for hiding this comment

fkanout Apr 21, 2022

Choose a reason for hiding this comment

mgiota Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

mgiota commented Apr 21, 2022

mgiota commented Apr 25, 2022

kibana-ci commented Apr 25, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History

fkanout left a comment

Choose a reason for hiding this comment

mgiota Apr 21, 2022 •

edited

Loading

mgiota Apr 21, 2022 •

edited

Loading

mgiota Apr 21, 2022 •

edited

Loading

mgiota Apr 21, 2022 •

edited

Loading