Skip to content

Conversation

@abhi-kn
Copy link
Contributor

@abhi-kn abhi-kn commented Jan 14, 2020

Fixes:
https://issues.redhat.com/browse/ODC-2586

Analysis:
This US requires a monitoring tab to be displayed in topology side panel when a workload type resource is selected. Workload types supported in this PR are Deployment, DaemonSet & StatefulSet. Other type of workloads yet to be analysed w.r.t Prometheus support.

Solution Description:
Monitoring tab has been implemented in dev console & exposed through dev console plugin. Metrics section reuses MonitoringDashboardGraph component from /monitoring/dashboard to display graphs/plots. Prometheus queries used are yet to be reviewed by @christianvogt & Steve. Proposal doc for queries.
Note: post 4.4 feature freeze we would look into leveraging dashboard work done in console

Screen shots / Gifs for design review:
@openshift/team-devconsole-ux
Screenshot 2020-01-23 at 1 39 22 AM
Screenshot 2020-01-23 at 1 39 42 AM
Screenshot 2020-01-23 at 1 40 01 AM

Unit test coverage report:
Screenshot 2020-01-23 at 1 32 16 AM

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2020
@abhi-kn
Copy link
Contributor Author

abhi-kn commented Jan 14, 2020

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 14, 2020
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/dev-console Related to dev-console labels Jan 14, 2020
@abhi-kn
Copy link
Contributor Author

abhi-kn commented Jan 15, 2020

/assign @vikram-raj @invincibleJai

@abhi-kn
Copy link
Contributor Author

abhi-kn commented Jan 16, 2020

/retest

@vikram-raj
Copy link
Member

@abhi-kn Tests are failing because you didn't add my commit here. As it depends on this #3923

@abhi-kn abhi-kn force-pushed the ODC-2586 branch 2 times, most recently from 2331822 to 6e823ff Compare January 17, 2020 19:44
@openshift-ci-robot openshift-ci-robot added the component/sdk Related to console-plugin-sdk label Jan 17, 2020
@abhi-kn
Copy link
Contributor Author

abhi-kn commented Jan 18, 2020

/retest

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 20, 2020
@abhi-kn abhi-kn changed the title [WIP] added monitoring tab with metrics section added monitoring tab with metrics section Jan 20, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 20, 2020
Copy link
Member

Choose a reason for hiding this comment

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

I think the Gird layout is not needed here. Because anyway we are displaying a single graph in a row.

Copy link
Contributor

Choose a reason for hiding this comment

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

one component per file please

Copy link
Member

Choose a reason for hiding this comment

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

/cc @openshift/openshift-team-monitoring

Copy link

Choose a reason for hiding this comment

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

these seem pretty duplicate to the grafana dashboards we're embedding into the console, do we really want multiple pages that show the same data?

Copy link
Member

@invincibleJai invincibleJai Jan 22, 2020

Choose a reason for hiding this comment

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

@brancz considering feature freeze in 2 days we decided to continue with pre-canned queries for now. Post that will take a spike to see how we can leverage dashboard work done for monitoring using configmaps for Grafna cc @abhi-kn

https://coreos.slack.com/archives/CR8NP36LT/p1579709940016100?thread_ts=1579709940.016100&cid=CR8NP36LT

Copy link
Member

Choose a reason for hiding this comment

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

/cc @openshift/openshift-team-monitoring

@openshift-ci-robot openshift-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 22, 2020
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 22, 2020
@openshift-ci-robot openshift-ci-robot added the component/shared Related to console-shared label Jan 22, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a grid ? Seems like it's just stacking on top of each other. Also don't wrap single elements in a fragment.

@christianvogt
Copy link
Contributor

How often are we getting events that is causing the entire topology to rerender?
And how many events are being fetched in real world project?

@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2020
@invincibleJai
Copy link
Member

invincibleJai commented Jan 23, 2020

@christianvogt @abhi-kn have added below issues to track based on comments and showing just three visualization

cc @serenamarie125

updates extension support for sidebar
Copy link
Member

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

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

\lgtm

Copy link
Member

@vikram-raj vikram-raj left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhi-kn, christianvogt, vikram-raj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vikram-raj
Copy link
Member

/test analyze

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 3607a4d into openshift:master Jan 23, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/sdk Related to console-plugin-sdk component/shared Related to console-shared kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.