[APM]Fix manual refresh and auto refresh issue #157472

achyutjhunjhunwala · 2023-05-12T10:43:14Z

Fixes - #156363

Depends on - #157468

Summary

The PR fixes the issues

Where when manually Refresh button on the Unified Search Bar is clicked, the refresh would do nothing.
When setting refresh interval and enabling automatic refresh, the page won't refresh respecting the settings.
When refresh interval was set and enabled, now when the whole page was refreshed, these settings would be lost as these settings were not originally synced to the URL

elasticmachine · 2023-05-12T10:43:17Z

Pinging @elastic/apm-ui (Team:APM)

apmmachine · 2023-05-12T10:43:29Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

x-pack/plugins/apm/public/components/shared/date_picker/index.tsx

dgieselaar · 2023-05-15T10:31:19Z

x-pack/plugins/apm/public/components/shared/unified_search_bar/index.tsx

+  const refreshIntervalFromUrl =
+    'refreshInterval' in query
+      ? toNumber(query.refreshInterval)
+      : DEFAULT_REFRESH_INTERVAL;
+  const refreshPausedFromUrl =
+    'refreshPaused' in query ? query.refreshPaused : 'true';


I think these should be retrieved from advanced settings. @sqren?

I'm not a huge fan of this approach tbh - we essentially lose any routing type safety. But out of scope for this PR.

I saw they were retrieved from the Query Bar in the previous implementation of Date Picker, so i replicated them here.

https://github.com/achyutjhunjhunwala/kibana/blob/08925c46ae9241ec260903621cf21a6abe226f4a/x-pack/plugins/apm/public/components/shared/date_picker/apm_date_picker.tsx#L26-L31

kibana/x-pack/plugins/apm/public/components/shared/date_picker/apm_date_picker.tsx

Lines 26 to 31 in 08925c4

const {

rangeFrom,

rangeTo,

refreshPaused: refreshPausedFromUrl = 'true',

refreshInterval: refreshIntervalFromUrl,

} = query;

dgieselaar · 2023-05-15T10:35:39Z

x-pack/plugins/apm/public/components/shared/unified_search_bar/index.tsx

+  useEffectOnce(() => {
+    // Sync Refresh Interval with Search Bar
+    const refreshInterval = {
+      pause: toBoolean(refreshPausedFromUrl),
+      value: refreshIntervalFromUrl,
+    };
+    if (
+      !deepEqual(
+        timeFilterService.timefilter.getRefreshInterval(),
+        refreshInterval
+      )
+    ) {
+      timeFilterService.timefilter.setRefreshInterval(refreshInterval);
+    }
+  });


Why just once? What happens if we navigate using browser history?

is there any way we can use the search bar as a purely controlled component? also realise this is out of scope for this PR but all this syncing back and forth is a bug waiting to happen IMHO

Looks like there were some changes in my local which could have caused some issue. When i was placing it under regular useEffect, the toggle for enabling/disabling would behave as uncontrolled component and would not let me toggle it. But now i don't see that issue anymore. So i will move it back to useEffect.

Regarding making it a controlled component, i can try that in the next PR, as far as i remember, when i started using the props from the component, to update the state from the URL, they were not working. But with fixes done on the Unified Search component, i can give it a try in the next PR

Added a follow up ticket here - #157715

dgieselaar · 2023-05-15T10:46:26Z

x-pack/plugins/apm/public/components/shared/unified_search_bar/index.tsx

+  const onRefresh = () => {
+    clearCache();
+    incrementTimeRangeId();
+  };


was this never implemented in the unified search bar implementation?

Do you mean on the original Unified Search Component from Platform ?
On our side, this was a miss which caused the refresh button to not work

dgieselaar · 2023-05-15T10:51:24Z

x-pack/plugins/apm/public/components/shared/unified_search_bar/index.tsx

+      !deepEqual(
+        timeFilterService.timefilter.getRefreshInterval(),
+        refreshInterval
+      )


why do we need the (deep) equal check? what happens if it's the same value and we call setRefreshInterval anyway? will it cause infinite recursion?

Yup can be removed

dgieselaar

Approved in the interest of time, but eager to see us follow up with some changes that can make this code more robust. Thanks @achyutjhunjhunwala !

kibana-ci · 2023-05-15T14:55:40Z

💚 Build Succeeded

Buildkite Build
Commit: 4c8fd23

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
`apm`	3.5MB	3.5MB	+536.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	400	404	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	480	484	+4
total			+6

History

💔 Build #127794 failed d934c09
💚 Build #127690 succeeded 08925c4
💔 Build #127449 failed db3cdc8
💔 Build #127391 failed 9a70018
💔 Build #127367 failed 434405c

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

cc @achyutjhunjhunwala

