Skip to content

Conversation

@Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Oct 5, 2020

Summary

This PR implements #79299. It ensures all logs pages (stream, anomalies, and categories) sync their timeranges with wider Kibana, via the data plugin.

timestamp-sync

The order of precedence now becomes:

  • URL params
  • Shared Kibana timerange
  • Defaults (these shouldn't be needed as the time filter service sets defaults based off the Kibana setting)

⚠️ Semantics change ⚠️

The changes here alter the semantics of the defaults used. Previously the defaults were as follows:

  • Stream: now - 1d
  • Anomalies: now - 2w
  • Categories: now - 2w

Now, the default will be the default set by the time filter service (unless, of course, the user has interacted with the time range and set a new one) when it is instantiated. The default used by the time filter service is from the timePicker.timeDefaults setting under Advanced Settings. This defaults to now - 15m, and as such the default for the logs pages becomes now - 15m, unless the user has set something else.

(I have also kept our original defaults in an || clause but these shouldn't ever be used, it was more in case something went wrong, but if something has gone wrong with the data plugin there are much bigger problems 🤷‍♀️)

Screenshot 2020-10-05 at 12 20 14

Testing

Time ranges should sync between wider Kibana (Discover etc) and all logs pages. This should be two-way.

@Kerry350 Kerry350 added release_note:enhancement v8.0.0 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 v7.10.0 labels Oct 5, 2020
@Kerry350 Kerry350 added this to the Logs UI 7.10 milestone Oct 5, 2020
@Kerry350 Kerry350 requested a review from a team October 5, 2020 12:26
@Kerry350 Kerry350 self-assigned this Oct 5, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty clean and the time range sync between the tabs feels pretty convenient. 👍

I'm a bit concerned about the semantic changes to the default time ranges. While defaulting to the last 15 minutes seems fine for the logs stream, this value seems impractical for the anomalies and categories tabs. With a bucket span of 15 minutes for the ML jobs there is a high change of not getting any meaningful results on the default view.

No good solution comes to mind, but here are some ideas:

  • don't include the anomalies and categories tabs in the sync
  • display a warning about the time range being too small for meaningful results

In an unrelated thought, while looking at the data plugin's timefilter service, I noticed it also supports syncing the refresh interval. Would it make sense to also include that?

@Kerry350
Copy link
Contributor Author

Kerry350 commented Oct 6, 2020

(Note: this is being deferred until the defaults situation can be addressed)

@Kerry350 Kerry350 modified the milestones: Logs UI 7.10, Logs UI 7.11 Oct 6, 2020
@Kerry350 Kerry350 requested a review from a team as a code owner October 7, 2020 14:35
@Kerry350
Copy link
Contributor Author

Kerry350 commented Oct 7, 2020

Following the semantics change, comment here: #79444 (review) I've pushed changes to try and address the defaults situation.

There were some additional things to consider when I started the implementation, I've highlighted these below:

TimeFilter side:

  • getTime on the TimeFilter instance now accepts a set of defaults as a parameter. Solutions can pass what they like here, and if nothing is provided the master constructor defaults are used (derived from the advanced Kibana settings). If time has been touched, then that is always returned.

  • When setTime is called (and actually changes something, e.g. the new time is different to the current time) a boolean flag (isTimeTouched) is flipped.

Logs side:

  • We can't just use a useEffect that always syncs the time range back to Kibana with setTime, because useEffect will always run on mount, and if we're currently in the defaults fork of the order of precedence (no URL params, and !isTimeTouched), then we will always sync our defaults back to the TimeFilter service, we don't want this. We want to sync user initiated changes.

  • The behaviour in wider Kibana (Discover etc) is that URL params that match the defaults, are not synced either (for the initial mount phase). This is the correct behaviour imo. When we first mount and the reconciliation phase is complete, our defaults will always ultimately end up in the URL (if doing a fresh navigation), if a user bookmarks this and loads the URL later, we still don't want those defaults to sync, we want them treated as defaults.

    • Changing back to the defaults in a user initiated fashion, will sync those changes for the session.
    • There's a small argument here that a user could make change A and then change B, change B happens to be the same as the defaults, this gets pushed to the URL (and it's a user change), and then when they reload the URL from a bookmark it's treated as defaults and therefore not synced to wider Kibana. I think this is a small edge case, and once things end up in the URL in a bookmark we can't later make a differentiation.
  • I've added two hooks useKibanaTimefilterTime and useSyncKibanaTimeFilterTime to deal with the complexity (well, not really complexity, just repetition). I have not merged these into one hook as we have dependencies between the URL state hook and the time filter hooks that make this unviable. To make them all play together actually makes things more confusing imo. Also, this allows things to opt in to getting / setting time without necessarily having to utilise sync.

    • The sync hook differentiates a mount phase and an update phase (using react-use hooks) to make sure the point above is handled nicely.
    • The useSyncKibanaTimeFilterTime hook plays really nicely with the useUrlState state hook (which anomalies and categories tabs both use). However, the stream is still using a pre-hooks container for handling the URL state with <UrlStateContainer />, and this has it's own initialize phase, as such I have used a separate useEffect for the stream. At a later date I would suggest we swap to the hook for the stream, I can file a ticket.

I think this covers everything 🤔

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really elegant and seems to work flawlessly - well done 👏

I wonder if the useSync... hook could subsume the other hook once the old url sync component is gone from the logs stream. 🤔 But for now the separation makes sense 👍

@weltenwort weltenwort added v7.11.0 and removed v7.10.0 labels Oct 8, 2020
@Kerry350
Copy link
Contributor Author

Kerry350 commented Oct 8, 2020

@weltenwort

I wonder if the useSync... hook could subsume the other hook once the old url sync component is gone from the logs stream.

Yep, I think once everything is on useUrlState we can make that magic 💥 happen.

Kerry350 and others added 6 commits October 8, 2020 13:00
…e_log_entry_categories_results_url_state.tsx

Co-authored-by: Felix Stürmer <[email protected]>
…entry_rate_results_url_state.tsx

Co-authored-by: Felix Stürmer <[email protected]>
- Move decision of using defaults to the solution consumer
- Add unit tests
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AppArch code LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
infra 1142 1143 +1

async chunks size

id before after diff
infra 3.8MB 3.8MB +12.7KB

page load bundle size

id before after diff
data 1.1MB 1.1MB +162.0B

History

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

@Kerry350 Kerry350 merged commit 74561d8 into elastic:master Oct 8, 2020
Kerry350 added a commit to Kerry350/kibana that referenced this pull request Oct 8, 2020
* Sync logs timestamps with wider Kibana

Co-authored-by: Felix Stürmer <[email protected]>
Kerry350 added a commit that referenced this pull request Oct 9, 2020
* Sync logs timestamps with wider Kibana

Co-authored-by: Felix Stürmer <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 12, 2020
* master: (23 commits)
  Table visualization renderer (elastic#79455)
  Migrate Jest JUnit reporter to TS (elastic#79919)
  store sorted bundleRefExportIds (elastic#80011)
  update chromedriver dependency to 86.0.0 (elastic#79972)
  [Security Solution][Case] Fix bug when changing connectors (elastic#80002)
  [kbn/std] add observable helpers to aid with rxjs 7 upgrade (elastic#79752)
  [Security Solution][Resolver] Pill numbers in compact notation (elastic#80038)
  [Logs UI] Sync logs timerange with wider Kibana (elastic#79444)
  [DOCS] Adds quick start (elastic#78822)
  [Ingest Manager]Fix ingest manager UI renaming (elastic#80036)
  [Monitoring] Fixed internal monitoring check (elastic#79241)
  [Security Solution][Exception Modal] Removes list operators in exception modal for EQL rules (elastic#79871)
  Update development documentation about REST API best practices (elastic#80009)
  [Monitoring] Improve indices loading against larger metricbeat-* indices (elastic#79190)
  [CI] Move kibana build dir outside of repo for functional tests (elastic#80018)
  [kbn/optimizer] bump low or add missing limits when updating automatically (elastic#80013)
  [Enterprise Search] Added a Credentials page to App Search (elastic#79749)
  [DOCS] Canvas refresh for 7.10 (elastic#80017)
  [TSVB] Add ignore global filters to series options (elastic#79337)
  Remove this check (elastic#79202)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Logs UI Logs UI feature release_note:enhancement Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.11.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Logs App] Use the timefilter API to load and persist time range selections

5 participants