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

[Infra UI] Add tooltips to Asset Details #164858

Conversation

jennypavlova
Copy link
Member

@jennypavlova jennypavlova commented Aug 25, 2023

Closes #164594

Summary

This PR adds tooltips to explain the time range used to collect process data and metadata. The tooltips will be shown inside the overview (metadata summary section) tab, metadata tab, and processes tab (both flyout and full page views).

Fixes

I saw that the icons showing different explanation/ docs links inside the overveiew tab are not consistent ( sometimes we have questionInCircle and sometimes iInCircles) - the designs are using iInCircles so I changed it everywhere:
icons

Testing

  1. Go to host view and open the flyout for any host. The new explanation and tooltips should be shown in:

    • Overview tab
    overview
    • Metadata tab
    metadata
    • Processed tab
    processes
  2. Click Open as page and check the same on the asset details page

Screenshot 2023-08-28 at 11 28 47 Screenshot 2023-08-28 at 11 28 32 Screenshot 2023-08-28 at 11 28 16

@jennypavlova jennypavlova added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Aug 25, 2023
@jennypavlova jennypavlova self-assigned this Aug 25, 2023
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@jennypavlova jennypavlova marked this pull request as ready for review August 28, 2023 10:30
@jennypavlova jennypavlova requested a review from a team as a code owner August 28, 2023 10:30
@elasticmachine
Copy link
Contributor

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

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.

Nice 🚀 ! The tooltips are working as expected. Just left one comment concerning the metadata timestamp, but, overall, it looks good.

Comment on lines 56 to 59
const { getDateRangeInTimestamp } = useDateRangeProviderContext();
const dateFromRange = new Date(getDateRangeInTimestamp().to);
const dateString = dateFromRange.toLocaleDateString();
const timeString = dateFromRange.toLocaleTimeString();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is OK (I think), but it's not entirely true. The true timestamp of the latest metadata info, should come from the /api/infra/metadata endpoint.

The query is here, adding TIMESTAMP_FIELD to _source and propagating it in the endpoint response will give the FE the right information.

Both values (metadata API timestamp and date picker timestamp) will be pretty close if the period in metricbeat is set to 10s, but I suspect that they might diverge if the period in metricbeat is set to 1m.

Might be worth testing it.

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 am not sure if I understand this issue: I checked the request and the date/time shown in there and we have the to matching with the date shown:
image
In my metricbeat config I have:

  - module: system
    metricsets:
        ............
    period: 1m
    processes: [".*"]

So I saw that we pass that time range when we do the request to get the metadata as this is the to date should we have "before" {date/time} not "on" {date/time} in the text to be clear as it's at the end of the range 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

What you're seeing in your screenshot is the request payload? We pass that to the API, which runs a query using that range and returns that top 1 desc document.

This top 1 desc doc timestamp could be the same or pretty close to to. But if the last document ingested was 30 seconds before to, then there should be a small difference between that and to.

Not sure if it's clear. Happy to discuss over slack.

@crespocarlos
Copy link
Contributor

One thing I just realized:

image

Date formats can vary, I wonder if we should print it as DD-Mon-YYYY ex 07 Jul, 2023 , to avoid confusion

@jennypavlova
Copy link
Member Author

One thing I just realized:

image Date formats can vary, I wonder if we should print it as `DD-Mon-YYYY` ex `07 Jul, 2023` , to avoid confusion

I followed the design on that - I used toLocaleDateString to make sure that the local format would be displayed. Maybe if we change the format we should also change the text to for example 07 Jul, 2023 @ 11:11 the time should be also set based on the locale time like it is now ( showing 15 or 3 pm for example) 🤔 Maybe it's a design/UX question how it will be easier for the user but I saw in other places (Discover) we have that format (07 Jul, 2023 @ 11:11) so I will change that to be consistent.

@jennypavlova
Copy link
Member Author

@crespocarlos So I updated the date format and added the missing seconds to the time value and now it should match the datapicker and also should be localized as I used the i18n formatter. Wdyt?
date_time

@@ -83,6 +83,7 @@ export const InfraMetadataInfoRT = rt.partial({
cloud: InfraMetadataCloudRT,
host: InfraMetadataHostRT,
agent: InfraMetadataAgentRT,
'@timestamp': rt.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: would it be better if the API returned timestamp without the @?

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the difference there - Is it because of the usage of metadata?.info?.['@timestamp']? Where do we want to rename it - if I change the type the response should also change the _source and there I should use the @timestamp. So after changing this type, I need to have a different type here in the response and then map the response to the timestamp key using the @timestamp value from the response. I don't see how changing this type will fix all that, it looks more confusing to use the timestamp mapped to the @timestamp and have 2 types for the response 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

A new type was added as discussed (commit)

@crespocarlos
Copy link
Contributor

crespocarlos commented Aug 29, 2023

Thanks for making the changes @jennypavlova.

I've noticed a couple of things. On the metadata, while the page waits for the endpoint to respond, the date/and time shows the value from the date picker.

metadata_loading.mov

Also on the processes tab, when looking at a date range where there is no data, we are showing the message. I now I'm starting to think that it's confusing. We might want to double check that with @roshan-elastic and @kkurstak

image

It seems like we shouldn't show the message + tooltip when there is data to be in those tabs. Wdyt?

@jennypavlova
Copy link
Member Author

@crespocarlos Thanks for checking that!
For the metadata loading case, we can solve that with a spinner while loading like the summary values, for example, wdyt?
Yes for the processes no data case, this message looks strange but the tooltip information can be still useful to the user as it includes documentation links:
image
So I am not sure if hiding it should be the solution for this 🤔

@roshan-elastic
Copy link

Also on the processes tab, when looking at a date range where there is no data, we are showing the message. I now I'm starting to think that it's confusing. We might want to double check that with @roshan-elastic and @kkurstak

@crespocarlos @kkurstak - Maybe we should just remove the 'Showing process data...' message completely if no processes are found? Maybe it's helpful to show it though?

@kkurstak - what's your initial reaction?

@jennypavlova
Copy link
Member Author

So regarding the metadata loading, I added a spinner, it will look like this:

metadata_loading_spinner.mov

@jennypavlova
Copy link
Member Author

The processes message is now displayed only if we have data and no error:
image

image

Regarding the metadata integration test that is currently failing - I am getting different dates locally and in the ci ( It should be the same data I think but I got some failed tests because of that here.
What I did there is check if the timestamps are inside the range and separately check the host, agent, and cloud properties so that way the timestamp won't affect the results and make it easy to fail in this commit
cc: @crespocarlos

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.

hey @jennypavlova , there are a couple of things I'd like to get your opinion on:

  1. When looking at a future date, the metadata tooltip is shown with the date picker's end date. Should we hide when there is no data too?
image
  1. Should there be a loading indicator for the Processes tab's tooltip?
processes_loading_indicator.mov

@roshan-elastic
Copy link

The processes message is now displayed only if we have data and no error: image

image Regarding the metadata integration test that is currently failing - I am getting different dates locally and in the ci ( It should be the same data I think but I got some failed tests because of that [here](https://buildkite.com/elastic/kibana-pull-request/builds/154581#018a41f3-f773-4074-837a-58ccd2d63b68). What I did there is check if the timestamps are inside the range and separately check the `host`, `agent`, and `cloud` properties so that way the timestamp won't affect the results and make it easy to fail in this [commit](https://github.com//pull/164858/commits/711c5b1f34cf10aa6795b3d8dbd2bd955dead053) cc: @crespocarlos

This is great @jennypavlova - I like your initiative and thinking here. Why do we need me? 🤣

@jennypavlova
Copy link
Member Author

Hey @crespocarlos, I mentioned you there to help with the code review in case you are wondering what the issue was and why I am changing it and to hear what you think about it :)

@jennypavlova
Copy link
Member Author

@crespocarlos Thank you for the review!

  1. When looking at a future date, the metadata tooltip is shown with the date picker's end date. Should we hide when there is no data too?

It makes sense to check the metadata before showing the tooltip - In this case now that we have timestamp there we can check directly the property - it won't be there when the metadata.info is empty :)

  1. Should there be a loading indicator for the Processes tab's tooltip?

Added, thank you for finding that!

Maybe we should just remove the 'Showing process data...' message completely if no processes are found?

@roshan-elastic Thanks for the input! We did that and added similar logic for the metadata 🙂

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 all the improvements.

@jennypavlova jennypavlova enabled auto-merge (squash) August 30, 2023 16:20
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #63 / Alerting aggregate "after each" hook for "should aggregate alert status totals"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1428 1430 +2

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.0MB 2.0MB +5.1KB

History

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

cc @jennypavlova

@jennypavlova jennypavlova merged commit a511426 into elastic:main Aug 30, 2023
@jennypavlova jennypavlova deleted the 164594-infra-ui-add-tooltips-to-asset-details branch August 31, 2023 09:49
eokoneyo pushed a commit to eokoneyo/kibana that referenced this pull request Aug 31, 2023
Closes elastic#164594
## Summary

This PR adds tooltips to explain the time range used to collect process
data and metadata. The tooltips will be shown inside the overview
(metadata summary section) tab, metadata tab, and processes tab (both
flyout and full page views).

## Fixes

I saw that the icons showing different explanation/ docs links inside
the overveiew tab are not consistent ( sometimes we have
`questionInCircle` and sometimes `iInCircles`) - the designs are using
`iInCircles` so I changed it everywhere:
<img width="671" alt="icons"
src="https://github.com/elastic/kibana/assets/14139027/edda271b-5030-4d83-9722-448fbae8cf8b">


## Testing
1. Go to host view and open the flyout for any host. The new explanation
and tooltips should be shown in:
   - Overview tab
   
<img width="938" alt="overview"
src="https://github.com/elastic/kibana/assets/14139027/ac4ae369-d825-4fba-8865-c9410de29c28">

   - Metadata tab
   
<img width="945" alt="metadata"
src="https://github.com/elastic/kibana/assets/14139027/4d174bf3-3411-40a5-a208-eb5df2266c61">

   - Processed tab
   
<img width="937" alt="processes"
src="https://github.com/elastic/kibana/assets/14139027/11d32c66-4a25-4fce-a95e-42698f2e1682">
2. Click Open as page and check the same on the asset details page 


<img width="1653" alt="Screenshot 2023-08-28 at 11 28 47"
src="https://github.com/elastic/kibana/assets/14139027/342d1565-bb51-4961-b8ac-8b8270c851ff">
<img width="1637" alt="Screenshot 2023-08-28 at 11 28 32"
src="https://github.com/elastic/kibana/assets/14139027/63b66a12-d634-4ecc-83de-ad1e1b79334c">
<img width="1649" alt="Screenshot 2023-08-28 at 11 28 16"
src="https://github.com/elastic/kibana/assets/14139027/59720bf3-88ad-44b1-8584-769b38d956ed">

---------

Co-authored-by: Kibana Machine <[email protected]>
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 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 v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infra UI] Add tooltips to Asset Details
7 participants