Bug 1856351: Fix build details page charts #6879

TheRealJon · 2020-10-08T21:17:40Z

Update build details page charts to only show utilization data from when the build ran.

Create new global time related constants
Update build details page area charts to only show data from build start to completion
Minimum queried timepan to one minute
Show seconds on build charts if timespan is less than 5 minutes
Force x domain on build charts to match the queried timespan so that chart will not show less than
one minute on the x-axis.
Add new xMutator callback arg to graph util mapLimitsRequests function to allow for post-processing of
utilization chart x values
Add new xMutator and yMutator callback args to graph util getRangeVectorStats function to allow
for post-processing of prometheus response data x and y values
Remove logic from getRangeVectorStats function that removed second and millisecond data from all x
values
Update PrometheusMultilineUtilizationItem to use new xMutator argument to trim seconds and
milliseconds off of multiline area chart data points.
Create new global constants for prometheus query default samples and timespan
Update Area and AreaChart components to use new global prometheus constants and accept domain prop
Update twentyFourHourTime util function to accept showSeconds arg and get rid of
twentyFourHourTimeWithSeconds function.
Update usage of twentyFourHourTime/twentyFourHourTimeWithSeconds function where needed

openshift-ci-robot · 2020-10-08T21:17:47Z

@TheRealJon: This pull request references Bugzilla bug 1856351, 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.7.0) matches configured target release for branch (4.7.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1856351: Fix build details page charts

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 · 2020-10-09T14:52:21Z

@TheRealJon: This pull request references Bugzilla bug 1856351, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1856351: Fix build details page charts

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 · 2020-10-09T15:13:38Z

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

Test name	Commit	Details	Rerun command
ci/prow/analyze	`7d4e944`	link	`/test analyze`

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.

TheRealJon · 2020-10-09T15:27:44Z

This is likely failing the the analyze test because I am importing moment into the datetime helper file. I did this to simplify the implementation, but it's probably overkill. I'll update to remove this import.

openshift-ci-robot · 2020-10-14T18:23:17Z

@TheRealJon: This pull request references Bugzilla bug 1856351, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

[WIP] Bug 1856351: Fix build details page charts

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.

TheRealJon · 2020-10-14T18:25:06Z

I think this is ready for review.

/assign @rohitkrai03

cc @spadgett

spadgett · 2020-10-14T20:13:39Z

/cc @kyoto

TheRealJon · 2020-10-14T20:25:29Z

/hold

need to fix some unit tests

TheRealJon · 2020-10-14T20:26:29Z

cc @rawagner

TheRealJon · 2020-10-14T20:32:24Z

/hold cancel

kyoto

Thanks @TheRealJon!

A few comments on the smaller details, but generally LGTM 👍

kyoto · 2020-10-21T01:03:34Z

frontend/packages/console-shared/src/constants/time.ts

+export const ONE_MONTH = 30 * ONE_DAY;
+export const ONE_YEAR = 365.25 * ONE_DAY;


I'd suggest removing ONE_MONTH and ONE_YEAR since they're not currently used and obviously both 30 days and 325.25 days are not technically accurate. In practice, I think the definition of month and year will depend on the context they're used in.

kyoto · 2020-10-21T01:13:51Z

frontend/public/components/monitoring/query-browser.tsx

-
-    const xTickFormat = span < 5 * 60 * 1000 ? twentyFourHourTimeWithSeconds : twentyFourHourTime;
-
+    const xTickFormat = (d) => twentyFourHourTime(d, span < 5 * 60 * 1000);


I guess we use ONE_MINUTE here 😄

kyoto · 2020-10-21T01:15:56Z

frontend/public/components/utils/datetime.ts

-  return `${hours}:${minutes}:${seconds}`;
+export const twentyFourHourTime = (date: Date, showSeconds?: boolean): string => {
+  const hours = zeroPad(date?.getHours() ?? 0);
+  const minutes = `:${zeroPad(date?.getMinutes() ?? 0)}`;


nit: I think including the : in the return statement as before is a little easier to read.

To keep it consistent with seconds, I think we should keep the colon as part of minutes.

kyoto · 2020-10-21T01:19:19Z

frontend/public/components/utils/datetime.ts

-  const seconds = zeroPad(date.getSeconds());
-  return `${hours}:${minutes}:${seconds}`;
+export const twentyFourHourTime = (date: Date, showSeconds?: boolean): string => {
+  const hours = zeroPad(date?.getHours() ?? 0);


Do we need to handle the case where date is not set? Since it's not an optional parameter and we use type checking, can we assume that it is indeed a Date and lose the ?. checks?

kyoto · 2020-10-21T01:23:28Z

frontend/public/components/utils/datetime.ts

+export const twentyFourHourTime = (date: Date, showSeconds?: boolean): string => {
+  const hours = zeroPad(date?.getHours() ?? 0);
+  const minutes = `:${zeroPad(date?.getMinutes() ?? 0)}`;
+  const seconds = `:${zeroPad(date?.getSeconds() ?? 0)}`;


Probably not significant in practice, but I notice the computation for seconds is done just to be thrown away unless showSeconds is true.

How about this?

const seconds = showSeconds ? `:${zeroPad(date?.getSeconds() ?? 0)}` : '';

kyoto · 2020-10-21T01:54:21Z

frontend/public/components/graphs/area.tsx

  ...rest
 }) => {
-  const [utilization, , utilizationLoading] = usePrometheusPoll({
+  const prometheusPollProps = {


kyoto · 2020-10-21T02:15:27Z

frontend/public/components/build.tsx

  if (!podName) {
    return null;
  }
-
+  const endTime = build?.status?.completionTimestamp


We're assuming that build is set elsewhere in this function, so I guess we can use build. instead of build.? everywhere.

kyoto · 2020-10-21T02:33:09Z

frontend/public/components/build.tsx

+    formatDate: (d) => twentyFourHourTime(d, timespan < 5 * ONE_MINUTE),
+    domain: { x: [endTime - timespan, endTime] }, // force domain to match queried timespan


I think we're OK in this case, but in general I've had trouble with triggering unnecessary graph re-renders by having function / object / array props that are recreated in the render function like this.

If the graph does not seem to be re-rendering too frequently, I think this is fine, but in general I'm trying to avoid these situations. For example, passing startTime and endTime integer props instead of a domain object might avoid some re-renders, depending on how BuildGraphs is used.

I'll pull out domain into a separate declaration and memoize it so it will only get re-initialized if timespan or endTime change.

I'll also memoize areaProps so that it won't cause unnecessary re-renders.

kyoto · 2020-10-21T02:38:59Z

frontend/public/components/graphs/area.tsx

+            domain={{
+              ...(allZero && { y: [0, 1] }),
+              ...domain,
+            }}


The could be triggering unnecessary re-renders. If so, domain could be memoized.

kyoto · 2020-10-21T02:42:31Z

frontend/public/components/graphs/area.tsx

          >
-            {xAxis && <ChartAxis tickCount={tickCount} tickFormat={formatDate} />}
+            {xAxis && <ChartAxis tickCount={tickCount} tickFormat={(tick) => formatDate(tick)} />}


Using (tick) => formatDate(tick) instead of just formatDate might cause ChartAxis to re-render unnecessarily too. An easy solution to avoid that possibility would be to define const xAxisTickFormat = (tick) => formatDate(tick); outside the component.

We can't define this outside the component since formatDate is a prop. I'll use the React callback hook to make sure the function definition only changes when necessary.

TheRealJon · 2020-10-23T02:08:02Z

/retest

TheRealJon · 2020-10-23T14:09:13Z

@kyoto Addressed your feedback

- Create new global time related constants - Update build details page area charts to only from build start to completion - Minimum queried timepan to one minute - Show seconds on build charts if timespan is less than 5 minutes - Force x domain on build charts to match the queried timespan so that chart will not show less than one minute on the x-axis. - Add new xMutator callback arg to graph util `mapLimitsRequests` function to allow for post-processing of utilization chart x values - Add new xMutator and yMutator callback args to graph util `getRangeVectorStats` function to allow for post-processing of prometheus response data x and y values - Remove logic from getRangeVectorStats function that removed second and millisecond data from all x values - Update PrometheusMultilineUtilizationItem to use new xMutator argument to trim seconds and milliseconds off of multiline area chart data points. - Create new global constants for prometheus query default samples and timespan - Update Area and AreaChart components to use new global prometheus constants and accept domain prop - Update twentyFourHourTime util function to accept `showSeconds` arg and get rid of twentyFourHourTimeWithSeconds function. - Update usage of twentyFourHourTime/twentyFourHourTimeWithSeconds function where needed

kyoto · 2020-10-28T11:51:18Z

Thanks @TheRealJon!

/lgtm

openshift-ci-robot · 2020-10-28T11:51:35Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/OWNERS~~ [kyoto]

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-28T13:34:37Z

/retest

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

openshift-bot · 2020-10-28T14:55:00Z

/retest

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

openshift-ci-robot · 2020-10-28T15:07:14Z

@TheRealJon: All pull requests linked via external trackers have merged:

openshift/console#6879

Bugzilla bug 1856351 has been moved to the MODIFIED state.

In response to this:

Bug 1856351: Fix build details page charts

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 added bugzilla/severity-low Referenced Bugzilla bug's severity is low 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. labels Oct 8, 2020

openshift-ci-robot requested review from jeff-phillips-18 and kyoto October 8, 2020 21:17

openshift-ci-robot added the component/monitoring Related to monitoring label Oct 8, 2020

TheRealJon force-pushed the 1856351-bz branch from 7461f5d to 6a7f091 Compare October 9, 2020 14:46

TheRealJon force-pushed the 1856351-bz branch from 6a7f091 to 7d4e944 Compare October 9, 2020 14:52

TheRealJon changed the title ~~Bug 1856351: Fix build details page charts~~ [WIP] Bug 1856351: Fix build details page charts Oct 9, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2020

TheRealJon force-pushed the 1856351-bz branch from 7d4e944 to 5b8d3bf Compare October 14, 2020 18:22

openshift-ci-robot added component/dashboard Related to dashboard component/shared Related to console-shared labels Oct 14, 2020

TheRealJon changed the title ~~[WIP] Bug 1856351: Fix build details page charts~~ Bug 1856351: Fix build details page charts Oct 14, 2020

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 Oct 14, 2020

openshift-ci-robot assigned rohitkrai03 Oct 14, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2020

TheRealJon force-pushed the 1856351-bz branch from 5b8d3bf to a1650d8 Compare October 14, 2020 20:32

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2020

TheRealJon requested a review from rawagner October 14, 2020 20:32

kyoto reviewed Oct 21, 2020

View reviewed changes

TheRealJon force-pushed the 1856351-bz branch 2 times, most recently from 498788a to c9c0d12 Compare October 22, 2020 20:50

TheRealJon requested a review from kyoto October 23, 2020 14:08

TheRealJon force-pushed the 1856351-bz branch from c9c0d12 to 3a85227 Compare October 27, 2020 17:59

TheRealJon force-pushed the 1856351-bz branch from 3a85227 to f6c127a Compare October 27, 2020 19:40

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Oct 27, 2020

openshift-ci-robot assigned kyoto Oct 28, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 28, 2020

openshift-merge-robot merged commit 984dbf4 into openshift:master Oct 28, 2020

spadgett added this to the v4.7 milestone Oct 29, 2020

TheRealJon deleted the 1856351-bz branch January 22, 2021 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1856351: Fix build details page charts #6879

Bug 1856351: Fix build details page charts #6879

TheRealJon commented Oct 8, 2020 •

edited

openshift-ci-robot commented Oct 8, 2020

openshift-ci-robot commented Oct 9, 2020

openshift-ci-robot commented Oct 9, 2020

TheRealJon commented Oct 9, 2020

openshift-ci-robot commented Oct 14, 2020

TheRealJon commented Oct 14, 2020

spadgett commented Oct 14, 2020

TheRealJon commented Oct 14, 2020

TheRealJon commented Oct 14, 2020

TheRealJon commented Oct 14, 2020

kyoto left a comment

kyoto Oct 21, 2020

kyoto Oct 21, 2020

kyoto Oct 21, 2020

TheRealJon Oct 22, 2020 •

edited

kyoto Oct 21, 2020

kyoto Oct 21, 2020

kyoto Oct 21, 2020

kyoto Oct 21, 2020

kyoto Oct 21, 2020

TheRealJon Oct 22, 2020

TheRealJon Oct 22, 2020

kyoto Oct 21, 2020

kyoto Oct 21, 2020

TheRealJon Oct 22, 2020

TheRealJon commented Oct 23, 2020

TheRealJon commented Oct 23, 2020

kyoto commented Oct 28, 2020

openshift-ci-robot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-ci-robot commented Oct 28, 2020

		export const ONE_MONTH = 30 * ONE_DAY;
		export const ONE_YEAR = 365.25 * ONE_DAY;


		const xTickFormat = span < 5 * 60 * 1000 ? twentyFourHourTimeWithSeconds : twentyFourHourTime;

		const xTickFormat = (d) => twentyFourHourTime(d, span < 5 * 60 * 1000);

		formatDate: (d) => twentyFourHourTime(d, timespan < 5 * ONE_MINUTE),
		domain: { x: [endTime - timespan, endTime] }, // force domain to match queried timespan

Bug 1856351: Fix build details page charts #6879

Bug 1856351: Fix build details page charts #6879

Conversation

TheRealJon commented Oct 8, 2020 • edited

openshift-ci-robot commented Oct 8, 2020

openshift-ci-robot commented Oct 9, 2020

openshift-ci-robot commented Oct 9, 2020

TheRealJon commented Oct 9, 2020

openshift-ci-robot commented Oct 14, 2020

TheRealJon commented Oct 14, 2020

spadgett commented Oct 14, 2020

TheRealJon commented Oct 14, 2020

TheRealJon commented Oct 14, 2020

TheRealJon commented Oct 14, 2020

kyoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealJon Oct 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealJon commented Oct 23, 2020

TheRealJon commented Oct 23, 2020

kyoto commented Oct 28, 2020

openshift-ci-robot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-bot commented Oct 28, 2020

openshift-ci-robot commented Oct 28, 2020

TheRealJon commented Oct 8, 2020 •

edited

TheRealJon Oct 22, 2020 •

edited