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] Hosts: Convert existing KPI summary metric charts to LensEmbeddable counterparts #150550

Closed
formgeist opened this issue Feb 8, 2023 · 13 comments · Fixed by #154903
Assignees
Labels
beta Required for a feature to move to beta enhancement New value added to drive a business result Feature:Metrics UI Metrics UI feature Feature:ObsHosts Hosts feature within Observability Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services

Comments

@formgeist
Copy link
Contributor

formgeist commented Feb 8, 2023

Related tickets

Summary

We have plenty of incentives for adopting the platform components such as LensEmbeddables because they offer lots of options for the user to further explore the data that the visualizations represent. This proposes to replace our existing KPI summary metric charts with LensEmbeddables.

CleanShot 2023-02-08 at 13 26 16@2x

@formgeist formgeist added enhancement New value added to drive a business result Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Epic: Host Observability Feature:ObsHosts Hosts feature within Observability labels Feb 8, 2023
@elasticmachine
Copy link
Contributor

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

@crespocarlos
Copy link
Contributor

This is blocked by #150600.

@crespocarlos
Copy link
Contributor

I've added this ticket as part of the performance improvement because based on some tests I've run, Lens way to request the data is better than what we doing now: more details https://github.com/elastic/obs-infraobs-team/issues/909#issuecomment-1429961510

@crespocarlos
Copy link
Contributor

@neptunian since we're not going to use counter_rate with the hosts field, we might have to create a new ticket for Lens team if the formulas for RX and TX using hosts field don't work.

@crespocarlos
Copy link
Contributor

cc @roshan-elastic

@neptunian found out that we are using the wrong fields in the RX and TX metric charts: ( #150600), and we tried to build the KPI tiles for RX and TX using the correct hosts.network fields.

The good news is that we managed to build them using lens:

Image

Image

The bad new is that the average on the bottom right side is wrong (as you can see in the second screenshot). So we're now partially blocked by this ticket instead of the one mentioned previously.

I think we can start migrating the KPIs, but we can't release them with this bug.

@roshan-elastic roshan-elastic added the beta Required for a feature to move to beta label Mar 13, 2023
@roshan-elastic
Copy link

Cheers for being proactive here @crespocarlos .

Couple of Qs:

  1. Is that the only ticket which blocks us from migrating to lens?
  1. What's the impact if we don't move to Lens for KPI tiles? I'm sure you did some analysis but will it likely considerably slow down the page?

If we find we need this for the Improve Hosts Feature UI Responsiveness with enterprise-level datasets epic to be successful, I can push it with the Lens to try and get it prioritised for 8.8..

If they can't do it, we'll either have to:

(a) Migrate to Lens but work around this (e.g. could we change the metrics being shown whilst we wait for the issue to be resolved...
(b) Live with the current behaviour (i.e. non-lens)
(c) Remove the KPI tiles altogether

Would you mind:

  1. Confirming if this the only blocker to moving to Lens?
  2. Confirming the performance impact of not migrating to Lens (sorry, I think you've already done this previously)?

Depending on the answers, we can perhaps discuss with @neptunian on whether we go for (a), (b) or (c).

@crespocarlos
Copy link
Contributor

@roshan-elastic

Is that the only ticket which blocks us from migrating to lens?
#152014

The ticket you've mentioned is a bug. I'd consider it to be a blocker too.

What's the impact if we don't move to Lens for KPI tiles? I'm sure you did some analysis but will it likely considerably slow down the page?

In the tests I've run, Lens performed better in general. I wouldn't say that not migrating to Lens will considerably slow down the page. We can continue to use the current custom API, if Lens team doesn't prioritize them for 8.8, making a couple of adjustments on the frontend to try to improve the performance of the KPIs.

@roshan-elastic
Copy link

Hey @crespocarlos, thanks for that.

The ticket you've mentioned is a bug. I'd consider it to be a blocker too.
Just to confirm I understand correctly, this is the only blocker? If not, just let me know if there are any other blockers.

In the tests I've run, Lens performed better in general. I wouldn't say that not migrating to Lens will considerably slow down the page. We can continue to use the current custom API, if Lens team doesn't prioritize them for 8.8, making a couple of adjustments on the frontend to try to improve the performance of the KPIs.

Cool - that'll help with prioritisation in the Lens team for the following ticket:

If I remember right, I think you had some empirical data on how much switching the KPI tiles to lens would improve performance?

Would you mind adding that to the bug ticket to help @ninoslavmiskovic prioritise accordingly? He has a lot of tickets so adding concise description/data of the expected performance increase will help him prioritise.

@crespocarlos crespocarlos self-assigned this Apr 5, 2023
@crespocarlos
Copy link
Contributor

There are still some missing details in Lens' Metric chart. I opened a ticket proposing a few changes

#154529

@roshan-elastic , I think this is a blocker because the charts built with Lens won't look the same as the current KPI charts. What do you think?

@roshan-elastic
Copy link

Hey @crespocarlos - as discussed:

  • We can add the (average) to the chart title)
  • We can update the tooltips as discussed to mention the metric is PBit/s (i.e. we show the suffix here)
  • We can ensure the KPI tiles have a (?) overlay to make it clear there are tooltips (ideally, we'd show the tooltip as soon as you hover on the (?)

Once the following issue is resolved, we can add in an issue to update these KPI tiles:

(feel free to add an issue yourself and just do it if you notice the above is completed)

@crespocarlos
Copy link
Contributor

crespocarlos commented Apr 17, 2023

@roshan-elastic

We can update the tooltips as discussed to mention the metric is PBit/s (i.e. we show the suffix here)

They do mention it already: "Number of bytes which have been received per second on the public interfaces of the hosts."

We can ensure the KPI tiles have a (?) overlay to make it clear there are tooltips (ideally, we'd show the tooltip as soon as you hover on the (?)

Based on [Infrastructure UI] Hosts: Show icon and tooltip in the Metrics component summary#150318. I would leave the icons out for now. wdyt?

@roshan-elastic
Copy link

Hey @crespocarlos,

Based on #150318. I would leave the icons out for now. wdyt?
Yeah, let's leave it for now.

Let me know once you feel the KPI tiles are ready and I can feed back before you submit a PR.

Shout if you need anything though.

@roshan-elastic
Copy link

Completed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Required for a feature to move to beta enhancement New value added to drive a business result Feature:Metrics UI Metrics UI feature Feature:ObsHosts Hosts feature within Observability Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants