[APM] Ensure refresh button works by dgieselaar · Pull Request #112652 · elastic/kibana

dgieselaar · 2021-09-21T11:13:28Z

After some routing changes, the refresh button no longer works, due to the DatePicker component and useFetcher using different ways to invalidate the current time range. This PR addresses that issue by:

Separating the date pickers for the APM app and the UX dashboard, as the former uses the new router, and the latter uses URLParamsContext
Create a useDateRangeRedirect which returns a redirect function and a isDateRangeSet property
For the APM date picker, when refresh is clicked, call incrementTimeRangeId from the useTimeRangeId hook, which is also used by useFetcher to refresh data
For the UX dashboard date picker, use refreshTimeRange from useUxUrlParams, and useDateRangeRedirect
wrap the APM app in a RedirectWithDefaultDateRange component, which uses a newly implemented getRoutesToMatch function, that returns the routes that are expected to match, without validating the query parameters. This enables us to intercept invalid routes before they are rendered, and redirect, without rendering child components that expect valid & parsed routing parameters

Closes elastic#110465.

…fresh

dgieselaar · 2021-09-22T09:19:43Z

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

+      return (
+        route.path === '/services' ||
+        route.path === '/traces' ||
+        route.path === '/service-map' ||
+        route.path === '/backends' ||
+        route.path === '/services/{serviceName}' ||
+        location.pathname === '/' ||
+        location.pathname === ''
+      );


This isn't great. Ideally this would be a property of the route, as the route itself is disconnected from setting the default parameters. I've considered some alternatives, but they come at a cost. One issue is that the date range defaults are (AFAIK) only available via a hook, so we have to render a React element to get the appropriate defaults. We can't easily use a middleware layer for that reason (e.g. onRouteActivate). One option would be to allow the route to specify a pre element that gets rendered as an immediate child of RouterProvider, before any other elements, that can block rendering of child routes. That's a little bit of a weird API though, and I think we should wait for more use cases before implementing it.

elasticmachine · 2021-09-22T09:20:13Z

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

justinkambic

UX changes LGTM

elasticmachine · 2021-09-22T17:32:31Z

Pinging @elastic/uptime (Team:uptime)

…fresh

dgieselaar · 2021-09-27T07:16:55Z

@elasticmachine merge upstream

…fresh

…o time-range-refresh

cauemarcondes

a few suggestions

cauemarcondes · 2021-10-04T13:29:42Z

x-pack/plugins/apm/public/components/shared/search_bar.tsx

+  const refreshPaused = refreshPausedFromUrl
+    ? toBoolean(refreshPausedFromUrl)
+    : undefined;


do you really need this check? you're adding a default value to refreshPausedFromUrl o line 44, so it'll never be undefined, right?

fixed, thanks!

cauemarcondes · 2021-10-04T13:29:50Z

x-pack/plugins/apm/public/components/shared/search_bar.tsx

+  const refreshInterval = refreshIntervalFromUrl
+    ? toNumber(refreshIntervalFromUrl)
+    : undefined;


Same here?!

fixed, thanks!

cauemarcondes · 2021-10-04T13:31:26Z

x-pack/plugins/apm/public/components/shared/search_bar.tsx

+      refreshPaused={refreshPaused}
+      refreshInterval={refreshInterval}
+      onTimeRangeRefresh={() => {
+        incrementTimeRangeId();


Is it really the responsibility of who calls DatePicker to call incrementTimeRangeId? I would expect that the DatePicker component to handle it automatically.

The DatePicker component itself is being used by the UX app as well, and they're using urlParams still. So we have APMDatePicker and RUMDatePicker that each deal with date picker state differently.

cauemarcondes · 2021-10-04T13:37:42Z

x-pack/plugins/apm/public/hooks/use_date_range_redirect.ts

+import { TimePickerTimeDefaults } from '../components/shared/DatePicker/typings';
+import { useApmPluginContext } from '../context/apm_plugin/use_apm_plugin_context';
+
+export function useDateRangeRedirect() {


We already have a hook that handles time range useTimeRange do you think we could centralize this change in one single hook? does it make sense?

They serve different purposes, useTimeRange gets the (parsed) range parameters from the url and expects the apm client router to be used, this one checks whether the date range is set in the URL, and can be used by both the APM app and the UX app.

Why is it necessary for the date range to be set in the url - as @cauemarcondes says, can't we just get the date from useTimeRange?

discussed with @sqren in AON, we agreed this is ok for now but we will look into whether we can make these query parameters optional and use the kibana date store as the single source of truth.

…fresh

sorenlouv · 2021-10-05T11:46:41Z

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

+import { useApmRouter } from '../../../hooks/use_apm_router';
+import { useDateRangeRedirect } from '../../../hooks/use_date_range_redirect';
+
+export function RedirectWithDefaultDateRange({


Can you add a description to this. The purpose of this function was not clear to me until I read the PR description.

Perhaps some like:

// intercept invalid routes before they are rendered, and redirect, without rendering child components that expect valid & parsed routing parameters

I'm also wondering what an "invalid route" is in this context. Are we specifically referring to routes that require date range and don't have it? And isn't this something io-ts can handle for us?

sorenlouv · 2021-10-05T11:57:05Z

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

+      onRefresh={() => {
+        onRefreshTimeRange();
+      }}


tiny nit (just to keep in consistent with the other handlers)

Suggested change

onRefresh={() => {

onRefreshTimeRange();

}}

onRefresh={onRefreshTimeRange}

…fresh

kibanamachine · 2021-10-06T09:53:47Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1174	1177	+3

Async chunks

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

id	before	after	diff
`apm`	2.7MB	2.7MB	+1.6KB
`observability`	355.2KB	355.2KB	-7.0B
total			+1.6KB

History

💚 Build #157399 succeeded 230afe5
💚 Build #155635 succeeded 270a993
💚 Build #155328 succeeded cff3061
💔 Build #154884 failed 313ada0
💔 Build #154524 failed 8118445

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

feedback addressed

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-10-06T11:06:41Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Dario Gieselaar <dario.gieselaar@elastic.co>

dgieselaar added 3 commits September 21, 2021 10:05

[APM] Ensure refresh button works

f10671f

Closes elastic#110465.

Merge branch 'master' of github.com:elastic/kibana into time-range-re…

8b77f78

…fresh

Lint errors

8118445

dgieselaar added Team:APM - DEPRECATED Use Team:obs-ux-infra_services. release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Sep 21, 2021

dgieselaar added 4 commits September 22, 2021 09:43

Merge branch 'master' of github.com:elastic/kibana into time-range-re…

31a4329

…fresh

Don't render routes into itself

9f9f8fb

Fix JSX syntax error from merge conflict

262a155

Use isDateRangeSet from hook

313ada0

dgieselaar commented Sep 22, 2021

View reviewed changes

dgieselaar marked this pull request as ready for review September 22, 2021 09:20

dgieselaar requested a review from a team September 22, 2021 09:20

dgieselaar requested a review from a team as a code owner September 22, 2021 09:20

justinkambic approved these changes Sep 22, 2021

View reviewed changes

botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Sep 22, 2021

dgieselaar added 2 commits September 23, 2021 15:14

Merge branch 'master' of github.com:elastic/kibana into time-range-re…

7c7834d

…fresh

Fix types

cff3061

kibanamachine and others added 3 commits September 27, 2021 03:16

Merge branch 'master' into time-range-refresh

270a993

Merge branch 'master' of github.com:elastic/kibana into time-range-re…

aa83d39

…fresh

Merge branch 'time-range-refresh' of github.com:dgieselaar/kibana int…

230afe5

…o time-range-refresh

cauemarcondes previously requested changes Oct 4, 2021

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into time-range-re…

bc71f97

…fresh

sorenlouv reviewed Oct 5, 2021

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into time-range-re…

45c6046

…fresh

sorenlouv approved these changes Oct 6, 2021

View reviewed changes

Review feedback

baf608f

dgieselaar added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 6, 2021

dgieselaar merged commit 6aade8f into elastic:master Oct 6, 2021

dgieselaar deleted the time-range-refresh branch October 6, 2021 11:03

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 6, 2021

[APM] Ensure refresh button works (elastic#112652)

259498c

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine mentioned this pull request Oct 6, 2021

[7.x] [APM] Ensure refresh button works (#112652) #114082

Merged

Conversation

dgieselaar commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 22, 2021

Uh oh!

justinkambic left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 22, 2021

Uh oh!

dgieselaar commented Sep 27, 2021

Uh oh!

cauemarcondes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Oct 6, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

kibanamachine commented Oct 6, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dgieselaar commented Sep 21, 2021 •

edited

Loading

sorenlouv Oct 5, 2021 •

edited

Loading