Skip to content

Conversation

@invincibleJai
Copy link
Member

@invincibleJai invincibleJai commented Jan 8, 2020

Dev metrics: visualization of queries in the metrics tab

  • Add Monitoring Metrics tab to run out-of-the-box queries on a project
  • Show Dropdown with pre-defined queries
  • Select should show series chart along with associated legends

Tracks:

Gif:

ezgif com-video-to-gif (14)

@invincibleJai invincibleJai changed the title Dev metrics: visualisation of queries in metrics [WIP] Dev metrics: visualisation of queries in metrics Jan 8, 2020
@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 8, 2020
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/dev-console Related to dev-console component/monitoring Related to monitoring labels Jan 8, 2020
@invincibleJai invincibleJai force-pushed the fix-monitoring-metrics branch from ed3a146 to eba9d22 Compare January 8, 2020 17:18
@andrewballantyne
Copy link
Contributor

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 8, 2020
@invincibleJai invincibleJai force-pushed the fix-monitoring-metrics branch from eba9d22 to 61ffe63 Compare January 10, 2020 08:20
Copy link
Contributor

Choose a reason for hiding this comment

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

we can namespace this to odc-metrics-dropdownfield

Copy link
Contributor

Choose a reason for hiding this comment

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

if we are not accepting title to be changed from parent component, then the type can be string

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are using this react.memo approach to improve the performance and eslint-disable comment to avoid get lint errors, then its a good practice to enable it back after the usage.

Suggested change
]);
]);
/* eslint-enable react-hooks/exhaustive-deps */

Copy link
Contributor

Choose a reason for hiding this comment

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

Only use eslint-disable-next-line

@invincibleJai invincibleJai force-pushed the fix-monitoring-metrics branch 3 times, most recently from 57007c4 to 6f379a6 Compare January 13, 2020 08:54
@invincibleJai invincibleJai changed the title [WIP] Dev metrics: visualisation of queries in metrics Dev metrics: visualisation of queries in metrics Jan 13, 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 13, 2020
@invincibleJai
Copy link
Member Author

/cc @openshift/team-devconsole-ux @serenamarie125

@invincibleJai invincibleJai force-pushed the fix-monitoring-metrics branch 2 times, most recently from 71e501a to 2317dc6 Compare January 13, 2020 13:57
@karthikjeeyar
Copy link
Contributor

Verified locally, looks good to me.
/lgtm

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

/retest

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.

Tested it locally and its work fine as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assign is unnecessary here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Prefer length 0 check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this is?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see a mix between type and interface. Choose one and stick to it in the same file.

I don't think there is an agreement on preference overall but at least in the same file we should be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

have made use of type across

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this run the first time through with an empty string metric?

@invincibleJai invincibleJai force-pushed the fix-monitoring-metrics branch from 2317dc6 to b28e8fb Compare January 14, 2020 03:12
@invincibleJai
Copy link
Member Author

@christianvogt thanks for the review, have fixed review comments PTAL.

@openshift-bot
Copy link
Contributor

/retest

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

9 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@spadgett
Copy link
Member

/hold
to avoid retest while CI is broken

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 19, 2020
@serenamarie125
Copy link
Contributor

@invincibleJai couple of questions:

  1. what is the paging control for that's in your gif?
  2. could you put a border around the entire panel including the filter/graph/legend so it looks visually connected?

@invincibleJai
Copy link
Member Author

invincibleJai commented Jan 20, 2020

@serenamarie125 pagination control in the gif is for legends.

could you put a border around the entire panel including the filter/graph/legend so it looks visually connected?

yes we can have a border, it'll wrap pagination as well right. Let me know your thoughts, it'll be as in below screenshot

we have associated PR #3955 , this adds PromQL editor PTAL

Screenshot 2020-01-20 at 9 47 53 AM

@invincibleJai invincibleJai force-pushed the fix-monitoring-metrics branch from 5f48659 to 9621349 Compare January 20, 2020 05:27
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2020
@invincibleJai invincibleJai force-pushed the fix-monitoring-metrics branch from 9621349 to 8bf261d Compare January 20, 2020 08:20
Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@invincibleJai this looks great, thanks for acting on the feedback!

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 20, 2020
@vikram-raj
Copy link
Member

/retest

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, karthikjeeyar, serenamarie125, TheRealJon, 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

@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2020
@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@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 d0b8d94 into openshift:master Jan 21, 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/monitoring Related to monitoring 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.