[Logs UI] Use the Unified Search Bar for date range selection #144351

weltenwort · 2022-11-01T18:36:05Z

📝 Summary

This enables the date-picker of the unified search bar introduced into the Logs UI in #143222 and simultaneously removes the custom date picker.

closes #142767

🎨 Previews

🕵️‍♀️ Implementation notes

`kibana_utils` changes

The kibana_utils plugin now also exports IStateStorage so additional storages can be implemented.

Alerts page test changes

The functional test of the observability alerts page were coupled to the assumed default state of the log stream. I changed them to use the observability overview for external timefilter checks instead and cleaned up some of the navigation code.

`logPosition` state

The log position state is now expressed using a state container, which is synced with the url and unified search bar via the common syncState utility in combination with a KbnUrlStateStorage and a newly implemented TimefilterStateStorage respectively.

%%{init: { "sequence": { "mirrorActors": false } } }%%
sequenceDiagram

participant LogPositionProvider
participant useLogPosition
participant logPositionStateContainer
participant useLogPositionUrlStateSync
participant useLogPositionTimefilterStateSync

LogPositionProvider -->>+ useLogPosition: mount

useLogPosition -->> useLogPositionUrlStateSync: read initial value
useLogPosition -->> useLogPositionTimefilterStateSync: read initial value

useLogPosition -->>+ logPositionStateContainer: create with merged initial values

useLogPosition -->>+ useLogPositionUrlStateSync: effect: start sync
useLogPosition -->>+ useLogPositionTimefilterStateSync: effect: start sync

useLogPositionUrlStateSync -->>- useLogPosition: effect end: stop sync
useLogPositionTimefilterStateSync -->>- useLogPosition: effect end: stop sync

useLogPosition -->>- LogPositionProvider: unmount

deactivate logPositionStateContainer

The structure of the logPosition URL query parameter remains compatible, but is now validated via io-ts and ignored if invalid.

For simplicity, the actual representation of the timeRange and refreshInterval states are closer to the structure used in the timefilter service. The streaming state is derived on-the-fly from the refresh pause toggle and value.

`TimefilterStateStorage`

In order to be able to use the common syncState utility, I added a state storage that is backed by the timefilter service. That means in contrast to other state storages it doesn't support arbitrary key-value pairs. I tried to represent that in its interface types.

Date-picker and "stream live" buttom

I left the "stream live" buttom on the second row and reduced it to an EuiEmptyButton to prevent clashes with the refresh button of the search bar.

The streaming mode can be enabled/disabled both via that button or via the refresh settings of the datepicker.

Kerry350

Thanks for the detailed description ♥️

Dropping a comment on from my functional review before I look at the code. On the whole this seemed to work really well, and I know the state wasn't simple to untangle so big kudos for how solid everything felt.

I did run in to some issues with "Stream live" and refresh intervals. When I turn on streaming this is persisted to the URL (the streamLive boolean), but the refresh interval isn't.

This results in some surprising behaviour on refresh of the page:

On refresh this is set to 0 and I see a small warning icon:

Due to the overall streamLive boolean state being persisted to the URL the button does return to being "on", and the streaming message does display at the bottom of the stream, but no requests are actually dispatched on an interval.

Quickly looking at the code this makes sense as refreshInterval ends up getting set to:

{
  pause: false,
  value: 0
}

in this scenario (due to updateStateFromTimefilterState(), where a 0 bug exists: https://github.com/elastic/kibana/pull/144389/files#r1011529270), and the useInterval returns null when latestLogPositionState.refreshInterval.value <= 0.

Apart from this my other tests seemed to work as expected. I'll take a look at the code now.

weltenwort · 2022-11-03T12:10:48Z

I did run in to some issues with "Stream live" and refresh intervals. When I turn on streaming this is persisted to the URL (the streamLive boolean), but the refresh interval isn't.

That's true. We never had the interval in the URL and I didn't want to change the URL param structure for now due to BWC. I'll look into working around that by setting it to 5s if it's 0s.

Kerry350

I left a few minor comments / questions regarding the code but all in all it looks really solid to me. Thanks for investing the time with the state containers, state storage etc. Even though some of this might change with the state refactoring I think it sets up a really good foundation overall — it was definitely a lot easier to follow than what we had previously. I might just have to brush up on some functional programming concepts 😄

x-pack/plugins/infra/public/containers/logs/log_position/log_position_state.ts

x-pack/plugins/infra/public/containers/logs/log_position/use_log_position.ts

x-pack/plugins/infra/public/utils/kbn_url_state_context.ts

weltenwort · 2022-11-04T17:46:23Z

@Kerry350 thanks for the feedback

I hope to have fixed the tests by reducing the strictness of the url state parsing a bit by making all properties optional (as they were before).

I also tried to address the refresh interval problem you discovered. To do that I had to move the visiblePosition state into the state container too, so it can be kept in sync with the targetPosition. I think it works out pretty well with latestPosition taking over the role of the separate visibleMidpoint state we had before. Let me know what you think.

Additionally, I removed the dependency on redux-logger in favor of connecting the redux devtools browser extension. It's read-only for now, but that's all I needed for debugging. This should help to keep the bundle smaller.

elasticmachine · 2022-11-04T19:00:17Z

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

Kerry350 · 2022-11-08T15:03:20Z

@weltenwort

The functionality seems to work as expected now with regards to refresh intervals 🙏

Unfortunately, my testing was a bit inconsistent with regards to the stream live button. When I try to turn streaming on or off (via the button) I seem to hit something that causes the call stack to be exceeded and the browser locks up. I've tried this with and without Redux dev tools, and with the dev tools closed entirely, and it still seems to happen. I don't see anything obvious (like Redux dev tools being spammed with actions or something similar). But there is a message about payloads being too large (sometimes), but I don't see an abnormally sized payload in the dev tools.

This doesn't happen via the "Refresh every" toggle in the date picker, it's just the button. I'd say it happens 90% of the time.

(I guess it's not 100% clear in the gif, but the mouse cursor kind of denotes when it's locked up).

In terms of data I'm using an oblt-cli generated cluster against Edge.

weltenwort · 2022-11-08T20:21:34Z

Good catch! I was able to reproduce that and find the cause:

The startLiveStreaming and stopLiveStreaming transitions are used directly as React event handlers. That means they were called with the respective SyntheticEvent as the argument, which doesn't play well with the devtool's attempt to serialize it.

I added a safeguard against that. Unfortunately there's no "official" way to check whether a value is a SyntheticEvent. 😬

kibana-ci · 2022-11-08T21:20:03Z

💚 Build Succeeded

Buildkite Build
Commit: e1ebdb9

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`infra`	1105	1114	+9

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`kibanaUtils`	420	424	+4

Async chunks

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

id	before	after	diff
`infra`	1.1MB	1.1MB	+19.0KB
`observability`	485.7KB	485.7KB	+55.0B
total			+19.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`kibanaUtils`	9	8	-1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`infra`	85.4KB	85.6KB	+223.0B

Unknown metric groups

API count

id	before	after	diff
`kibanaUtils`	615	624	+9

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
`infra`	45	44	-1
`osquery`	108	113	+5
`securitySolution`	440	446	+6
total			+18

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	67	73	+6
`infra`	53	52	-1
`osquery`	109	115	+6
`securitySolution`	517	523	+6
total			+19

History

💚 Build #85568 succeeded 325103c
💚 Build #85531 succeeded 34242ea
💔 Build #85266 failed 2c5e0a9
💔 Build #84887 failed a731f88
💔 Build #84597 failed e35620f
💔 Build #84483 failed 25eff4e3244b8efbd4774970cdcaba7424db4b18

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

cc @weltenwort

maryam-saeidi · 2022-11-09T08:37:47Z

x-pack/test/observability_functional/apps/observability/pages/alerts/state_synchronization.ts

      await setTimeRangeToXDaysAgo(10);
-      await (await find.byLinkText('Alerts')).click();
+      await pageObjects.observability.clickSolutionNavigationEntry(


maryam-saeidi · 2022-11-09T09:06:15Z

x-pack/test/observability_functional/apps/observability/pages/alerts/state_synchronization.ts

-      await numerOfDaysField.clearValueWithKeyboard();
-      await numerOfDaysField.type(numberOfDays.toString());
+      const numberField = await find.byCssSelector('[aria-label="Time value"]');
+      await numberField.clearValueWithKeyboard();


Was there an issue that led to this change or is it an improvement?

Yes, previously the test assumed that "days" was selected as the time unit and it therefore just changed the number. But now the previous date picker state might contain "hours", so the test needs to explicitly set it to "days".

maryam-saeidi

AO changes LGTM!

Kerry350

Thanks for the patience whilst I worked through the review 🙏 I couldn't find any issues. I know this wasn't a trivial issue at all, great work 🙌

I think it works out pretty well with latestPosition taking over the role of the separate visibleMidpoint state we had before. Let me know what you think.

Agreed, this seemed like a positive change to me, and a better location.

I added a safeguard against that. Unfortunately there's no "official" way to check whether a value is a SyntheticEvent. 😬

Nice one catching the problem here. For all it would be nice to have an "official" method thanks for making it obvious with the helper functions.

I can confirm this was fixed in my testing:

Kerry350 · 2022-11-09T13:03:21Z

x-pack/plugins/infra/public/utils/state_container_devtools.ts

+      ...config,
+      serialize: {
+        ...(typeof config?.serialize === 'object' ? config.serialize : {}),
+        replacer: (_key: string, value: unknown) => replaceReactSyntheticEvent(value),


Dosant · 2022-11-09T15:23:43Z

x-pack/plugins/infra/public/utils/timefilter_state_storage.ts

+  refreshInterval?: RefreshInterval;
+}
+
+export const createTimefilterStateStorage = ({


Interesting approach! haven't thought of it.

There is an existing util that sets up syncing between a state container and data services. In addition to time range it also supports filters and query, but can be configured to only sync time range

src/plugins/data/public/query/state_sync/connect_to_query_state.ts

The usage would be something like this:

const stop = connectToQueryState(data.query, stateContainer, { time: true, refreshInterval: true})

Take a look! Maybe this could work well and will allow to clean up some code (the new code looks good tbh 👍 )

Otherwise lgtm, no objections exporting more interfaces

I didn't know about that utility 💡 thanks for pointing it out

weltenwort added Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Nov 1, 2022

weltenwort self-assigned this Nov 1, 2022

weltenwort added 9 commits November 2, 2022 10:14

Introduce provider for shared KbnUrlStateStorage

3f919b7

Introduce a timefilter state storage

8d313da

Express TimeKey as a runtime type

12d2ac7

Move out utility function

7b7dca0

Refactor log position state to use state container

b3a9437

Remove unused files

4215b32

Remove the custom date picker

e2b1a9c

Throttle update of timestamp for "now" date

280b1b1

Remove unused styled component

e35620f

weltenwort force-pushed the logs-unified-search-bar-datepicker branch from 25eff4e to e35620f Compare November 2, 2022 10:20

Fix linter warning about type-only export

bf8520d

Kerry350 self-requested a review November 2, 2022 15:31

Kerry350 reviewed Nov 2, 2022

View reviewed changes

Relax url state parsing and add some tests

a731f88

Kerry350 reviewed Nov 3, 2022

View reviewed changes

weltenwort added 10 commits November 3, 2022 18:37

Only log state changes in development

94dab45

Move visible positions into state container

7f58b73

Add some log position state unit tests

3f3ff76

Avoid creating a new history entry on scrolling

e3a78df

Merge branch 'main' into logs-unified-search-bar-datepicker

2c5e0a9

Merge branch 'main' into logs-unified-search-bar-datepicker

f7998ae

Make page title test more reliable

0087179

Decouple alerts page tests from log stream state

5bbfdfe

Avoid resetting the refresh interval

34242ea

Use redux devtools instead of redux-logger

325103c

weltenwort marked this pull request as ready for review November 4, 2022 19:00

weltenwort requested review from a team as code owners November 4, 2022 19:00

weltenwort added 3 commits November 8, 2022 17:46

Merge branch 'main' into logs-unified-search-bar-datepicker

5dfe7de

Avoid serialization errors with SyntheticEvents

b8ba64f

Disable redux devtools features requiring dispatch

e1ebdb9

maryam-saeidi reviewed Nov 9, 2022

View reviewed changes

maryam-saeidi approved these changes Nov 9, 2022

View reviewed changes

Kerry350 approved these changes Nov 9, 2022

View reviewed changes

weltenwort added the release_note:enhancement label Nov 9, 2022

weltenwort changed the title ~~[Logs UI] Support the Unified Search Bar for date range selection~~ [Logs UI] Use the Unified Search Bar for date range selection Nov 9, 2022

Dosant approved these changes Nov 9, 2022

View reviewed changes

vadimkibana approved these changes Nov 9, 2022

View reviewed changes

weltenwort merged commit c88a680 into elastic:main Nov 9, 2022

weltenwort deleted the logs-unified-search-bar-datepicker branch November 9, 2022 17:49

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Logs UI] Use the Unified Search Bar for date range selection #144351

[Logs UI] Use the Unified Search Bar for date range selection #144351

weltenwort commented Nov 1, 2022 •

edited

Loading

Kerry350 left a comment •

edited

Loading

weltenwort commented Nov 3, 2022

Kerry350 left a comment

weltenwort commented Nov 4, 2022

elasticmachine commented Nov 4, 2022

Kerry350 commented Nov 8, 2022 •

edited

Loading

weltenwort commented Nov 8, 2022

kibana-ci commented Nov 8, 2022

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

maryam-saeidi Nov 9, 2022

maryam-saeidi Nov 9, 2022

weltenwort Nov 9, 2022

maryam-saeidi left a comment

Kerry350 left a comment

Kerry350 Nov 9, 2022

Dosant Nov 9, 2022

weltenwort Nov 9, 2022

[Logs UI] Use the Unified Search Bar for date range selection #144351

[Logs UI] Use the Unified Search Bar for date range selection #144351

Conversation

weltenwort commented Nov 1, 2022 • edited Loading

📝 Summary

🎨 Previews

🕵️‍♀️ Implementation notes

kibana_utils changes

Alerts page test changes

logPosition state

TimefilterStateStorage

Date-picker and "stream live" buttom

Kerry350 left a comment • edited Loading

Choose a reason for hiding this comment

weltenwort commented Nov 3, 2022

Kerry350 left a comment

Choose a reason for hiding this comment

weltenwort commented Nov 4, 2022

elasticmachine commented Nov 4, 2022

Kerry350 commented Nov 8, 2022 • edited Loading

weltenwort commented Nov 8, 2022

kibana-ci commented Nov 8, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

maryam-saeidi Nov 9, 2022

Choose a reason for hiding this comment

maryam-saeidi Nov 9, 2022

Choose a reason for hiding this comment

weltenwort Nov 9, 2022

Choose a reason for hiding this comment

maryam-saeidi left a comment

Choose a reason for hiding this comment

Kerry350 left a comment

Choose a reason for hiding this comment

Kerry350 Nov 9, 2022

Choose a reason for hiding this comment

Dosant Nov 9, 2022

Choose a reason for hiding this comment

weltenwort Nov 9, 2022

Choose a reason for hiding this comment

weltenwort commented Nov 1, 2022 •

edited

Loading

`kibana_utils` changes

`logPosition` state

`TimefilterStateStorage`

Kerry350 left a comment •

edited

Loading

Kerry350 commented Nov 8, 2022 •

edited

Loading