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

Query Browser: Redesign tooltips and add stacked graph option #7408

Merged
merged 1 commit into from Jan 6, 2021
Merged

Query Browser: Redesign tooltips and add stacked graph option #7408

merged 1 commit into from Jan 6, 2021

Conversation

kyoto
Copy link
Member

@kyoto kyoto commented Dec 3, 2020 •
edited

Addresses https://issues.redhat.com/browse/MON-787 and https://issues.redhat.com/browse/MON-1299.

Add a "Stacked" checkbox option to the Query Browser. Enabled for the
Metrics page of both the Admin and Dev Consoles.

Change the query browser graph tooltips to display all data series in
the graph at a single timestamp (instead of showing the data for just a
single data point).

If formatLegendLabel is provided, use that to generate the tooltip
string for the series names so that the names in the tooltip match those
in the graph legend. Otherwise generate a name using all of the data
series labels.

Removes the need to patch the query into Redux, so that code can be
removed from graph.tsx.

Fixes a bug with the tooltips where the vertical position of the tooltip
sometimes did not align to the corresponding data series line.

Admin Console

screenshot-2

Dev Console

screenshot-3
screenshot-1

FYI @cshinn

@openshift-ci-robot openshift-ci-robot added component/dev-console Related to dev-console approved Indicates a PR has been approved by an approver from all required OWNERS files. component/monitoring Related to monitoring labels Dec 3, 2020
@kyoto
Copy link
Member Author

kyoto commented Dec 3, 2020

/retest

@kyoto
Copy link
Member Author

kyoto commented Dec 4, 2020

/retest

@kyoto
Copy link
Member Author

kyoto commented Dec 4, 2020

/retest

2 similar comments
@kyoto
Copy link
Member Author

kyoto commented Dec 7, 2020

/retest

@kyoto
Copy link
Member Author

kyoto commented Dec 7, 2020

/retest

@kyoto kyoto mentioned this pull request Dec 7, 2020
Merged
@kyoto
Copy link
Member Author

kyoto commented Dec 9, 2020

I realized that this change allows us to remove 2 cases where patchQuery was being used to set the (now unused) query for the tooltip, so I've added that change to this PR.

@spadgett Could you please review?

spadgett
spadgett approved these changes Dec 9, 2020
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold for approvals
/assign @yapei @ahardin-rh @sferich888

@kyoto Can you link to a JIRA story tracking the change?

@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 Dec 9, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2020
@kyoto
Copy link
Member Author

kyoto commented Dec 10, 2020

@kyoto Can you link to a JIRA story tracking the change?

This change covers 2 Jira stories. I added both to the PR description.

@ahardin-rh
Copy link

/label docs-approved

@openshift-ci-robot openshift-ci-robot added the docs-approved Signifies that Docs has signed off on this PR label Dec 10, 2020
@yapei
Copy link
Contributor

yapei commented Dec 11, 2020

tested locally and all works as expected

@yapei
Copy link
Contributor

yapei commented Dec 11, 2020

/label qe-approved

@openshift-ci-robot openshift-ci-robot added the qe-approved Signifies that QE has signed off on this PR label Dec 11, 2020
@lihongyan1
Copy link

@spadgett For monitoring related PR, when you complete review, you should assign QE @lihongyan1 or @juzhao

@juzhao
Copy link

juzhao commented Dec 14, 2020

/assign @hongyli

@openshift-ci-robot
Copy link
Contributor

@juzhao: GitHub didn't allow me to assign the following users: hongyli.

Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @hongyli

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.

@lihongyan1
Copy link

@kyoto Filed bug https://bugzilla.redhat.com/show_bug.cgi?id=1907269

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2020

@kyoto: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/kubevirt-plugin 0fb81f6 link /test kubevirt-plugin

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@kyoto
Query Browser: Redesign tooltips and add stacked graph option
f08b2a8 
Add a "Stacked" checkbox option to the Query Browser. Enabled for the
Metrics page of both the Admin and Dev Consoles.

Change the query browser graph tooltips to display all data series in
the graph at a single timestamp (instead of showing the data for just a
single data point).

If `formatLegendLabel` is provided, use that to generate the tooltip
string for the series names so that the names in the tooltip match those
in the graph legend. Otherwise generate a name using all of the data
series labels.

Removes the need to patch the query into Redux, so that code can be
removed from `graph.tsx`.

Fixes a bug with the tooltips where the vertical position of the tooltip
sometimes did not align to the corresponding data series line.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2020
@kyoto
Copy link
Member Author

kyoto commented Dec 23, 2020 •
edited

Force pushed a change with some fixes.

  • Fix bug https://bugzilla.redhat.com/show_bug.cgi?id=1907269
  • Fix JS error caused by datum.x sometimes not being a Date
  • Fix an issue where the tooltip would be displayed when hovering over empty areas of the graph (it
    would render for the nearest data point even if that was far from the mouse cursor)
  • Render our own vertical line instead of using VictoryCursorContainer (which was included using createContainer). This means the line is now correctly aligned with the data point rather than the mouse position.
  • Some code cleanup

@openshift-ci-robot
Copy link
Contributor

@lihongyan1: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

  • /test analyze
  • /test backend
  • /test e2e-gcp-console
  • /test frontend
  • /test images
  • /test kubevirt-plugin

Use /test all to run the following jobs:

  • pull-ci-openshift-console-master-analyze
  • pull-ci-openshift-console-master-backend
  • pull-ci-openshift-console-master-e2e-gcp-console
  • pull-ci-openshift-console-master-frontend
  • pull-ci-openshift-console-master-images

In response to this:

/retest by QE
https://bugzilla.redhat.com/show_bug.cgi?id=1907269 is fixed
1 When perform sample query and 'aggregator_openapi_v2_regeneration_count', stack check box is gone for displaying info 'Displaying with reduced resolution due to large dataset.',
2 When perform sample query and 'aggregator_openapi_v2_regeneration_duration', when checked stack, the alert data chart is at the bottom and uncheck stack, the alert data will be at top
Please clarify if these 2 issues are bug

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.

@lihongyan1
Copy link

/retest
https://bugzilla.redhat.com/show_bug.cgi?id=1907269 is fixed

spadgett
spadgett approved these changes Jan 5, 2021
View reviewed changes
Copy link
Member

@spadgett spadgett 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 5, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kyoto, spadgett

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

@kyoto
Copy link
Member Author

kyoto commented Jan 6, 2021

@sferich888 Could you approve?

@reestr
Copy link

reestr commented Jan 6, 2021

/label px-approved

@openshift-ci-robot openshift-ci-robot added the px-approved Signifies that Product Support has signed off on this PR label Jan 6, 2021
@spadgett
Copy link
Member

spadgett commented Jan 6, 2021

/hold cancel

@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 6, 2021
@openshift-merge-robot openshift-merge-robot merged commit a7571fa into openshift:master Jan 6, 2021
@kyoto kyoto deleted the query-browser-stacked branch January 7, 2021 00:22
@spadgett spadgett added this to the v4.7 milestone Jan 7, 2021
@kyoto kyoto mentioned this pull request Feb 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

@debsmita1 debsmita1 Awaiting requested review from debsmita1

@TheRealJon TheRealJon Awaiting requested review from TheRealJon

Assignees

@spadgett spadgett

@sferich888 sferich888

@yapei yapei

@ahardin-rh ahardin-rh

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 docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
v4.7
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@kyoto @ahardin-rh @yapei @lihongyan1 @juzhao @openshift-ci-robot @reestr @spadgett @sferich888 @openshift-merge-robot