Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Logs UI] Fix log stream data fetching #93201

Merged
merged 4 commits into from
Mar 4, 2021

Conversation

Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Mar 2, 2021

Summary

This fixes some issues with log stream data fetching, found in #92215 (comment) and #92382 (comment).

This switches to a ReplaySubject for the around requests coordination. This was already implemented here: 202fdf2 at the data search hook level, but around has some additional complexity as it wraps a before and after, this extends that treatment to the custom observables used as they, again, fall foul of React's lifecycle and missing subscriptions on the "cold" observable from the data plugin.

The newDateRangeDoesNotOverlap comparison has also been flipped around, as it was checking for the opposite (overlap). The way that full refetches are determined has been changed.

Testing

  • I could get the missing requests scenario to happen 100% of the time by trying to use the "View in stream" link from the examples on the anomalies page.

  • Changing the time range to cast a wider net should also now update the entries.

@Kerry350 Kerry350 added bug Fixes for quality problems that affect the customer experience 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.12.0 labels Mar 2, 2021
@Kerry350 Kerry350 requested a review from a team March 2, 2021 12:32
@Kerry350 Kerry350 self-assigned this Mar 2, 2021
@elasticmachine
Copy link
Contributor

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

@Kerry350 Kerry350 added the release_note:skip Skip the PR/issue when compiling release notes label Mar 2, 2021
@neptunian
Copy link
Contributor

I checked this out and navigated to the Logs stream page and it keeps making requests and reloading about every second or more. Can you try to reproduce?
Screen Shot 2021-03-02 at 2 50 25 PM

@neptunian
Copy link
Contributor

neptunian commented Mar 2, 2021

I don't seem to be getting the refreshing issue I mentioned above now. Will let you know if it happens again.
I tested it on the Logs stream page and it now updates the log stream to have data when the time range has been changed when before there was no data. But now I'm seeing the opposite where if you have data in the currently selected time range and update the time range to a range that does not have data, it still shows data. Notice how the mini map was updated to not have any data but there is data in the stream from an incorrect time range:
Screen Shot 2021-03-02 at 3 07 08 PM

@Kerry350
Copy link
Contributor Author

Kerry350 commented Mar 2, 2021

@neptunian

Thanks 🙏

I don't seem to be getting the refreshing issue I mentioned above now. Will let you know if it happens again.

I couldn’t replicate this either. Did it carry on infinitely when you saw it? With async search there are multiple partial queries, and I have seen a years worth of data take around 8 (mileage will vary) requests against the Obs cluster.

I’ll look into the other issue tomorrow.

@neptunian
Copy link
Contributor

I don't seem to be getting the refreshing issue I mentioned above now. Will let you know if it happens again.

I couldn’t replicate this either. Did it carry on infinitely when you saw it? With async search there are multiple partial queries, and I have seen a years worth of data take around 8 (mileage will vary) requests against the Obs cluster.

I don't think it was that because it was very fast and I probably logged over 50 before i stopped it. I'll keep an eye out for it but maybe something just weird happening locally.

Verified that the "view in stream" link works now 🎉

@Kerry350
Copy link
Contributor Author

Kerry350 commented Mar 4, 2021

@neptunian This should be ready for another look. The way our state was setup there was always going to be a bug in one direction with regards to timeranges. I've changed the way we assess when to do a full refetch with fetchEntries. On the logs team I think we'll need to reassess the state on the stream, and most likely move to useReducer as on the ML pages as there's lots of interconnected setState calls and scope for race conditions, but that's for another day and another ticket 😅

These should all work now:

  • Changing the timerange via the date picker
  • Pagination / infinite scroll by scrolling to the top / bottom boundaries with more entries
  • Extending the timerange. This one is easier to see when you use a filter and timerange with a small set of entries.

Screenshot 2021-03-04 at 12 12 29

  • Clicking at some point in time in the summary minimap on the right hand side
  • Live stream via the Stream live button

And there still shouldn't be any "missed" requests, coming from the anomalies link etc.

@neptunian
Copy link
Contributor

neptunian commented Mar 4, 2021

All the cases above work! There is just one thing which I believe I noticed yesterday before your most recent commit. Though I don't know if its related to this PR for sure. When I click the "View in stream" link for an item with an unknown dataset, I get this error. I have some old ES data that is maybe causing problems so perhaps you can check if it works for you.

Screen Shot 2021-03-04 at 8 56 33 AM

@Kerry350
Copy link
Contributor Author

Kerry350 commented Mar 4, 2021

👍 It shouldn't be related to this PR, but I'll take a look and push a fix here.

@Kerry350
Copy link
Contributor Author

Kerry350 commented Mar 4, 2021

@neptunian Pushed a fix for the links when the dataset doesn't exist.

@neptunian
Copy link
Contributor

Tested locally and works.

@kibanamachine
Copy link
Contributor

💚 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 1.9MB 1.9MB +466.0B

History

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

cc @Kerry350

@Kerry350 Kerry350 merged commit e45718d into elastic:master Mar 4, 2021
Kerry350 added a commit to Kerry350/kibana that referenced this pull request Mar 4, 2021
* Use ReplaySubject and amend date comparisons

* Assess date range expressions separately

* Only add dataset filter to view in stream links if one exists
Kerry350 added a commit to Kerry350/kibana that referenced this pull request Mar 4, 2021
* Use ReplaySubject and amend date comparisons

* Assess date range expressions separately

* Only add dataset filter to view in stream links if one exists
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 4, 2021
* master: (107 commits)
  [Logs UI] Fix log stream data fetching (elastic#93201)
  [App Search] Added relevance tuning search preview (elastic#93054)
  [Canvas] Fix reports embeddables (elastic#93482)
  [ILM] Added new functional test in ILM for creating a new policy (elastic#92936)
  Remove direct dependency on statehood package (elastic#93592)
  [Maps] Track tile loading status (elastic#91585)
  [Discover][Doc] Improve main documentation (elastic#91854)
  [Upgrade Assistant] Disable UA and add prompt (elastic#92834)
  [Snapshot Restore] Remove cloud validation for slm policy (elastic#93609)
  [Maps] Support GeometryCollections in GeoJson upload (elastic#93507)
  [XY Charts] fix partial histogram endzones annotations (elastic#93091)
  [Core] Simplify context typings (elastic#93579)
  [Alerting] Improving health status check (elastic#93282)
  [Discover] Restore context documentation (elastic#90784)
  [core-docs] Edits core-intro section for the new docs system (elastic#93540)
  add missed codeowners (elastic#89714)
  fetch node labels via script execution (elastic#93225)
  [Security Solution] Adds getMockTheme function (elastic#92840)
  Sort dependencies in package.json correctly (elastic#93590)
  [Bug] missing timepicker:quickRanges migration (elastic#93409)
  ...
Kerry350 added a commit that referenced this pull request Mar 4, 2021
* Use ReplaySubject and amend date comparisons

* Assess date range expressions separately

* Only add dataset filter to view in stream links if one exists
Kerry350 added a commit that referenced this pull request Mar 4, 2021
* Use ReplaySubject and amend date comparisons

* Assess date range expressions separately

* Only add dataset filter to view in stream links if one exists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Logs UI Logs UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.12.0 v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants