ui: Further improvements to time selection code #76593
Labels
A-sql-console-general
SQL Observability issues on the DB console spanning multiple areas. Includes Cockroach Cloud Console
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
no-issue-activity
X-stale
Comments
jocrl
added
C-enhancement
craig bot
pushed a commit
that referenced
this issue
Feb 25, 2022
75586: ui: De-duplicate and clarify time selection state r=jocrl a=jocrl Resolves #75579. This is an entirely internal refactor. https://github.com/cockroachlabs/managed-service/pull/7998 makes companion changes in managed-service. Suggestions from comments from this PR review have been separated into [this issue](#76593). # Commits ## Commit 1 ui: Always write to `windowEnd` on `scale`; read from `scale` for the metrics graph. Partially addresses #75579 Previously, `windowEnd` on `scale` was not always written to, meaning that it was not always safe/correct to read off the `scale` property. `currentWindow` wasn't always safe to use either. This commit modifies `windowEnd` to always be set, by making it an non-optional property and letting Typescript do the enforcement. It also modifies the metrics page graph to read from `scale` instead of `currentWindow` There was a [comment](https://github.com/cockroachdb/cockroach/blob/master/pkg/ui/workspaces/db-console/src/views/shared/containers/metricDataProvider/index.tsx#L258) where `currentWindow` is used to fix a bug on the metrics page because `scale` was incorrect and out of sync. This commit does not regress that drag-to-zoom bug. Side note about the bug: at the code state when the #70594 bug was fixed, the `windowEnd` property was not set (this commit changes `linegraph/index.tsx` to set it). Yet, even without changing `linegraph/index.tsx` to set `windowEnd`, reading the start and end times from `scale` did not recreate the drag-to-zoom granularity bug. However, reading from `scale` without setting `windowEnd` does cause a separate major bug in that the timescale endpoint is always queried with `now` as an end time instead of the time that the user selected and is displayed. This PR does not have this bug. Release note: None ## Commit 2 ui: Make `currentWindow.end` the single source of truth Partially addresses #75579 Previously, there are [two properties](https://github.com/cockroachdb/cockroach/blob/f18fe733c724cf51bffcde0535caa5d4bab7fa6c/pkg/ui/workspaces/db-console/src/redux/timewindow.ts#L57) on `timewindow` state, `scale` and `currentWindow`. They seemed like duplicates (see #75579 issue for further explanation). It was unclear what the distinction was, if any. 1) `currentWindow` is not used in cluster-ui - In cluster-ui, the `currentWindow` property on state is only ever read from in [this test](https://github.com/cockroachdb/cockroach/blob/f18fe733c724cf51bffcde0535caa5d4bab7fa6c/pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timescale.spec.tsx#L73). Further, the local variable that it is assigned to is immediately overwritten in the line right after. So to say, cluster-ui does not use the `currentWindow` state, even if there are a lot of local variables that are named `currentWindow`. `TimeWindow` is used as a type, but is not used on state. 2) `currentWindow.end` differs from `scale.windowEnd` in that the former is always a specific time which is used to do polling in the db console metrics page - Users may select that they want the end time to be "now", or a specific end time. When "now" is selected, `scale.windowEnd` is `null`, and `currentWindow.end` is a specific time, e.g. 3.51pm on a Monday. `currentWindow.end` is continually updated according to the `windowValid` frequency on the closest `DefaultTimeScaleOption`. The continual update of `currentWindow.end` is used to poll for new data for graphs on the metrics page. - There are two other places where the time selection component is used: the serverless metrics page (which is a completely different component from the db console metrics page), and the statements and transactions pages. Both of these other usages use completely different polling mechanisms that do not rely on `currentWindow.end` (and actually, also don't use the `windowValid` frequency). - So to say, `currentWindow.end` is only used for the db console metrics page. `scaleChanged` and `useTimeRange` are also only used for the db console metrics page, and help with bookkeeping around updating `currentWindow.end`. This commit thus - deletes the `currentWindow`, `scaleChanged`, and `useTimeRange` states in cluster-ui - nests the `currentWindow`, `scaleChanged`, and `useTimeRange` states under the `metricsTime` property in db-console Release note: None ## Commit 3 ui: Rename variables and add comments Finishes addressing #75579 This commit adds comments and renames various variables to better reflect their meaning and usage. https://github.com/cockroachlabs/managed-service/pull/7998 also renames `windowEnd` -> `fixedWindowEnd` in the managed-service repo. The following renames were made: - `scaleChanged` -> `shouldUpdateMetricsWindowFromScale` - `useTimeRange` -> `isFixedWindow` - `SET_WINDOW` -> `SET_METRICS_MOVING_WINDOW`, `setTimeWindow` -> `setMetricsMovingWindow` - `SET_RANGE` -> `SET_METRICS_FIXED_WINDOW`, `setTimeRange` -> `setMetricsFixedWindow` - `windowEnd` -> `fixedWindowEnd`, with its type changed from `moment.Moment | null` to `moment.Moment | false`. `false` indicates that the end time is a dynamically moving "now" - `db-console/src/redux/timewindow.ts` -> `db-console/src/redux/timeScale.ts `, same for the sibling test file, `TimeWindowState` -> `TimeScaleState`, `timeWindowReducer` -> `timeScaleReducer`. To make it clear that the `scale` property is the state to use, and that `TimeWindow` is mostly just a type interface (except for the db console metrics page polling usage) - `TimeWindowManager` -> `MetricsTimeManager`, `db-console/src/views/app/containers/timewindow/index.tsx` -> `db-console/src/views/app/containers/metricsTimeManager/index.tsx`, and the sibling test file. To distinguish from the `TimeWindow` type interface, name it for the main manager component in the file, and make it clear that this is just for the metrics page - `db-console/src/views/cluster/containers/timescale/index.tsx` -> `db-console/src/views/cluster/containers/timeScaleDropdownWithSearchParams/index.tsx`, and the sibling `.styl` file Release note: None 77009: pgwire: fix flaky AOST test r=otan a=rafiss fixes #76965 Change the test so it doesn't rely on sleeps, and instead uses cluster-provided timestamps. Release note: None Co-authored-by: Josephine Lee <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
|
We have marked this issue as stale because it has been inactive for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
#75586 sought to de-duplicate React states involved in selecting a time. Comments on that pull request brought up good ideas that were out of scope of that task. This issue documents these further improvements that are not urgent but could be made to clean up the time selection code.
For further details, see comments in #75586
key: "Custom"vs settingfixedWindowEndto a specific timemoment.utc()intimeScaleDropdown.tsxfixedWindowEndproperty in db console about reducing/simplifying the interface on TimeScaleTimeScaleStateinherit from the cluster-ui copytimeScaleReducersetisFixedWindow, but there's a potential that future code may accidentally and conflictingly setisFixedWindowin the payload as wellJira issue: CRDB-13184
The text was updated successfully, but these errors were encountered: