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] Load <LogStream> entries via async searches #86899

Merged

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Dec 23, 2020

Summary

This PR replaces the usage of plain HTTP routes to load the log stream entries with async search strategy calls.

related to #86439

Implementation notes

The changes in this PR can be categorized roughly into one of these categories:

  • remove unused GraphQL types
  • make common log entry types http-independent
  • implement server-side "log entries" search strategy
  • improve client-side hooks to work with observables and the data plugin's search functionality
  • implement client-side hooks to load before, after and around a cursor
  • use these hooks in the useLogStream hook, which is called by the "view-in-context" and embeddable component
  • adapt tests and storybooks to the new hooks

To the user nothing really changes, except that running requests are now cancelled if the <LogStream /> component is unmounted because the "view-in-context" modal is closed or the APM trace log tab is hidden. The progress indicators and cancellation buttons will improve the UX later (see follow-ups below).

Follow-up tasks

For the sake of limiting the scope of this PR still leaves many of the ACs of #86439 and #86441 to follow-up PRs:

  • use the search strategy in the main log stream
  • create and use a search strategy to load the minimap summary
  • removal of unused HTTP routes afterwards
  • show async loading progress indicators
    • for loading the stream initially
    • for loading more "before" and "after"
  • offer manual cancellation controls for the above

@weltenwort weltenwort added 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 Dec 23, 2020
@weltenwort weltenwort added this to the Logs UI 7.12 milestone Dec 23, 2020
@weltenwort weltenwort self-assigned this Dec 23, 2020
@weltenwort weltenwort force-pushed the logs-ui-async-log-entries-search branch 2 times, most recently from 893f2c2 to 96cd32a Compare January 11, 2021 15:57
@weltenwort weltenwort force-pushed the logs-ui-async-log-entries-search branch 2 times, most recently from 76451b9 to 05b55aa Compare January 15, 2021 18:20
@weltenwort weltenwort force-pushed the logs-ui-async-log-entries-search branch 2 times, most recently from 34a4a34 to a855f10 Compare January 26, 2021 12:15
@weltenwort weltenwort changed the title [Logs UI] Load log stream entries and summary via async searches [Logs UI] Load <LogStream> entries via async searches Jan 27, 2021
@weltenwort weltenwort marked this pull request as ready for review January 27, 2021 15:08
@weltenwort weltenwort requested a review from a team as a code owner January 27, 2021 15:08
@elasticmachine
Copy link
Contributor

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

@weltenwort weltenwort added the release_note:skip Skip the PR/issue when compiling release notes label Jan 27, 2021
@afgomez afgomez self-requested a review January 28, 2021 15:43
@weltenwort
Copy link
Member Author

⌛ resolving merge conflicts...

Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

I downloaded the branch and everything seems to work correctly and code LGTM. I want to run two things with you (which can be addressed in a different PR).


The first thing is about the useFetchLogEntries {Before,After,Around} hook separation. Is the intention to expose them to other possible consumers? If so, I wonder if they could be unified somehow. I don't see scenarios where the hooks are not used together and right now we have some duplication to initialise them.

const {
  fetchLogEntriesBefore,
  isRequestRunning: isLogEntriesBeforeRequestRunning,
  logEntriesBeforeSearchResponse$,
} = useFetchLogEntriesBefore({
  sourceId,
  startTimestamp,
  endTimestamp,
  query: parsedQuery,
  columnOverrides: columns,
});

const {
  fetchLogEntriesAround,
  isRequestRunning: isLogEntriesAroundRequestRunning,
  logEntriesAroundSearchResponses$,
} = useFetchLogEntriesAround({
  sourceId,
  startTimestamp,
  endTimestamp,
  query: parsedQuery,
  columnOverrides: columns,
});

const {
  fetchLogEntriesAfter,
  isRequestRunning: isLogEntriesAfterRequestRunning,
  logEntriesAfterSearchResponse$,
} = useFetchLogEntriesAfter({
  sourceId,
  startTimestamp,
  endTimestamp,
  query: parsedQuery,
  columnOverrides: columns,
});

If we could unify them, the API becomes simpler.

const {
  fetchLogEntries,
  isRequestRunning: isLogEntriesRequestRunning,
  logEntriesSearchResponse$,
} = useFetchLogEntries({
  sourceId,
  startTimestamp,
  endTimestamp,
  query: parsedQuery,
  columnOverrides: columns,
});

fetchLogEntries({ before: 'last' })
fetchLogEntries({ after: 'first' })
fetchLogEntries({ center/around: {...} })

The second comment is about the stateful (for the lack of a better word) nature of the hook. We pass the source, timestamp, query and columns on hook creation, and the cursor when calling the callback. This means that if any value changes (not uncommon) the hook gets recreated again. Would it make sense to make the hook stateless and pass everything on the callback?

const {
  fetchLogEntries,
  isRequestRunning: isLogEntriesRequestRunning,
  logEntriesSearchResponse$,
} = useFetchLogEntries();

fetchLogEntries({
  sourceId,
  startTimestamp,
  endTimestamp,
  query: parsedQuery,
  columnOverrides: columns,
  before: 'last',
})

I think this only matters if the hooks will be used more publicly (i.e. in other hooks that we own). I understand this might make handling the center/around use case harder.

@weltenwort
Copy link
Member Author

You're making good points, @afgomez.

If so, I wonder if they could be unified somehow. I don't see scenarios where the hooks are not used together and right now we have some duplication to initialise them.

While I don't particularly favor the duplication either, I can think of a few reason to keep them separate:

  1. The request priorities differ in that the useLogEntriesAround cancels any running request when a new one is started while the before and after variants ignore new requests until the previous ones are finished. That corresponds to the semantics we currently need in the UI.
  2. Having the responses in three different observables allows us to threat them differently: to replace the entries for around, to prepend to the current entries for before or to append for after. One handler for all three would be a lot more convoluted.
  3. Handling the request observables separately means we can have before and after running in parallel, which also matches the semantics we need in the UI. A window resize, for example, could trigger loading in both directions.
  4. The request flow of the around variant is significantly different, because it composes before and after in a almost-but-not-quite sequential fashion. One big hook that handles all three cases feels like mixing too many different things.

I tried to factor out the common aspects of three variants to shared code paths. It's not ideal but optimizes for write-once/read-many-times to an acceptable degree IMHO. 🤔 But maybe I'm missing the obvious unifying approach, so I'm open for specific suggestions.

Would it make sense to make [...] pass everything on the callback?

That was indeed an almost arbitrary choice. I tried to separate the arguments into those that likely won't change with every request and those that likely will. My criterion was: "Assuming the fetch function gets passed on to another component to call once or multiple times at some later point, which arguments should be hidden from that component and which would it want to pass?"

But if we realize that this decision doesn't match the usage in the future we could obviously revise it at any time. Does that make sense?

@weltenwort
Copy link
Member Author

@afgomez, just having written the above response the following idea came to mind: The arguments could easily be made less repetitious by factoring them out as in:

const commonFetchArguments = {
  sourceId,
  startTimestamp,
  endTimestamp,
  query: parsedQuery,
  columnOverrides: columns,
};

const {
  fetchLogEntriesBefore,
  isRequestRunning: isLogEntriesBeforeRequestRunning,
  logEntriesBeforeSearchResponse$,
} = useFetchLogEntriesBefore(commonFetchArguments);

const {
  fetchLogEntriesAround,
  isRequestRunning: isLogEntriesAroundRequestRunning,
  logEntriesAroundSearchResponses$,
} = useFetchLogEntriesAround(commonFetchArguments);

const {
  fetchLogEntriesAfter,
  isRequestRunning: isLogEntriesAfterRequestRunning,
  logEntriesAfterSearchResponse$,
} = useFetchLogEntriesAfter(commonFetchArguments);

What do you think of that? Interestingly this also seems to validate my reasoning about dividing the arguments into the more general and more specific ones in the response above.

@afgomez
Copy link
Contributor

afgomez commented Feb 2, 2021

I can think of a few reason to keep them separate

I've read through them and I agree. Let's keep it like this :)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1101 1108 +7

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.2MB 2.2MB +29.8KB

Page load bundle

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

id before after diff
infra 172.1KB 172.3KB +213.0B

History

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

@weltenwort weltenwort merged commit 7fa30ba into elastic:master Feb 2, 2021
@weltenwort weltenwort deleted the logs-ui-async-log-entries-search branch February 2, 2021 14:42
@weltenwort
Copy link
Member Author

ℹ️ I'll wait for #89871 before I create the backport

@afgomez
Copy link
Contributor

afgomez commented Feb 2, 2021

ℹ️ I'll wait for #89871 before I create the backport

Merged!

weltenwort added a commit that referenced this pull request Feb 2, 2021
…90027)

Backports the following commits to 7.x:
 - [Logs UI] Load <LogStream> entries via async searches (#86899)
phillipb added a commit to phillipb/kibana that referenced this pull request Feb 2, 2021
…-ml-jobs

* 'master' of github.com:elastic/kibana: (254 commits)
  [Security Solution] [Detections] Remove allow_no_indices to prevent error being thrown in response of field capabilities (elastic#89927)
  Skip test for cloud (elastic#89450)
  [Fleet] Fix duplicate data streams being shown in UI (elastic#89812)
  Bump package dependencies (elastic#90034)
  [App Search] DRY helper for encoding/decoding routes that can have special characters in params (elastic#89811)
  TypeScript project references for Observability plugin (elastic#89320)
  [SearchSource] Combine sort and parent fields when serializing (elastic#89808)
  Made imports static (elastic#89935)
  [ml] migrate file_data_visualizer/import route to file_upload plugin (elastic#89640)
  [Discover] Adapt default column behavior (elastic#89826)
  Round start and end values (elastic#89030)
  Rename getProxyAgents to getCustomAgents (elastic#89813)
  [Form lib] UseField `onError` listener (elastic#89895)
  [APM] use latency sum instead of avg for impact (elastic#89990)
  migrate more core-owned plugins to tsproject ref (elastic#89975)
  [Logs UI] Load <LogStream> entries via async searches (elastic#86899)
  [APM] Abort browser requests when appropriate (elastic#89557)
  [Alerting] Allow user to select existing connector of same type when fixing broken connector (elastic#89062)
  [Data Table] Use shared CSV export mechanism (elastic#89702)
  chore(NA): improve logic check when installing Bazel tools (elastic#89634)
  ...
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: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 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants