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] Fetch single log entries via a search strategy #81710

Merged

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Oct 27, 2020

Summary

This replaces the log item API with a single-log-entry search strategy. This is used to fetch the data for display in the details flyout. There should be no significant visual difference to the user.

Request cancellation support will be added in a follow-up PR.

closes #78001

Implementation details

I've commented inline on various aspects of the implementation.

  • The search strategy internally delegates to the ese search strategy to fetch the actual data.
  • This introduces a server-side service directory structure to match the platform conventions regarding plugin substructure:
    • a LogEntrySearchStrategy
    • a LogEntriesService, which registers the above search strategy
  • The typed_search_strategy.ts utilities are designed to be re-usable in upcoming search-strategy-related PRs.

Previews

image

Follow-up tasks

@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.11.0 labels Oct 27, 2020
@weltenwort weltenwort added this to the Logs UI 7.11 milestone Oct 27, 2020
@weltenwort weltenwort self-assigned this Oct 27, 2020
@afgomez afgomez self-requested a review November 3, 2020 10:48
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 did a more functional pass today. Overall it works as intended. However I found two things.

  • The first is what I commented on yesterday. When using the dev cluster it tries to load indefinitely, then it fails silently. The error seems to be on the ES side, but it's not being handled in the UI.

Screenshot 2020-11-25 at 13 33 22

  • The second: the fields are no longer displayed shorted.

Before:
Screenshot 2020-11-25 at 12 06 47

After:
Screenshot 2020-11-25 at 11 57 37

@weltenwort
Copy link
Member Author

weltenwort commented Nov 26, 2020

Thank you for the review ❤️

When using the dev cluster it tries to load indefinitely, then it fails silently. The error seems to be on the ES side, but it's not being handled in the UI.

This is caused by the fields: ['*'] bug, that you noticed a few weeks ago. It's fixed by now, but the cluster is stuck on an old version. I developed against a local cluster.

the fields are no longer displayed shorted

Good catch, I'll fix that.

@weltenwort
Copy link
Member Author

weltenwort commented Nov 26, 2020

@afgomez, I replaced the EuiBasicTable with an EuiInMemoryTable, which gives us sorting without extra effort. Because it was so easy I also enabled the search box above the table so the fields can be filtered interactively:

image

I'd like to tackle the error display in #83906, because it completely replaces the current client-side code that calls the search strategy. Would that work for you?

@afgomez
Copy link
Contributor

afgomez commented Dec 2, 2020

I replaced the EuiBasicTable with an EuiInMemoryTable, which gives us sorting. [...] I also enabled the search box above the table so the fields can be filtered interactively.

Nice! I tried the filter and it's very useful :)

I noticed there are two fields missing: _id and _index.

Screenshot 2020-12-02 at 15 25 02

We added them manually in the API. I see the _index is also present as index but there's no corresponding id field.

Maybe it makes more sense to show the ID somewhere else in the UI, like in the header:

Screenshot 2020-12-02 at 15 31 42

@afgomez
Copy link
Contributor

afgomez commented Dec 2, 2020

When using the dev cluster it tries to load indefinitely, then it fails silently. The error seems to be on the ES side, but it's not being handled in the UI.

This is caused by the fields: ['*'] bug, that you noticed a few weeks ago.

My comment was more related to the UI not handling the error. I'm not sure we do any error handling in master either, so maybe we can tackle that on a separate PR.

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.

Everything works 🎉! There is the question of what to do with the added _id and _index fields. I'm not explicitly approving to keep track of that, but consider it virtually approved :)

@weltenwort
Copy link
Member Author

Maybe it makes more sense to show the ID somewhere

Good point, I forgot about rendering id and index. I'll try to find a place to display them in the flyout. 👍

The errors were indeed silent even before this change. I'm cleaning that up in #83906, which will be ready for review soon.

@weltenwort
Copy link
Member Author

@afgomez I think the header works reasonably well:

image

What do you think?

id="xpack.infra.logFlyout.flyoutTitle"
values={{
logEntryId: flyoutItem ? <code>{flyoutItem.id}</code> : '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use <EuiCode> here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that, but decided against it, because <EuiCode> is a pretty complex component that performs language-specific syntax highlighting on the content. It felt like overkill here, since we're not displaying code but only want to stylize the immutable values.

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 think the header works reasonably well

Yeah I think so too :) I would consider using <EuiCode> instead of <code>, but other than that LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1051 1054 +3

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.6MB 2.6MB +4.2KB

Distributable file count

id before after diff
default 43460 43466 +6

Page load bundle

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

id before after diff
infra 181.0KB 182.1KB +1.1KB

History

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

@weltenwort weltenwort merged commit 3cb26eb into elastic:master Dec 3, 2020
@weltenwort weltenwort deleted the logs-ui-add-log-entry-search-strategy branch December 3, 2020 12:05
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 3, 2020
…overy-action-group

* upstream/master: (48 commits)
  [Lens] accessibility screen reader issues (elastic#84395)
  [Logs UI] Fetch single log entries via a search strategy (elastic#81710)
  fix: 🐛 don't add separator befor group on no main items (elastic#83166)
  [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323)
  [APM] Add APM agent config options (elastic#84678)
  Fixed a11y issue on rollup jobs table selection (elastic#84567)
  [Discover] Refactor getContextUrl to separate file (elastic#84503)
  [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654)
  [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749)
  [Lens] Migrate legacy es client and remove total hits as int (elastic#84340)
  Improve logging pipeline in @kbn/legacy-logging (elastic#84629)
  Catch @hapi/podium errors (elastic#84575)
  [Discover] Unskip date histogram test (elastic#84727)
  Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791)
  [Enterprise Search] Fix schema errors button (elastic#84842)
  [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589)
  [Maps] Always initialize routes on server-startup (elastic#84806)
  [Fleet] EPM support to handle uploaded file paths (elastic#84708)
  [Snapshot Restore] Fix initial policy form state (elastic#83928)
  Upgrade Node.js to version 14 (elastic#83425)
  ...
weltenwort added a commit that referenced this pull request Dec 3, 2020
… (#84890)

Backports the following commits to 7.x:
 - [Logs UI] Fetch single log entries via a search strategy (#81710)
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.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Create primary Logs search strategy and server side foundations
4 participants