Skip to content

Monitoring: Add metric graphs to Alert and Rule details pages #1237

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

Merged

Conversation

kyoto
Copy link
Member

@kyoto kyoto commented Feb 28, 2019

Adds a QueryBrowser component, which plots a PromQL query as a line
graph along with controls for changing the graph to display the query
for any time range.

Adds the QueryBrowser to both the alert details page and the rule
details page. The alert graph shows the single metric for that alert and
the rule graph shows all metrics data for that rule.

Upgrade Plotly to version 1.44.4.

Move the "View in Prometheus UI" link from the top of the page to the
graph area.

Make some changes to the graph Base class.

  • Make the title property optional
  • Add the ability to invoke fetch() without enabling polling
  • Set a lower limit of 5 seconds on the graph refresh interval to
    prevent too frequent polling when viewing narrow time ranges.
  • Add optional numSamples property that defines how many data points
    will be plotted for each metric

Work left for future PRs includes

  • Add an "ending" input to improve the UX when viewing time spans in the past
  • Improve the styling of invalid graph time spans (text input)
  • Graph legend

alert

rule

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 28, 2019
@sichvoge
Copy link

@kyoto can you add an image on how this looks like?

@kyoto
Copy link
Member Author

kyoto commented Feb 28, 2019

@sichvoge Sorry, I hit the create button before adding the screenshots. Now updated.

@kyoto
Copy link
Member Author

kyoto commented Feb 28, 2019

/test console-e2e

@kyoto
Copy link
Member Author

kyoto commented Feb 28, 2019

FYI @cshinn

@spadgett
Copy link
Member

Will need #1226 to fix the test failure

@spadgett
Copy link
Member

#1240 fast-forwards master-next for the CI fix

@sichvoge
Copy link

image

This looks crazy. Is there a legend to understand what actually been shown here?

@spadgett spadgett added this to the v4.1 milestone Feb 28, 2019
@kyoto
Copy link
Member Author

kyoto commented Mar 1, 2019

@sichvoge Good point. There's no legend as part of this PR, but that will need to be added in a future PR.

@kyoto kyoto force-pushed the query-browser-alert-graph branch from e0b3e84 to a550b15 Compare March 1, 2019 01:34
@kyoto
Copy link
Member Author

kyoto commented Mar 1, 2019

Rebased

Adds a `QueryBrowser` component, which plots a PromQL query as a line
graph along with controls for changing the graph to display the query
for any time range.

Adds the `QueryBrowser` to both the alert details page and the rule
details page. The alert graph shows the single metric for that alert and
the rule graph shows all metrics data for that rule.

Upgrade Plotly to version 1.44.4.

Move the "View in Prometheus UI" link from the top of the page to the
graph area.

Make some changes to the graph Base class.
  - Make the `title` property optional
  - Add the ability to invoke `fetch()` without enabling polling
  - Set a lower limit of 5 seconds on the graph refresh interval to
    prevent too frequent polling when viewing narrow time ranges.
  - Add optional `numSamples` property that defines how many data points
    will be plotted for each metric
@kyoto kyoto force-pushed the query-browser-alert-graph branch from a550b15 to cb4fd3e Compare March 4, 2019 07:46
@kyoto
Copy link
Member Author

kyoto commented Mar 4, 2019

Force pushed a couple of fixes.

@kyoto
Copy link
Member Author

kyoto commented Mar 4, 2019

/retest

2 similar comments
@kyoto
Copy link
Member Author

kyoto commented Mar 5, 2019

/retest

@kyoto
Copy link
Member Author

kyoto commented Mar 7, 2019

/retest

@spadgett spadgett changed the base branch from master-next to master March 9, 2019 13:26
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.

Nice work on this

/lgtm

const units = {w, d, h, m, s};

// Formats a duration in milliseconds like "1h 10m"
export const formatPrometheusDuration = (ms: number) => {
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should update formatDuration to be this and use it everywhere. Is the primary difference to support longer durations like days and weeks?

const spans = ['5m', '15m', '30m', '1h', '2h', '6h', '12h', '1d', '2d', '1w', '2w'];
const dropdownItems = _.zipObject(spans, spans);

class QueryBrowser_ extends Line_ {
Copy link
Member

Choose a reason for hiding this comment

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

General comment that we've been trying to get away from inheritance in React components, but I'm fine with it here since it's how the other charts work.

https://reactjs.org/docs/composition-vs-inheritance.html

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2019
@spadgett
Copy link
Member

spadgett commented Mar 9, 2019

/retest

1 similar comment
@spadgett
Copy link
Member

spadgett commented Mar 9, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit 6c2164b into openshift:master Mar 9, 2019
@kyoto kyoto deleted the query-browser-alert-graph branch March 12, 2019 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants