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

[Infrastructure UI] Integrated the logs tab to the Hosts View #152995

Conversation

mohamedhamed-ahmed
Copy link
Contributor

@mohamedhamed-ahmed mohamedhamed-ahmed commented Mar 9, 2023

closes #957
closes #958
closes #959

Summary

This PR is for integrating the logs tab in the Hosts view and adding a search bar to allow further filtering the logs, it also enables navigating to the Logs stream view using an Open in logs link.

Demo

Screen.Recording.2023-03-20.at.11.40.29.mov

Next Steps

  1. Implementing Telemetry
  2. Functional Tests

@mohamedhamed-ahmed mohamedhamed-ahmed added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Feature:ObsHosts Hosts feature within Observability v8.8.0 labels Mar 9, 2023
@mohamedhamed-ahmed mohamedhamed-ahmed marked this pull request as ready for review March 21, 2023 13:51
@mohamedhamed-ahmed mohamedhamed-ahmed requested a review from a team as a code owner March 21, 2023 13:51
@elasticmachine
Copy link
Contributor

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

@mohamedhamed-ahmed mohamedhamed-ahmed self-assigned this Mar 21, 2023
@mohamedhamed-ahmed mohamedhamed-ahmed removed their assignment Mar 21, 2023
Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

Nice, It works fine 🙌 I added some questions/comments and just spotted some things that I am not sure should be covered here:

  • Should we support auto-complete like the other search bar because when I am typing I see this validation error message instead of a hostname suggestion (when I put the name it is showing the logs correctly)?
    image It is similar in inventory so I am not sure 🤔

  • Regarding the history issue we checked with the logs stream page: this will be something checked on the logs page separately right?

Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

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

Thanks for this change. I checked it and everything works as expected. I would only suggest - if possible - covering this new tab in our functional test in this PR.

Regarding the URL generated to link to the Logs UI, I was wondering if it would be possible to build the querystring to populate the unified search filters with the host names. I'm not sure if this is possible and/or if it's an easy change, but I think it would provide better UX if the user wants to remove a host from the list.

image

Another thing is that if you click on the browser back button, it doesn't return to the Hosts view. Apparently, the Logs UI adds items to the browser history - I guess that's what @jennypavlova mentioned above, right?

image

@mohamedhamed-ahmed
Copy link
Contributor Author

mohamedhamed-ahmed commented Mar 21, 2023

  • Should we support auto-complete like the other search bar because when I am typing I see this validation error message instead of a hostname suggestion (when I put the name it is showing the logs correctly)?

@jennypavlova I have already discussed this with Nathan and we agreed to keep it this way for now as much work in the logs is coming in the future.

  • Regarding the history issue we checked with the logs stream page: this will be something checked on the logs page separately right?

I am fixing this now and will be part of this PR, but yes it will be fixed in the logs page as discussed with Kerry

  • Another thing is that if you click on the browser back button, it doesn't return to the Hosts view. Apparently, the Logs UI adds items to the browser history - I guess that's what @jennypavlova mentioned above, right?

@crespocarlos yes its the same, I discussed it with Kerry and will get this fixed 👍

  • Thanks for this change. I checked it and everything works as expected. I would only suggest - if possible - covering this new tab in our functional test in this PR.

I was discussing this with Marco earlier and the plan was to cover it as part of this ticket.

@crespocarlos
Copy link
Contributor

I was discussing this with Marco earlier and the plan was to cover it as part of this #147691 (comment).

Right! The downside is that we'll be vulnerable to changes that could potentially cause problems with this feature. As long as it's mapped there it's ok :). But to avoid adding items to that list of debts, if the feature is done and no further development is needed, I'd consider including tests in the PR.

Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! I ran another round of tests and noticed a couple of things, mostly questions about how the tab should behave in certain scenarios.

@mohamedhamed-ahmed
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

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

Just a few nits that I didn't notice in my previous reviews, but the PR looks good.

@mohamedhamed-ahmed
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1282 1291 +9

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.3MB 1.3MB +6.1KB
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for this change and applying the suggestions :)

@mohamedhamed-ahmed mohamedhamed-ahmed merged commit cd96ab4 into elastic:main Mar 27, 2023
@mohamedhamed-ahmed mohamedhamed-ahmed deleted the 957-integrate-logs-tab-in-hosts-view branch March 27, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:ObsHosts Hosts feature within Observability release_note:feature Makes this part of the condensed release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants