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

Bug 1827753: autoselect query from metric dropdown #5184

Merged
merged 1 commit into from May 7, 2020

Conversation

vikram-raj
Copy link
Member

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

Analysis / Root cause:
When clicking on any metrics graph in the dashboard or sidebar in topology view takes you to metrics in the monitoring tab but the query field remains unchanged

Solution Description:
autoselect query from the metric dropdown if it matches.

Screen shots / Gifs for design review:
monitoring

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@vikram-raj
Copy link
Member Author

/retitle Bug 1827753: autoselect query from metric dropdown

@openshift-ci-robot openshift-ci-robot changed the title autoselect query from metric dropdown Bug 1827753: autoselect query from metric dropdown Apr 24, 2020
@openshift-ci-robot
Copy link
Contributor

@vikram-raj: This pull request references Bugzilla bug 1827753, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1827753: autoselect query from metric dropdown

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 24, 2020
Comment on lines 20 to 24
'CPU Usage': 'CPU Usage',
'Memory Usage': 'Memory Usage',
'Filesystem Usage': 'Filesystem Usage',
'Receive Bandwidth': 'Receive Bandwidth',
'Transmit Bandwidth': 'Transmit Bandwidth',
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion the Idea of having enum/const variables is that you can change the value (string) in one place and those variables used elsewhere should receive the changed string. But here in this case you need to change both Key and Value, which is not very useful.

const [title, setTitle] = React.useState('Select Query');
const [selectedKey, setselectedKey] = React.useState('');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [selectedKey, setselectedKey] = React.useState('');
const [selectedKey, setSelectedKey] = React.useState('');

@vikram-raj vikram-raj force-pushed the odc-3064 branch 3 times, most recently from 8b5b61c to d6ab61d Compare April 27, 2020 07:28
@karthikjeeyar
Copy link
Contributor

On clicking CPU usage graph, it takes me to the monitoring page which has list view under the graph but the graph is not showing up. By inspecting the page, graph appears again.

graph-appears-on-inspecting

@vikram-raj
Copy link
Member Author

On clicking CPU usage graph, it takes me to the monitoring page which has list view under the graph but the graph is not showing up. By inspecting the page, graph appears again.

The graph takes some time to load.

@vikram-raj
Copy link
Member Author

cc: @openshift/team-devconsole-ux

@invincibleJai
Copy link
Member

/assign @invincibleJai

Comment on lines +61 to +63
if (query) {
removeQueryArgument('query0');
}
Copy link
Member

Choose a reason for hiding this comment

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

what's the use of this , i see one in onChange

Copy link
Member Author

Choose a reason for hiding this comment

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

It removes query params from the URL. If the user starts editing the promql or changes the query using the query dropdown.

Copy link
Member

Choose a reason for hiding this comment

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

can we handle both scenarios in hook itself

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.

Quick question on this @vikram-raj
Our Out of the Box CPU Usage query is per project, correct?
I'm assuming that's why when clicking on the CPU Usage card from the dashboard, it defaults to CPU Usage, but when clicking on CPU Usage from the side panel in topology, it defaults to Custom Query

@vikram-raj
Copy link
Member Author

Quick question on this @vikram-raj
Our Out of the Box CPU Usage query is per project, correct?
I'm assuming that's why when clicking on the CPU Usage card from the dashboard, it defaults to CPU Usage, but when clicking on CPU Usage from the side panel in topology, it defaults to Custom Query

Correct, out of box CPU Usage query is per project. and CPU Usage from the side panel is per workload.

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.

LGTM! Approved by UX. Thanks @vikram-raj

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. and removed bugzilla/medium labels Apr 29, 2020
@vikram-raj vikram-raj force-pushed the odc-3064 branch 2 times, most recently from 5b98f0d to a5eda15 Compare April 29, 2020 10:33
@invincibleJai
Copy link
Member

/retest

@karthikjeeyar
Copy link
Contributor

verified locally and works fine
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2020
@invincibleJai
Copy link
Member

@vikram-raj it works fine in other scenarios however i see issue ns change if came from topology to metrics see GIF attached.

May-07-2020 17-58-50

@vikram-raj
Copy link
Member Author

@vikram-raj it works fine in other scenarios however i see issue ns change if came from topology to metrics see GIF attached.

@invincibleJai We have this issue in master and we can handle it separately. It is happening because the workload query is not under the pre-canned query that we define for the metrics dropdown.

@invincibleJai
Copy link
Member

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: invincibleJai, karthikjeeyar, serenamarie125, vikram-raj

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

The pull request process is described here

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2020
@openshift-merge-robot openshift-merge-robot merged commit 5ee5f6b into openshift:master May 7, 2020
@openshift-ci-robot
Copy link
Contributor

@vikram-raj: All pull requests linked via external trackers have merged: openshift/console#5184. Bugzilla bug 1827753 has been moved to the MODIFIED state.

In response to this:

Bug 1827753: autoselect query from metric dropdown

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@vikram-raj vikram-raj deleted the odc-3064 branch May 7, 2020 17:29
@vikram-raj
Copy link
Member Author

/cherry-pick release-4.4

@openshift-cherrypick-robot

@vikram-raj: new pull request created: #5383

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants