Monitoring: Fix graph refresh on alert and rule details pages #6881

kyoto · 2020-10-09T08:47:32Z

Pass query and ruleDuration to Graph instead of the whole rule
object to prevent some unnecessary re-rendering. This also removes the
need to memoize queries.

Memoize labels in AlertsDetailsPage to avoid triggering unnecessary
re-renders of Graph.

Initialize pollInterval to undefined instead of null. null
indicates that polling was disabled, whereas undefined indicates that
it is not specified and the QueryBrowser should use its default poll
interval.

spadgett · 2020-10-09T13:05:14Z

frontend/public/reducers/ui.ts

@@ -120,7 +120,7 @@ export default (state: UIState, action: UIAction): UIState => {
      }),
      queryBrowser: ImmutableMap({
        metrics: [],
-        pollInterval: null,
+        pollInterval: undefined,


I try to avoid code that differentiates between undefined and null, which isn't very obvious and can be a source of bugs. I think this at least deserves a comment explaining.

Thanks, the null / undefined distinction was not a great solution 😬

I'm now thinking that a better approach is to force the polling interval (pollInterval={Math.round(timespan / 120)}) instead of using QueryBrowser's default polling interval logic. This removes the need for the null / undefined change.

spadgett · 2020-10-09T13:06:54Z

frontend/public/components/monitoring/alerting.tsx

@@ -596,10 +593,15 @@ const alertStateToProps = (state: RootState, { match }): AlertsDetailsPageProps
 export const AlertsDetailsPage = withFallback(
  connect(alertStateToProps)((props: AlertsDetailsPageProps) => {
    const { alert, loaded, loadError, namespace, rule, silencesLoaded } = props;
-    const { annotations = {}, labels = {}, silencedBy = [] } = alert || {};
-    const { alertname, severity } = labels as any;
+    const annotations = alert?.annotations ?? {};


You might want || instead of ?? in most of these cases. If you ever get annotations: null, could that cause an error?

Thanks @spadgett. After looking at this again, I ended up refactoring in a way that removes the need for these 2 ?? anyway.

Use a fixed polling interval based on the alerting rule's duration. Pass `query` and `ruleDuration` to `Graph` instead of the whole `rule` object to prevent some unnecessary re-rendering. This also removes the need to memoize `queries`. Memoize `labels` in `AlertsDetailsPage` to avoid triggering unnecessary re-renders of `Graph`.

kyoto · 2020-10-30T03:32:23Z

@spadgett Rebased and addressed your feedback

spadgett

/lgtm

openshift-ci-robot · 2020-10-30T15:18:21Z

[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:

~~frontend/public/components/monitoring/OWNERS~~ [kyoto,spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-10-30T16:41:59Z

/retest