Skip to content

Conversation

@invincibleJai
Copy link
Member

@invincibleJai invincibleJai commented Jan 14, 2020

  • Adds PromQL Editor to monitoring metrics

Tracks:

Gif:
ezgif com-video-to-gif (15)

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

@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
@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 14, 2020
@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 15, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually Show PromQL button is not shown in design mock up when no query is selected. If possible its good to hide otherwise we can set isPromQlDisabled default value to true

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's there Show PromQL button in the mockup

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 is looking good! Couple of things I see which would be good to address:

  1. When the graph comes up, the legend is viewed before the chart. Is there a way to display it at the same time ( picky I know, so if this is difficult, don't worry )
  2. Is it still possible to get the hints when creating the PromQL query as it was in the metrics page?
  3. in your animated gif, some tooltip shows up very quickly. Is that associated with the top line in the graph? Curious if we should have a bit of a delay (similar to the timing we do with the pipeline visualization) so that tooltips aren't showing up immediately.

@kyoto
Copy link
Member

kyoto commented Jan 21, 2020

FYI @sichvoge

@invincibleJai invincibleJai force-pushed the fix-promql-edit branch 2 times, most recently from a1320d1 to 8106741 Compare January 21, 2020 12:35
@invincibleJai invincibleJai changed the title [WIP] Adds PromQL Editor to monitoring metrics Adds PromQL Editor to monitoring metrics Jan 21, 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 21, 2020
@invincibleJai
Copy link
Member Author

@serenamarie125 Thanks for inputs, have updated the gif PTAL and added comments below.

When the graph comes up, the legend is viewed before the chart. Is there a way to display it at the same time ( picky I know, so if this is difficult, don't worry )

we are rendering the components at the same time, on the first load graph rendering takes more than legend

Is it still possible to get the hints when creating the PromQL query as it was in the metrics page?

have updated the gif with hints.

in your animated gif, some tooltip shows up very quickly. Is that associated with the top line in the graph? Curious if we should have a bit of a delay (similar to the timing we do with the pipeline visualization) so that tooltips aren't showing up immediately.

Graph component is reused from the console and internally it makes use of the Pf line chart. We can take this as an issue in separate PR if needed.

Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
const autocompleteFilter = (strtext, item) => fuzzy(strtext, item);
const autocompleteFilter = (strText, item) => fuzzy(strText, item);

Copy link
Member

Choose a reason for hiding this comment

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

nit: This way it is more readable.

Suggested change
if (selVal && selVal === ADD_NEW_QUERY) {
if (selectedValue && selectedValue === ADD_NEW_QUERY) {

Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
{showPromQl ? `Hide PromQL` : `Show PromQL`}
{showPromQl ? 'Hide PromQL' : 'Show PromQL'}

Copy link
Member

@vikram-raj vikram-raj Jan 21, 2020

Choose a reason for hiding this comment

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

Can we directly pass setShowPromQl to onClick as we are not doing anything else in togglePromQl?

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 thanks for the updates and responses to my comments. From a UX perspective, this PR is approved.

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 21, 2020
Comment on lines 107 to 111
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably omit this entire row if there is no content to show.

Comment on lines 60 to 61
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine these two conditions into one.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020
@invincibleJai invincibleJai force-pushed the fix-promql-edit branch 2 times, most recently from d8d3463 to 3d103fc Compare January 21, 2020 18:20
@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 21, 2020
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 22, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

@openshift-bot
Copy link
Contributor

/retest

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

@kyoto
Copy link
Member

kyoto commented Jan 22, 2020

FYI @openshift/openshift-team-monitoring

@openshift-merge-robot openshift-merge-robot merged commit e6e3fed into openshift:master Jan 22, 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.