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.

Sign up for GitHub

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

Jump to bottom

[Infra UI] Add Docker section to node details page #43627

Merged
merged 16 commits into from
Sep 27, 2019
Merged

[Infra UI] Add Docker section to node details page #43627

merged 16 commits into from
Sep 27, 2019

Conversation

simianhacker
Copy link
Member

@simianhacker simianhacker commented Aug 20, 2019 •
edited
Loading

Summary

This PR adds a Docker section the node details page.

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

@simianhacker simianhacker added release_note:enhancement Feature:Metrics UI Metrics UI feature v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.5.0 labels Aug 20, 2019
@simianhacker simianhacker self-assigned this Aug 20, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-logs-ui

@simianhacker
Copy link
Member Author

@sorantis I wanted to add a Docker section to the node details page. This is the only metric I could find that made sense. Can you think of any others? I was hoping to show how much CPU and Memory Docker consumed on this host but there doesn't seem to be an easy way to get that. I tried to get the information from the container metrics but there isn't a clean way to do that without having double counting.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorantis
Copy link

sorantis commented Aug 21, 2019 •
edited
Loading

@simianhacker The overview and container states information looks good.
What do you think of these:

  • average network rx and tx rates per container image. Should be easy to do according to our docs.
  • A list of the most traffic intensive containers (around 5 should be ok, we can play with the amount that nicely fits the layout).
  • CPU user/system by image. Docs
  • A list of the most CPU heavy containers (also 5).
  • Memory RSS by image. Docs
  • A list of the most memory intensive containers (also 5 should be ok).

Bonus?

  • top 5 containers with highest uptime
  • top 5 mostly used images

@simianhacker
Copy link
Member Author

simianhacker commented Aug 21, 2019 •
edited
Loading

@sorantis What if we started out with Top 5 Containers by CPU and Top 5 Containers by Memory?

image

I'm starting to think we might want to move from sections in the side navigation to tabs along the top. I believe @hbharding is looking into this for the APM PR (#42710). I like the idea of some of the other sections you mentioned with lists but we don't have the facilities to do that right now. I think if we decided to move to tabs instead of sections in the navigation, then we could make a much richer custom experience instead of just a page full of charts.

@simianhacker
Add chart with terms aggregation for top 5 CPU and Memory
812db4c 
- Add label to series returned from metrics endpoint
- Honor seriesOverride name or use label
- Fix chart tooltip for long names
- Add name to seriesOverrides for current layouts
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker simianhacker marked this pull request as ready for review August 22, 2019 16:50
@simianhacker simianhacker requested review from a team as code owners August 22, 2019 16:50
@sorantis
Copy link

sorantis commented Aug 23, 2019 •
edited
Loading

@simianhacker You're right, I couldn't find any Docker specific metrics in the module.
The charts are fine, imho the user can already see this information from a waffle on the Inventory page.
Screen Shot 2019-08-22 at 10 07 03

How does the user reach the Docker details this page?

Network rx/tx charts can be added to the Container details page.
Screen Shot 2019-08-23 at 16 54 46

+1 on improving navigation on this page (hope the tabs should enable a richer UX). The details page will also benefit from being more interactive.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker
Copy link
Member Author

@sorantis For the actual Docker node detail page (for a container), we already have sections for Disk IO and Network Traffic. The issue right now is that there isn't any data in the Observability demo cluster for docker.network and docker.diskio event types so they don't show up.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@simianhacker
Copy link
Member Author

@sorantis Where are we with this PR? Do feel it's ready from a feature perspective to merge? If so then I can push to have an engineer review the technical parts.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorantis
Copy link

@simianhacker LGTM. We can revisit the remaining proposed sections when we've resolved the limitations related to the navigation. Has there any further discussion regarding moving to tabs instead of sections in the navigation?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Titch990 Titch990 mentioned this pull request Sep 13, 2019
Closed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Kerry350 Kerry350 self-requested a review September 26, 2019 15:56
Kerry350
Kerry350 approved these changes Sep 26, 2019
View reviewed changes
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

Left a few small comments, but otherwise it seems to work as intended 👍

x-pack/legacy/plugins/infra/public/components/metrics/sections/chart_section.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/infra/public/components/metrics/sections/helpers/index.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/infra/server/lib/adapters/metrics/models/host/host_docker_top_5_by_cpu.ts Outdated Show resolved Hide resolved
.../legacy/plugins/infra/server/lib/adapters/metrics/models/host/host_docker_top_5_by_memory.ts Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

hbharding
hbharding approved these changes Sep 26, 2019
View reviewed changes
Copy link
Contributor

@hbharding hbharding left a comment

Choose a reason for hiding this comment

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

lgtm

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@simianhacker simianhacker merged commit 6f1a326 into elastic:master Sep 27, 2019
simianhacker added a commit to simianhacker/kibana that referenced this pull request Sep 27, 2019
@simianhacker
[Infra UI] Add Docker section to node details page (elastic#43627)
223d3f9 
* [Infra UI] Add Docker section to node detail page

* Add chart with terms aggregation for top 5 CPU and Memory

- Add label to series returned from metrics endpoint
- Honor seriesOverride name or use label
- Fix chart tooltip for long names
- Add name to seriesOverrides for current layouts

* Fixing types

* removing point changes, handled in seperate pr

* Changing infMetricsExplorerChart className to infrastructureChart

* updating comment with new functionality

* updating id to human readable string
@simianhacker simianhacker mentioned this pull request Sep 27, 2019
Merged
simianhacker added a commit that referenced this pull request Sep 27, 2019
@simianhacker
[Infra UI] Add Docker section to node details page (#43627) (#46825)
6d5824e 
* [Infra UI] Add Docker section to node detail page

* Add chart with terms aggregation for top 5 CPU and Memory

- Add label to series returned from metrics endpoint
- Honor seriesOverride name or use label
- Fix chart tooltip for long names
- Add name to seriesOverrides for current layouts

* Fixing types

* removing point changes, handled in seperate pr

* Changing infMetricsExplorerChart className to infrastructureChart

* updating comment with new functionality

* updating id to human readable string
@Titch990 Titch990 mentioned this pull request Nov 11, 2019
Closed
@simianhacker simianhacker deleted the add-docker-section branch April 17, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kerry350 Kerry350 Kerry350 approved these changes

@hbharding hbharding hbharding approved these changes

Assignees

@simianhacker simianhacker

Labels
Feature:Metrics UI Metrics UI feature release_note:enhancement Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.5.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@simianhacker @elasticmachine @sorantis @Kerry350 @hbharding