[Infra UI] Add date picker to asset details #164450

crespocarlos · 2023-08-22T13:18:28Z

closes: #160378

Summary

This PR adds a date-picker to the Asset Details (full page and flyout)

Flyout

Full page view

Setting a date range

full_page_date_picker.mov

Brush event filter

chart_date_filter.mov

Bonus

The uptime link was removed since the UI won't work.

How to test

Start a local Kibana instance
Start metricbeat with system module enabled
Navigate to Infrastructure > Hosts
- Click on a host name and check if the new Node Details page opened
  - Check if the date range is set with the same date range from the Host Detail view
Navigate to Infrastructure > Hosts
- Open the flyout
- Click on Open as page and check if the new Node Details page opened with the same date range from the Host Detail view
Navigate to Infrastructure > Inventory
- With Show: Host selected, click on a waffle item to open the flyout, click on Open as page and check if the new Node Details page opened with the same date range from the Inventory UI

Validate if the date picker value is applied to all tabs that have the component (Overview, Metadata, Processes, Logs, Anomalies)
Validate if the date range changes when selecting a time range in the charts (in the flyout, the date range from the Hosts View must not change)

apmmachine · 2023-08-22T13:18:46Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

…-picker-to-asset-details

crespocarlos · 2023-08-22T16:09:42Z

x-pack/plugins/infra/public/components/asset_details/tabs/processes/processes.tsx

-          <EuiIconTip
-            aria-label={i18n.translate(
-              'xpack.infra.metrics.nodeDetails.processesHeader.tooltipLabel',
+      <EuiFlexGroup direction="column" gutterSize="m">


Changed to use flexbox instead of having <EuiSpacer size="m" /> to separate each section

crespocarlos · 2023-08-22T16:10:37Z

x-pack/plugins/infra/public/components/asset_details/tabs/processes/processes_table.tsx

@@ -218,14 +228,6 @@ const ProcessesTableBody = ({ items, currentTime }: TableBodyProps) => (
  </>
 );

-const StyledTableBody = euiStyled(EuiTableBody)`


euiStyled is marked as deprecated.

crespocarlos · 2023-08-22T16:11:12Z

x-pack/plugins/infra/public/components/asset_details/tabs/processes/processes_table.tsx

-  overflow: hidden;
-  text-overflow: ellipsis;
-`;
+const CodeLine = ({ command }: { command: string }) => {


euiStyled is deprecated. Created this new component in order to replace it.

Thanks for changing those!

crespocarlos · 2023-08-22T16:12:30Z

...k/plugins/infra/public/pages/metrics/hosts/components/host_details_flyout/flyout_wrapper.tsx

  const [hostFlyoutState, setHostFlyoutState] = useHostFlyoutUrlState();

  return source ? (
    <AssetDetails
      asset={{ id: name, name }}
      assetType="host"
-      dateRange={searchCriteria.dateRange}
+      dateRange={parsedDateRange}


With this change, we make sure the flyout will always open with the same date range. Previously, if the date picker had relative dates, each time the flyout opened a new absolute date would be calculated.

crespocarlos · 2023-08-22T16:13:21Z

...s/metrics/inventory_view/components/ml/anomaly_detection/anomalies_table/anomalies_table.tsx

+        align-self: center;
+      `}
+    >
+      <EuiEmptyPrompt


Replacing with an EUI component

crespocarlos · 2023-08-22T16:14:32Z

...s/metrics/inventory_view/components/ml/anomaly_detection/anomalies_table/anomalies_table.tsx

+      {!hideDatePicker && (
+        <EuiFlexItem grow={false}>
+          <EuiSuperDatePicker
+            start={dateRange.from}
+            end={dateRange.to}
+            showUpdateButton={false}
+            onTimeChange={onTimeChange}
+            width="full"
+          />
+        </EuiFlexItem>


Didn't want to mess with other components that use the anomalies table. We might want to refactor this component in the future and split content and date picker into separate components

crespocarlos · 2023-08-22T16:15:04Z

x-pack/plugins/infra/public/pages/metrics/metric_detail/asset_detail_page.tsx

+  // Retrocompatibility
+  const handleTabStateChange = ({ dateRange: newDateRange }: TabState) => {
+    if (newDateRange) {
+      setTimeRange({
+        from: newDateRange.from,
+        to: newDateRange.to,
+        interval: parsedTimeRange.interval,
+      });
+    }
+  };


We can remove this once we start storing the asset details state in the URL.

…-picker-to-asset-details

crespocarlos · 2023-08-23T12:38:31Z

@elasticmachine merge upstream

elasticmachine · 2023-08-23T13:45:10Z

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

x-pack/plugins/infra/public/components/asset_details/content/content.tsx

mykolaharmash · 2023-08-24T10:09:30Z

x-pack/plugins/infra/public/components/asset_details/tabs/overview/metrics/metrics_grid.tsx

+      ({
+        range,
+        preventDefault,
+      }: Parameters<NonNullable<LensEmbeddableInput['onBrushEnd']>>[0]) => {


Might make sense to add an alias for this type to make it a bit easier to read

Good point.

jennypavlova

Good job! 👏 The new date picker works fine and the date value persists between the tabs. I have some questions as I think I found 2 issues that we can check:

In case the user changes the time range inside the flyout it persists when he opens the details page but when the back button is pressed the flyout loads with the default time range (not the selected one when the user left the page to see the details page):

Screen.Recording.2023-08-24.at.12.11.22.mov

When the user selects the date inside the flyout and refreshes the page/shares the URL the time range does not persist

Screen.Recording.2023-08-24.at.12.07.51.mov

I am not sure about that but sometimes when using "now" as "to" date it is converted to relative data and sometimes it stays as "now". Usually, when the "from" date is changed it changes to the relative date instead of "now" so what should be the expected behavior (should we use "now" or we should always convert to the relative date?):

Screen.Recording.2023-08-24.at.12.46.03.mov

Screen.Recording.2023-08-24.at.12.47.08.mov

jennypavlova · 2023-08-24T10:24:15Z

x-pack/plugins/infra/public/components/asset_details/tabs/processes/processes_table.tsx

-  overflow: hidden;
-  text-overflow: ellipsis;
-`;
+const CodeLine = ({ command }: { command: string }) => {


Thanks for changing those!

jennypavlova · 2023-08-24T10:36:39Z

x-pack/plugins/infra/public/components/asset_details/hooks/use_date_range.ts

+  initialDateRange: TimeRange;
+}
+
+export function useDateRangeProvider({ initialDateRange }: UseAssetDetailsStateProps) {


When we have the date range persisted in the URL (the case of the flyout for example) should we first get the date range from the URL value (passed to the asset details component) and if we don't have it to fallback to default (the value selected in the host view / the 15m default)?

You're completely right. I totally forgot to store the date range in the URL state.

jennypavlova · 2023-08-24T10:37:25Z

x-pack/test/functional/apps/infra/node_details.ts

+        ]);
+      });
+
+      describe('#Date picker', () => {


Nice ❤️

jennypavlova · 2023-08-24T11:01:10Z

x-pack/plugins/infra/public/components/asset_details/content/content.tsx

+            FlyoutTabIds.OVERVIEW,
+            FlyoutTabIds.LOGS,
+            FlyoutTabIds.METADATA,
+            FlyoutTabIds.PROCESSES,


Are the processes tab results affected by the new dates as in the process list API it will always check for 1-minute interval 🤔

Never mind I saw that we plan to add a tooltip to explain that in this issue so I guess it should be fine.

crespocarlos · 2023-08-24T12:40:26Z

Thanks @mykolaharmash and @jennypavlova for reviewing the PR.

In case the user changes the time range inside the flyout it persists when he opens the details page but when the back button is pressed the flyout loads with the default time range (not the selected one when the user left the page to see the details page):

That's an interesting use case. To achieve that we'd have to have a context provider to share the state between Hosts and Node Details. It's similar to this #164300, but the other way around. Perhaps we can rephrase that ticket to cover this use case?

When the user selects the date inside the flyout and refreshes the page/shares the URL the time range does not persist

Working on this.

I am not sure about that but sometimes when using "now" as "to" date it is converted to relative data and sometimes it stays as "now". Usually, when the "from" date is changed it changes to the relative date instead of "now" so what should be the expected behavior (should we use "now" or we should always convert to the relative date?):

Gotcha. The second video shows the problem. Will check

crespocarlos · 2023-08-24T14:09:10Z

@elasticmachine merge upstream

crespocarlos · 2023-08-24T15:01:37Z

@elasticmachine merge upstream

jennypavlova

Thanks for the changes! I tried the navigation again and now the full page is setting the last one hour based on the "to" date - not the whole interval set in the flyout:

nav_to_full_page.mov

Is that intentional or it should be changed?

But going back to the flyout is fixed (the selected dates before navigating to the full page view are shown) the URL sharing and persisting after refresh as well 🎉

jennypavlova · 2023-08-24T16:17:49Z

x-pack/plugins/infra/public/pages/metrics/metric_detail/hooks/use_metrics_time.ts

@@ -64,9 +64,11 @@ export const useMetricsTime = () => {
    parseRange(urlState.time || DEFAULT_TIMERANGE)
  );

-  const updateTimeRange = useCallback((range: MetricsTimeInput) => {
+  const updateTimeRange = useCallback((range: MetricsTimeInput, parseDate = true) => {


Nit: maybe we can always pass a parsed date to this function so here we can simplify the logic, wdyt?

I don't want to change this function so much. We won't use it after this #164147 is done

crespocarlos · 2023-08-24T17:46:12Z

Thanks for the changes! I tried the navigation again and now the full page is setting the last one hour based on the "to" date - not the whole interval set in the flyout:

nav_to_full_page.mov
Is that intentional or it should be changed?

@jennypavlova , thanks for thoroughly testing things! The link was doing what the inventory does: end date - 1h. I've fixed it and added a test to validate it.

kibana-ci · 2023-08-24T18:47:51Z

💚 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
`infra`	2.0MB	2.0MB	+241.0B

History

💚 Build #153215 succeeded a28f7ba
💔 Build #153189 failed 4383cf1
💔 Build #153168 failed f17194f
💚 Build #152819 succeeded 9dae997
💛 Build #152768 was flaky 9fe3bbe
💔 Build #152727 failed 21e007dece96b68f1e2564fb8f2468379e097f45

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

jennypavlova

LGTM 🚀 Thank you for the fixes!

Add date picker to Assed Details

ea4bf91

crespocarlos force-pushed the 160378-add-date-picker-to-asset-details branch from baec8be to ea4bf91 Compare August 22, 2023 13:19

crespocarlos added 2 commits August 22, 2023 15:32

Merge branch 'main' of github.com:elastic/kibana into 160378-add-date…

3d88118

…-picker-to-asset-details

Move date picker to content level; add tests

28f893a

crespocarlos commented Aug 22, 2023

View reviewed changes

crespocarlos added 2 commits August 23, 2023 09:50

Merge branch 'main' of github.com:elastic/kibana into 160378-add-date…

49fd1b4

…-picker-to-asset-details

Fix build

441db57

crespocarlos changed the title ~~160378 add date picker to asset details~~ [Infra UI] Add date picker to asset details Aug 23, 2023

Remove uptime link

8c1e5c7

crespocarlos force-pushed the 160378-add-date-picker-to-asset-details branch from 21e007d to 8c1e5c7 Compare August 23, 2023 12:38

Merge branch 'main' into 160378-add-date-picker-to-asset-details

9fe3bbe

crespocarlos marked this pull request as ready for review August 23, 2023 13:45

crespocarlos requested a review from a team as a code owner August 23, 2023 13:45

Small fix

9dae997

jennypavlova self-requested a review August 24, 2023 09:20

mykolaharmash reviewed Aug 24, 2023

View reviewed changes

jennypavlova reviewed Aug 24, 2023

View reviewed changes

CR fixes

f17194f

crespocarlos requested review from mykolaharmash and jennypavlova August 24, 2023 13:33

Fix build

88a4719

Merge branch 'main' into 160378-add-date-picker-to-asset-details

4383cf1

Merge branch 'main' into 160378-add-date-picker-to-asset-details

a28f7ba

jennypavlova reviewed Aug 24, 2023

View reviewed changes

Fix date range passed from the flyout to the node detail page

670b436

crespocarlos requested a review from jennypavlova August 24, 2023 17:46

jennypavlova approved these changes Aug 25, 2023

View reviewed changes

crespocarlos merged commit c7dd76d into elastic:main Aug 25, 2023

crespocarlos deleted the 160378-add-date-picker-to-asset-details branch August 25, 2023 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Infra UI] Add date picker to asset details #164450

[Infra UI] Add date picker to asset details #164450

crespocarlos commented Aug 22, 2023 •

edited

Loading

apmmachine commented Aug 22, 2023

crespocarlos Aug 22, 2023

crespocarlos Aug 22, 2023

crespocarlos Aug 22, 2023

jennypavlova Aug 24, 2023

crespocarlos Aug 22, 2023

crespocarlos Aug 22, 2023

crespocarlos Aug 22, 2023

crespocarlos Aug 22, 2023

crespocarlos commented Aug 23, 2023

elasticmachine commented Aug 23, 2023

mykolaharmash Aug 24, 2023

crespocarlos Aug 24, 2023

jennypavlova left a comment

jennypavlova Aug 24, 2023

jennypavlova Aug 24, 2023

crespocarlos Aug 24, 2023 •

edited

Loading

jennypavlova Aug 24, 2023

jennypavlova Aug 24, 2023

jennypavlova Aug 24, 2023

crespocarlos commented Aug 24, 2023

crespocarlos commented Aug 24, 2023

crespocarlos commented Aug 24, 2023

jennypavlova left a comment

jennypavlova Aug 24, 2023

crespocarlos Aug 24, 2023

crespocarlos commented Aug 24, 2023

kibana-ci commented Aug 24, 2023

jennypavlova left a comment

[Infra UI] Add date picker to asset details #164450

[Infra UI] Add date picker to asset details #164450

Conversation

crespocarlos commented Aug 22, 2023 • edited Loading

Summary

Bonus

How to test

apmmachine commented Aug 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos commented Aug 23, 2023

elasticmachine commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennypavlova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos Aug 24, 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

crespocarlos commented Aug 24, 2023

crespocarlos commented Aug 24, 2023

crespocarlos commented Aug 24, 2023

jennypavlova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos commented Aug 24, 2023

kibana-ci commented Aug 24, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

History

jennypavlova left a comment

Choose a reason for hiding this comment

crespocarlos commented Aug 22, 2023 •

edited

Loading

crespocarlos Aug 24, 2023 •

edited

Loading