Fixes - elastic#156363 Depends on - elastic#157468 ## Summary The PR fixes the issues 1. Where when manually Refresh button on the Unified Search Bar is clicked, the refresh would do nothing. 2. When setting refresh interval and enabling automatic refresh, the page won't refresh respecting the settings. 3. When refresh interval was set and enabled, now when the whole page was refreshed, these settings would be lost as these settings were not originally synced to the URL (cherry picked from commit 12dd688)

kibanamachine · 2023-05-15T15:02:47Z

💚 All backports created successfully

Status	Branch	Result
✅	8.8

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

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.8`: - [[APM]Fix manual refresh and auto refresh issue (#157472)](#157472)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Achyut Jhunjhunwala <achyut.jhunjhunwala@elastic.co>

Fixes - #156363 Depends on - #157468 ## Summary The PR fixes the issues 1. Where when manually Refresh button on the Unified Search Bar is clicked, the refresh would do nothing. 2. When setting refresh interval and enabling automatic refresh, the page won't refresh respecting the settings. 3. When refresh interval was set and enabled, now when the whole page was refreshed, these settings would be lost as these settings were not originally synced to the URL

Fix manual refresh and auto refresh issue

434405c

achyutjhunjhunwala added the Team:APM All issues that need APM UI Team support label May 12, 2023

achyutjhunjhunwala requested a review from a team as a code owner May 12, 2023 10:43

achyutjhunjhunwala self-assigned this May 12, 2023

achyutjhunjhunwala added release_note:skip Skip the PR/issue when compiling release notes v8.8.0 labels May 12, 2023

dgieselaar reviewed May 12, 2023

View reviewed changes

x-pack/plugins/apm/public/components/shared/date_picker/index.tsx Show resolved Hide resolved

achyutjhunjhunwala added 4 commits May 12, 2023 14:09

Merge branch 'main' into fix-unified-search-refresh_issue

9a70018

Merge branch 'main' into fix-unified-search-refresh_issue

db3cdc8

Merge branch 'main' into fix-unified-search-refresh_issue

f449e5d

Use isUpdate instead of manual calculation

08925c4

dgieselaar reviewed May 15, 2023

View reviewed changes

dgieselaar approved these changes May 15, 2023

View reviewed changes

achyutjhunjhunwala added 2 commits May 15, 2023 14:52

Fix review comments by removing useEffectOnce

17c0f0b

Merge branch 'main' into fix-unified-search-refresh_issue

d934c09

achyutjhunjhunwala enabled auto-merge (squash) May 15, 2023 12:54

Merge branch 'main' into fix-unified-search-refresh_issue

4c8fd23

achyutjhunjhunwala merged commit 12dd688 into elastic:main May 15, 2023

achyutjhunjhunwala deleted the fix-unified-search-refresh_issue branch May 15, 2023 14:56

kibanamachine added the v8.9.0 label May 15, 2023

kibanamachine mentioned this pull request May 15, 2023

[8.8] [APM]Fix manual refresh and auto refresh issue (#157472) #157744

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM]Fix manual refresh and auto refresh issue #157472

[APM]Fix manual refresh and auto refresh issue #157472

achyutjhunjhunwala commented May 12, 2023 •

edited by kibanamachine

Loading

elasticmachine commented May 12, 2023

apmmachine commented May 12, 2023

dgieselaar May 15, 2023

dgieselaar May 15, 2023

achyutjhunjhunwala May 15, 2023

dgieselaar May 15, 2023

dgieselaar May 15, 2023

achyutjhunjhunwala May 15, 2023

achyutjhunjhunwala May 15, 2023 •

edited

Loading

achyutjhunjhunwala May 15, 2023

dgieselaar May 15, 2023

achyutjhunjhunwala May 15, 2023

dgieselaar May 15, 2023

achyutjhunjhunwala May 15, 2023

dgieselaar left a comment

kibana-ci commented May 15, 2023

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented May 15, 2023

	const {
	rangeFrom,
	rangeTo,
	refreshPaused: refreshPausedFromUrl = 'true',
	refreshInterval: refreshIntervalFromUrl,
	} = query;

[APM]Fix manual refresh and auto refresh issue #157472

[APM]Fix manual refresh and auto refresh issue #157472

Conversation

achyutjhunjhunwala commented May 12, 2023 • edited by kibanamachine Loading

Summary

elasticmachine commented May 12, 2023

apmmachine commented May 12, 2023

🤖 GitHub comments

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

achyutjhunjhunwala May 15, 2023 • edited Loading

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

dgieselaar left a comment

Choose a reason for hiding this comment

kibana-ci commented May 15, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented May 15, 2023

💚 All backports created successfully

Questions ?

achyutjhunjhunwala commented May 12, 2023 •

edited by kibanamachine

Loading

achyutjhunjhunwala May 15, 2023 •

edited

Loading