ui: De-duplicate and clarify time selection state #75586
Conversation
jocrl
added
T-sql-observability
A-sql-console-general
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @Santamaura, and @xinhaoz)
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 74 at r3 (raw file):
// True if currentWindow should be unchanging. False if isFixedWindow: boolean; };
In some ways the nesting doesn't feel right, because not everything in metricsTime is always updated together. I think this is a pattern that hooks help discourage. But I'm not familiar with redux patterns, and maybe with redux it's more common/recommended? I decided to do this as a cheap solution since this is already large, but open to other suggestions.
Code quote:
metricsTime: {
// Start and end times to be used for metrics page graphs
currentWindow: TimeWindow;
// True if scale has changed since currentWindow was generated, and it should be re-generated from scale
shouldUpdateMetricsWindowFromScale: boolean;
// True if currentWindow should be unchanging. False if
isFixedWindow: boolean;
};
There was a problem hiding this comment.
Regarding your commit 0 suggestion with tests: the tests should be on the same commit where the change you would be testing was made
Reviewed 11 of 26 files at r3, all commit messages.
Reviewable status:
-- commits, line 13 at r3:
nit: I'm assuming you will correct the messages here to include description, issue number, release note, etc
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/utils.ts, line 115 at r3 (raw file):
// 0:00 to June 2 at 0:00 when the date is July 1 at 0:00 should return a custom timescale // instead of past day. // If the start is specified, and the window size matches
nit: add periods to your comments
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 24 at r3 (raw file):
export const SET_SCALE = "cockroachui/timewindow/SET_SCALE"; export const SET_METRICS_MOVING_WINDOW = "cockroachui/timewindow/SET_WINDOW"; export const SET_METRICS_FIXED_WINDOW = "cockroachui/timewindow/SET_RANGE";
can the values under cockroachui/timewindow/ also be changed? I think it could be confusing having the two different const for the same thing
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 66 at r3 (raw file):
* Specifically, when the end is "now", scale.fixedWindowEnd is null and metricsTime.currentWindow.end is a specific time * that is continually updated in order to do polling. */
can you make some way more clear the difference between comments you're adding just to ask questions during the review process and comments you intend to keep on the code itself?
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 68 at r3 (raw file):
*/ metricsTime: { // Start and end times to be used for metrics page graphs
nit: period
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 72 at r3 (raw file):
// True if scale has changed since currentWindow was generated, and it should be re-generated from scale shouldUpdateMetricsWindowFromScale: boolean; // True if currentWindow should be unchanging. False if
nit: false if?
pkg/ui/workspaces/db-console/src/views/app/containers/metricsTimeManager/index.tsx, line 28 at r3 (raw file):
} interface MetricsTimeManagerState {
will we ever want to use this component for any other uses besides the Metrics page? What is the specific on this component that we want to use this for the Metrics page instead of the other component being used everywhere else?
jocrl
force-pushed
the
de-duplicate-time-state
branch
3 times, most recently
from
caacf9e to
1ebb13e
Compare
There was a problem hiding this comment.
Got it, thanks!
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
nit: I'm assuming you will correct the messages here to include description, issue number, release note, etc
Yup, I will before un-drafting
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 182 at r5 (raw file):
I'm very confused about the fixedWindowEnd > moment.utc().subtract(currentScale.windowValid) clause. What is this testing, and how is windowValid used here?
The only other usage of windowValid is in the db console metrics page, where it is used to do continuous polling. This seems like a different usage.
The existing comment confuses me in that:
If the timescale extends into the future...
I almost wonder if the clause means to subtract windowSize (could be custom) instead of windowValid
Otherwise set the key to "Custom" so it appears correctly
This seems to suggest that we allow end times in the future if the windowSize is not one of the defaults. However, I cannot hit this code.
Code quote:
// If the timescale extends into the future then fallback to a default
// timescale. Otherwise set the key to "Custom" so it appears correctly.
if (
!windowEnd ||
windowEnd > moment.utc().subtract(currentScale.windowValid)
) {pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleTypes.ts, line 41 at r3 (raw file):
// is invalid if now > (currentWindow.end + windowValid). Default is ten // seconds. If fixedWindowEnd is set this is ignored. windowValid?: moment.Duration;
I want to modify the original comment below to reflect the actual windowValid in Cluster UI, but I don't understand the usage.
The original comment below (now > (currentWindow.end + windowValid)) describes the usage of windowValid in DB Console polling, which does not involve in this Cluster UI copy.
Instead, in Cluster UI is it used in the arrowClick function in timeScaleDropdown.tsx. I'm confused about that usage, and have added a comment-question there in timeScaleDropdown.tsx.
Code quote:
// The length of time the global time window is valid. The current time window
// is invalid if now > (currentWindow.end + windowValid). Default is ten
// seconds. If fixedWindowEnd is set this is ignored.
windowValid?: moment.Duration;pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 24 at r3 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
can the values under
cockroachui/timewindow/also be changed? I think it could be confusing having the two different const for the same thing
Thanks, done
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 66 at r3 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
can you make some way more clear the difference between comments you're adding just to ask questions during the review process and comments you intend to keep on the code itself?
Yup! I've moved all my questions from the code to reviewable comments. Everything in the code is now comments that I intend to have.
pkg/ui/workspaces/db-console/src/views/app/containers/metricsTimeManager/index.tsx, line 28 at r3 (raw file):
What is the specific on this component that we want to use this for the Metrics page instead of the other component being used everywhere else?
For comparison of everywhere else, the other two places of usage are currently the Statements and Transactions page, and the Severless Metrics page.
This component bakes in some specific assumptions that are only used in the DB Console Metrics page. They are:
- The use of states on
timeScale.metricsTime(the properties within it, not me naming it "metricsTime") in order to do polling. The Statements and Transactions, and severless Metrics pages use different mechanisms to do polling, and do not use the states that are hardcoded into the logic of this component. - The use of this component at all to do polling. This component works for polling by mounting a component that doesn't render anything. It's a weird pattern that I think comes from this being old 2016 code, that didn't have hooks available to share logic across components or sagas/thunk to do side effects. The Statements and Transactions, and severless Metrics pages both implement polling differently from both this component and each other, and there are even more other polling solutions scattered throughout db-console, cluster-ui, and managed-service. I don't know which polling solution would be recommended (that might be an interesting discussion to have). But I think component isn't the one to recommend.
- Use of the
windowValidproperty on the default time scale options (seetimeScaleDropdown/utils.ts) as a polling frequency. The logic of this component and thedefaultTimeScaleOptionsassume that if the "Past 10 Minutes" is selected we want a 10 second polling frequency; if "Past 30 Minutes" is selected we want 30 seconds. This frequency is tailored particularly to the needs of the DB Console metrics page. It is way too frequent for the Statements and Transactions page, and I suspect also isn't used for the severless Metrics page. We could refactor things to decouple the polling frequency from this component, but due to reason 2 I'm more inclined to isolate this component and ignore it.
Will we ever want to use this component for any other uses besides the Metrics page?
So yeah, given reason 2, I don't think we want to recommend this component as a solution for polling. If we do want to use this component for other uses in the future, I think it's better to leave it to that future usage to rename it and do the refactor (possibly taking into account use cases for that future use).
pkg/ui/workspaces/db-console/src/views/cluster/containers/timeScaleDropdownWithSearchParams/index.tsx, line 75 at r3 (raw file):
I'm not clear what sampleSize means and how it is used here. Comments on the type defintion of sampleSize say it is:
The expected duration of individual samples for queries at this time scale
This if block below says: if the time between now and the end (from query params, else now) is greater than the sample size.
I'm not clear what the meaning of this test is, and why it uses sampleSize (perhaps instead of windowSize?)
Code quote:
if (
moment.duration(now.diff(end)).asMinutes() >
timeScale.sampleSize.asMinutes()
) {
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 182 at r5 (raw file):
Previously, jocrl (Josephine) wrote…
I'm very confused about the
fixedWindowEnd > moment.utc().subtract(currentScale.windowValid)clause. What is this testing, and how iswindowValidused here?
The only other usage ofwindowValidis in the db console metrics page, where it is used to do continuous polling. This seems like a different usage.The existing comment confuses me in that:
If the timescale extends into the future...
I almost wonder if the clause means to subtract windowSize (could be custom) instead of windowValid
Otherwise set the key to "Custom" so it appears correctly
This seems to suggest that we allow end times in the future if the windowSize is not one of the defaults. However, I cannot hit this code.
edit: I'd like to understand the usage of windowValid in order to update the comment for this property in timeScaleTypes.ts
There was a problem hiding this comment.
This is quite dense. I only took a look at the first commit, I will come back to keep on looking later today once I have time.
Reviewed 3 of 26 files at r5.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 182 at r5 (raw file):
Previously, jocrl (Josephine) wrote…
edit: I'd like to understand the usage of
windowValidin order to update the comment for this property intimeScaleTypes.ts
Hmm looking at the db-console code, windowValid seems like should be really called dataValidityDurationPastCurrentWindow.
window end if now() is past this point, we need to issue another API call to load new data
v v
-------------------+-----------------------+------------+
^ ^~~~~~~~~~~~~~~~~~~~~~~~~
window start window valid duration
So, what this clause is checking, (that it seems like), can be rewrite as if (windowEnd + valid duration > now() )
In other word, I think what it's trying to do is, if we already have the time series data for the current time scale, we can just go ahead and use it.
pkg/ui/workspaces/db-console/src/views/cluster/containers/timeScaleDropdownWithSearchParams/index.tsx, line 75 at r3 (raw file):
Previously, jocrl (Josephine) wrote…
I'm not clear what
sampleSizemeans and how it is used here. Comments on the type defintion ofsampleSizesay it is:The expected duration of individual samples for queries at this time scale
This
ifblock below says: if the time between now and the end (from query params, else now) is greater than the sample size.
I'm not clear what the meaning of this test is, and why it usessampleSize(perhaps instead of windowSize?)
Hmm this really do puzzles me. The only case where findClosestTimeScale returns non Custom key is when it can find an option from the TimeScaleCollection, whose size exactly matches the second variable here.
So, it seems like, the code is is trying to do a couple of following:
- if
now < currentWindowEnd + sampleSize, use the default time scale option, and do continuous polling - else, don't bother (hence the
Customkey)
This is quite confusing and it seems like some other places would use windowValid variable instead.
There was a problem hiding this comment.
For sure, and it doesn't need to be today! There's no rush to look at this; I still need to write tests anyway. Thanks :)
It was definitely a brain exercise trying to understand the code, so I erred on more words trying to document my understanding. 100% appreciate suggestions to make things clearer and simpler.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 182 at r5 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Hmm looking at the
db-consolecode,windowValidseems like should be really calleddataValidityDurationPastCurrentWindow.window end if now() is past this point, we need to issue another API call to load new data v v -------------------+-----------------------+------------+ ^ ^~~~~~~~~~~~~~~~~~~~~~~~~ window start window valid durationSo, what this clause is checking, (that it seems like), can be rewrite as
if (windowEnd + valid duration > now() )In other word, I think what it's trying to do is, if we already have the time series data for the current time scale, we can just go ahead and use it.
Hmmm. I agree with your interpretation of the db-console usage, and that makes sense to me (also the diagram is great!). For this cluster-ui usage, I agree with rewriting the if condition as if (windowEnd + valid duration > now() ). But what the rest of the block does if true, is to move the end of the window to start at now(). In other words,
If this:
window end if now() is here
t0 v v
+------------------+---------------+-------------------+
^ ^~~~~~~~~~~~~~~~~~~~~~~~~
window start window valid duration
Turn it into this:
window has been shifted window end, == now()
t0 ----------> v
+----------------+-----------------+-------------------+
^ ^~~~~~~~~~~~~~~~~~~~~~~~~
window start window valid duration
I can see a motivation of setting to now() in that you don't window end to be in the future. So e.g. if the windowEnd would exceed now(), just set it to now(). However, a more accurate test for whether moving the window right once will exceed now, is to test if end + (distance between start and end) > now(). And that would be checking windowSize, not windowValid.
For instance, in practice, windowSize for the default options is always bigger than windowValid. So the diagram for the if test looks like this:
t0 window size window end if now() is here
<----------------->v v
+------------------+---------------+-------------------+
^ ^~~~~~~~~~~~~ |
window start window valid duration
|
<----------------->
window size duration
In this edge case, adding the valid duration does not exceed now(), but adding the window size does. And shifting the whole window right once would bring us past now(). So I wonder windowSize is what this code means to be testing? It would mean that windowValid is only used for the db console metrics polling.
I tried to trigger this edge case in the code but didn't manage to. If someone else wants to try/I might try more later.
pkg/ui/workspaces/db-console/src/views/cluster/containers/timeScaleDropdownWithSearchParams/index.tsx, line 75 at r3 (raw file):
Hmm. Yeah, so it seems to be saying if windowEnd is "close enough" to now(), just consider it now(). Where "close enough" is defined by being smaller that the sample size.
I'm not confident how "it is a moving now and we should do continuous polling" was determined in the past, but I'd like to make clearer by making it only controlled by the fixedWindowEnd property. I haven't thoroughly mapped out whenkey: "Custom" is or isn't in sync with that.
In my commit I always set fixedWindowEnd = false (i.e., moving now true), because three lines below windowEnd = null was always set.
I wonder if the conditional "Custom" key here mean that setting fixedWindowEnd = false should also be conditional...
FWIW, this is where the line was introduced. It looked like the previous code always set the "Custom" key, and the change was to fix the page not polling. (Which is also a bug I hit sometimes while refactoring! Excited to test 😛).
That PR also include a quote from @nathanstilwell that
this code is pretty complex and takes time to wrap my head around
so we're not alone hahaha. This is the second time Nathan that I'm seeing your review on a time component PR, so added you as a reviewer to this too.
jocrl
force-pushed
the
de-duplicate-time-state
branch
from
1ebb13e to
9726606
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/db-console/src/views/cluster/containers/timeScaleDropdownWithSearchParams/index.tsx, line 75 at r3 (raw file):
Previously, jocrl (Josephine) wrote…
Hmm. Yeah, so it seems to be saying if
windowEndis "close enough" tonow(), just consider itnow(). Where "close enough" is defined by being smaller that the sample size.I'm not confident how "it is a moving now and we should do continuous polling" was determined in the past, but I'd like to make clearer by making it only controlled by the
fixedWindowEndproperty. I haven't thoroughly mapped out whenkey: "Custom"is or isn't in sync with that.In my commit I always set
fixedWindowEnd = false(i.e., moving now true), because three lines belowwindowEnd = nullwas always set.I wonder if the conditional
"Custom"key here mean that settingfixedWindowEnd = falseshould also be conditional...FWIW, this is where the line was introduced. It looked like the previous code always set the
"Custom"key, and the change was to fix the page not polling. (Which is also a bug I hit sometimes while refactoring! Excited to test 😛).That PR also include a quote from @nathanstilwell that
this code is pretty complex and takes time to wrap my head around
so we're not alone hahaha. This is the second time Nathan that I'm seeing your review on a time component PR, so added you as a reviewer to this too.
edit: added comments on the type definitions about key: "Custom" and fixedWindowEnd being in sync, and a note to make an issue about possibly de-duplicating them
jocrl
force-pushed
the
de-duplicate-time-state
branch
from
9726606 to
ce14496
Compare
There was a problem hiding this comment.
Reviewed 3 of 6 files at r6.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 182 at r5 (raw file):
Previously, jocrl (Josephine) wrote…
Hmmm. I agree with your interpretation of the
db-consoleusage, and that makes sense to me (also the diagram is great!). For thiscluster-uiusage, I agree with rewriting the if condition asif (windowEnd + valid duration > now() ). But what the rest of the block does if true, is to move the end of the window to start at now(). In other words,If this:
window end if now() is here t0 v v +------------------+---------------+-------------------+ ^ ^~~~~~~~~~~~~~~~~~~~~~~~~ window start window valid durationTurn it into this:
window has been shifted window end, == now() t0 ----------> v +----------------+-----------------+-------------------+ ^ ^~~~~~~~~~~~~~~~~~~~~~~~~ window start window valid durationI can see a motivation of setting to now() in that you don't window end to be in the future. So e.g. if the windowEnd would exceed now(), just set it to now(). However, a more accurate test for whether moving the window right once will exceed now, is to test if
end + (distance between start and end) > now(). And that would be checkingwindowSize, notwindowValid.For instance, in practice,
windowSizefor the default options is always bigger thanwindowValid. So the diagram for the if test looks like this:t0 window size window end if now() is here <----------------->v v +------------------+---------------+-------------------+ ^ ^~~~~~~~~~~~~ | window start window valid duration | <-----------------> window size durationIn this edge case, adding the valid duration does not exceed now(), but adding the window size does. And shifting the whole window right once would bring us past
now(). So I wonderwindowSizeis what this code means to be testing? It would mean thatwindowValidis only used for the db console metrics polling.I tried to trigger this edge case in the code but didn't manage to. If someone else wants to try/I might try more later.
Hmm just to confirm, you are trying to trigger the else clause of the if (foundTiemScale) condition right?
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 163 at r8 (raw file):
switch (direction) { case ArrowDirection.RIGHT: if (fixedWindowEnd) {
Hmm since fixedWindowEnd here is derived from moment.utc(currentWindow.end), and currentWindow.end is derived from either currentScaled.fixedWindowEnd or now at the beginning of this component, the local variable of fixedWindowEnd ever be null ?
Or in other word, is this conditional check really needed?
Or maybe that was the reason why you mentioned in another comment below where you couldn't seem to hit that edge case?
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 171 at r8 (raw file):
break; case ArrowDirection.CENTER: // CENTER is used to set the time window to the current time.
I noticed that we are making multiple calls to moment.utc() throughout this function. (e.g. fixedWindowEnd = moment.utc() and fixedWindowEnd > moment.utc().subtract(currentScale.windowValid).
This might be subtle but each call to moment.utc() creates a slightly different timestamp as time progresses. I think it's worth for us to just only call moment.utc() once at the beginning of this function and store that timestamp in a variable.
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 74 at r3 (raw file):
Previously, jocrl (Josephine) wrote…
In some ways the nesting doesn't feel right, because not everything in metricsTime is always updated together. I think this is a pattern that hooks help discourage. But I'm not familiar with redux patterns, and maybe with redux it's more common/recommended? I decided to do this as a cheap solution since this is already large, but open to other suggestions.
Thoughts on inheriting the cluster-ui's TimeWindowState and introducing new additional fields?
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 115 at r8 (raw file):
} else { state.metricsTime.isFixedWindow = false; }
Hmm is there case where the payload explicitly has isFixedWindow field set?
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 124 at r8 (raw file):
state = _.cloneDeep(state); state.metricsTime.currentWindow = tw; state.metricsTime.shouldUpdateMetricsWindowFromScale = false;
Should we also update isFixedWindow = false here?
pkg/ui/workspaces/db-console/src/views/app/containers/metricsTimeManager/index.tsx, line 19 at r8 (raw file):
import _ from "lodash"; interface MetricsTimeManagerProps {
Ah this is awesome! Having overlapping names across two package can really makes it confusing.
jocrl
force-pushed
the
de-duplicate-time-state
branch
from
ce14496 to
85950fa
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/utils.ts, line 119 at r9 (raw file):
// Check if the end is before now. If so, it is a custom time. const end = moment.unix(startSeconds + seconds); if (end < moment()) {
This is just a clarifying rewrite. I've added test cases for this block and they pass both before and after the change.
Code quote:
// Check if the end is before now. If so, it is a custom time.
const end = moment.unix(startSeconds + seconds);
if (end < moment()) {pkg/ui/workspaces/db-console/src/views/cluster/containers/timeScaleDropdownWithSearchParams/index.tsx, line 75 at r3 (raw file):
Previously, jocrl (Josephine) wrote…
edit: added comments on the type definitions about
key: "Custom"andfixedWindowEndbeing in sync, and a note to make an issue about possibly de-duplicating them
Okay. Playing around with the difference in behavior between whether we include sampleSize in the comparison, I'm now more convinced by my "close enough" interpretation.
I believe that sampleSize here just provides a buffer. If the end is within the sampleSize tolerance, consider that close enough to now and just set things to now. This is because findClosestTimeScale only checks of the end is exactly equal to "now" . The use of the sampleSize buffer allows users to click left, click right, then quickly refresh and still have the dropdown selection as the past ten minutes instead of a fixed time frame. Using sampleSize instead of a fixed interval allows this tolerance buffer to be of different sizes depending on the size of the window, though I don't know if using specifically sampleSize is significant.
It does mean that I should set fixedWindowEnd to the end time. I've done so below, and added my explanation.
I am happy with this conclusion, unless anyone has anything else to add.
There was a problem hiding this comment.
Agreed this is quite dense. I appreciate your taking the time to wrap your head around it all, @jocrl!
Reviewed 26 of 26 files at r5, 6 of 6 files at r6, 20 of 26 files at r7, 3 of 3 files at r8, 3 of 3 files at r9, all commit messages.
Reviewable status:
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 62 at r9 (raw file):
* It is unclear if there are legitimate reasons for the two being out of sync. */ fixedWindowEnd: moment.Moment | false;
Seeing optional properties and reading through these usage instructions makes me wonder if it would be possible to hide this complexity inside an object somehow. I think there's enough in-flight on this PR that it wouldn't make sense to add here, just wanted to register the idea for the future.
jocrl
force-pushed
the
de-duplicate-time-state
branch
from
85950fa to
6dfd3aa
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 182 at r5 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Hmm just to confirm, you are trying to trigger the
elseclause of theif (foundTiemScale)condition right?
No, I'm not. I'm curious about that else block, but I'm not worried about that/it's not my main concern.
My main question is why windowValid is used here, in order to fix the comment on the windowValid type definition in cluster-ui. The comment that was previously there described it's usage in db-console. This is the only usage in cluster-ui, but it seems like this usage would be better off using windowSize. So I'm not sure what to put in the comments for windowValid on what it means.
It's just to try to fix misleading text in the comments. So, if this is confusing, I could just delete the comment line and describe our confusion instead.
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 163 at r8 (raw file):
Hmm since fixedWindowEnd here is derived from moment.utc(currentWindow.end), and currentWindow.end is derived from either currentScaled.fixedWindowEnd or now at the beginning of this component, the local variable of fixedWindowEnd ever be null ?
Or in other word, is this conditional check really needed?
Ah, agreed. I've renamed the local variable s/fixedWindowEnd/endTime, just to distinguish that it's not the fixedWindowEnd property. And I've added a comment explaining that we probably don't need the if block.
I'm trying not to change the logic of the code beyond the bare minimum that I need, unless it's something where the change is obviously equivalent and a comment would be more complicated. For potentially impossible code paths, I'm favoring adding comments instead of having to exhaustively verify that the path is not hit. Does just adding a comment sound good to you?
Or maybe that was the reason why you mentioned in another comment below where you couldn't seem to hit that edge case?
Mmm, renaming the variable and knowing that it's always truthy does change how you see that if section. I've added a comment there that the first part of the if clause should always be true.
if (!endTime || endTime > moment.utc().subtract(currentScale.windowValid)) {
I don't think it has to do with not being able to hit that edge case though (I still think that's cos of the right arrow being disabled when it would go through that code path). I'm not worried about hitting that edge case though, and talk more about this lack of worry in my reply for that.
Code quote:
ArrowDirection.RIGHTpkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 171 at r8 (raw file):
moment.utc()
I agree with the end goal! But I'm noticing that there are multiple calls to moment.utc() not just in the top-level of this component, but also in the functions that are defined in this file. Solving the multiple calls to moment.utc() in this file/component would require refactoring those functions and their tests to pass in a now variable.
That's not difficult to do, but this PR is getting really big. I have a "Potential future follow-up" in the PR description, to accumulate improvement ideas from this PR that I have no intention of doing with any priority, but that we could maybe one day do on a quick win day. Would you be comfortable shelving this in that section? I'm considering this potential bug "not any worse than before".
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 74 at r3 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Thoughts on inheriting the
cluster-ui'sTimeWindowStateand introducing new additional fields?
I think it's a good end goal, but this PR is getting big. Would you be down to shelve this under the "Potential future follow-up" section? I think it would make sense to do along with Matthew's comments a few lines above about simplifying the interface on TimeScale.
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 115 at r8 (raw file):
isFixedWindow
No, there isn't (only a default value in the constructor).
If you're not worried about major bugs coming from this, I'd rather ignore this weirdness for now (I'll put it in the "Potential future follow-up" section). This is just a 1-to-1 renaming of the existing implementation. At least it's only used in the metrics page, so the spaghetti is contained in the spaghetti plate 🙂.
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 124 at r8 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Should we also update
isFixedWindow = falsehere?
This is a direct rename of s/SET_WINDOW/SET_METRICS_MOVING_WINDOW. The prior implementation didn't set isFixedWindow = false, so I'm gonna say that we shouldn't here 😛. The logic for that is not the simplest and I'd rather not perturb the system. "Clean up the metrics page code" would be a pretty big task that I don't think is a priority.
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 62 at r9 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
Seeing optional properties and reading through these usage instructions makes me wonder if it would be possible to hide this complexity inside an object somehow. I think there's enough in-flight on this PR that it wouldn't make sense to add here, just wanted to register the idea for the future.
Thanks; agreed on all fronts. Will stick it in an issue for the improvement ideas that come out of this PR.
There was a problem hiding this comment.
Thanks so much for tackling this! 🙏
Reviewed 6 of 26 files at r10.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 34 at r5 (raw file):
export interface TimeScaleDropdownProps { currentScale: TimeScale; options?: DefaultTimesScaleOptions;
Can we rename this to just TimeScaleOptions, as from what I understand it's not just describing the type for the default, but any options we would want to pass here. Also I think there is s a double s here in TimesScale.
jocrl
force-pushed
the
de-duplicate-time-state
branch
from
6dfd3aa to
4a45a9c
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 34 at r5 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Can we rename this to just
TimeScaleOptions, as from what I understand it's not just describing the type for the default, but any options we would want to pass here. Also I think there is s a double s here inTimesScale.
Good catch; thanks! Done.
There was a problem hiding this comment.
Looks pretty good, thanks for the effort! Dropped just a couple of comments.
Reviewed 23 of 26 files at r12, 3 of 3 files at r13, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 167 at r13 (raw file):
* Thus, it seems like this `if` condition should always be true. */ if (endTime) {
A bit confused here, the if condition will always pass and even the comment indicates this..so is there a reason to keep it?
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/utils.ts, line 119 at r9 (raw file):
Previously, jocrl (Josephine) wrote…
This is just a clarifying rewrite. I've added test cases for this block and they pass both before and after the change.
This if check is a bit strange as we are comparing an epoch timestamp to a moment object. In your testing did the check ever come up as false? I ran some basic tests on this comparison in a moment sandbox and it always seemed to return true.
…cs graph Partially addresses cockroachdb#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 in `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 cockroachdb#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
jocrl
force-pushed
the
de-duplicate-time-state
branch
2 times, most recently
from
815420c to
e91b615
Compare
There was a problem hiding this comment.
Thanks everyone! Cleaned up and undrafted; PTAL 🙏
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 182 at r5 (raw file):
Previously, jocrl (Josephine) wrote…
No, I'm not. I'm curious about that
elseblock, but I'm not worried about that/it's not my main concern.My main question is why
windowValidis used here, in order to fix the comment on thewindowValidtype definition incluster-ui. The comment that was previously there described it's usage indb-console. This is the only usage incluster-ui, but it seems like this usage would be better off usingwindowSize. So I'm not sure what to put in the comments forwindowValidon what it means.It's just to try to fix misleading text in the comments. So, if this is confusing, I could just delete the comment line and describe our confusion instead.
Modified the comment.
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 167 at r13 (raw file):
Previously, Santamaura (Alex Santamaura) wrote…
A bit confused here, the if condition will always pass and even the comment indicates this..so is there a reason to keep it?
Not much reason, other than that trying not change the code beyond what I needed to. You and Archer both brought this up though, so I've removed it.
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleTypes.ts, line 41 at r3 (raw file):
Previously, jocrl (Josephine) wrote…
I want to modify the original comment below to reflect the actual
windowValidin Cluster UI, but I don't understand the usage.
The original comment below (now > (currentWindow.end + windowValid)) describes the usage ofwindowValidin DB Console polling, which does not involve in this Cluster UI copy.
Instead, in Cluster UI is it used in thearrowClickfunction intimeScaleDropdown.tsx. I'm confused about that usage, and have added a comment-question there intimeScaleDropdown.tsx.
Decided to just delete that part of the comment.
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/utils.ts, line 119 at r9 (raw file):
Previously, Santamaura (Alex Santamaura) wrote…
This if check is a bit strange as we are comparing an epoch timestamp to a moment object. In your testing did the check ever come up as false? I ran some basic tests on this comparison in a moment sandbox and it always seemed to return true.
Hmm. Aren't both moment objects?
end: https://momentjs.com/docs/#/parsing/unix-timestamp/
moment(): https://momentjs.com/docs/#/parsing/now/
This particular test causes this block to come up as false, which I've confirmed by adding an else block and a console log.
That said, I'm not thoroughly sure but I don't think it's actually hit in the code, due to the ways that findClosestTimeScale is called. startSeconds needs to be passed for the outer if block to be true, and in the only time that is passed it is always custom.
In theory there might be more we could do to remove redundant paths and logic, but I'm trying not to do more than I need here.
There was a problem hiding this comment.
I just added some tiny nits, otherwise make sure you addressed all the comments from other reviewers and get approval too
Reviewed 19 of 26 files at r12, 1 of 3 files at r13, 5 of 5 files at r14, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timescale.spec.tsx, line 209 at r14 (raw file):
describe("findClosestTimeScale", () => { it("should find the correct time scale", () => { // `seconds` != window size of any of the default options, `startSeconds` not specified
nit: period on those comments (more below)
pkg/ui/workspaces/db-console/src/redux/timeScale.ts, line 93 at r14 (raw file):
}; this.metricsTime = { // This is explicitly initialized as undefined to match the prior implementation while satisfying Typescript
nit: period
Partially addresses cockroachdb#75579 Previously, there were two properties on `timewindow` state: `scale` and `currentWindow`. It turns out that: 1) `currentWindow` is not used in cluster-ui 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 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 For a more in-depth description, see the PR description. Release note: None
jocrl
force-pushed
the
de-duplicate-time-state
branch
from
e91b615 to
ad6e433
Compare
There was a problem hiding this comment.
Thanks! Addressed, and bumping @matthewtodd, @Azhng, @Santamaura, and @xinhaoz. PTAL 🙏
Reviewable status:
There was a problem hiding this comment.
Reviewed 2 of 2 files at r15, all commit messages.
Reviewable status:
| !windowEnd || | ||
| windowEnd > moment.utc().subtract(currentScale.windowValid) | ||
| ) { | ||
| // The first `!endTime` part of the if` clause seems unnecessary since endTime is always a specific time. |
There was a problem hiding this comment.
minor nit: extra "`" after "if"
| * This (or the parent if block) is *not* hit by: | ||
| * - Select a default time, click left, select a custom time of the same range, then click right. The arrow is | ||
| * not disabled, but the clause doesn't seem to be true. | ||
| */ |
There was a problem hiding this comment.
super small nit: is changing the block comment style here on purpose?
There was a problem hiding this comment.
Reviewed 18 of 26 files at r12, 1 of 3 files at r13, 5 of 5 files at r14, 2 of 2 files at r15, all commit messages.
Reviewable status:
Finishes addressing cockroachdb#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
jocrl
force-pushed
the
de-duplicate-time-state
branch
from
ad6e433 to
5b5189b
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 178 at r15 (raw file):
Previously, Azhng (Archer Zhang) wrote…
minor nit: extra "`" after "if"
fixed
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 192 at r15 (raw file):
Previously, Azhng (Archer Zhang) wrote…
super small nit: is changing the block comment style here on purpose?
Yes, it was on purpose since it's getting long. I think neither javascript nor or code conventions is opinionated on which to use in the frontend, e.g. https://github.com/cockroachdb/cockroach/blob/master/pkg/ui/workspaces/cluster-ui/src/util/proto.ts#L89
|
pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/timeScaleDropdown.tsx, line 192 at r15 (raw file):
|
|
Thank you everyone! |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
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.
Commits
Commit 1 ui: Always write to
windowEndonscale; read fromscalefor the metrics graph.Partially addresses #75579
Previously,
windowEndonscalewas not always written to, meaning that it was not always safe/correct to read off thescaleproperty.currentWindowwasn't always safe to use either.This commit modifies
windowEndto 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 fromscaleinstead ofcurrentWindowThere was a comment where
currentWindowis used to fix a bug on the metrics page becausescalewas 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
windowEndproperty was not set (this commit changeslinegraph/index.tsxto set it). Yet, even without changinglinegraph/index.tsxto setwindowEnd, reading the start and end times fromscaledid not recreate the drag-to-zoom granularity bug. However, reading fromscalewithout settingwindowEnddoes cause a separate major bug in that the timescale endpoint is always queried withnowas 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.endthe single source of truthPartially addresses #75579
Previously, there are two properties on
timewindowstate,scaleandcurrentWindow. They seemed like duplicates (see #75579 issue for further explanation). It was unclear what the distinction was, if any.currentWindowis not used in cluster-uicurrentWindowproperty on state is only ever read from in this test. 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 thecurrentWindowstate, even if there are a lot of local variables that are namedcurrentWindow.TimeWindowis used as a type, but is not used on state.currentWindow.enddiffers fromscale.windowEndin that the former is always a specific time which is used to do polling in the db console metrics pagescale.windowEndisnull, andcurrentWindow.endis a specific time, e.g. 3.51pm on a Monday.currentWindow.endis continually updated according to thewindowValidfrequency on the closestDefaultTimeScaleOption. The continual update ofcurrentWindow.endis used to poll for new data for graphs on the metrics page.currentWindow.end(and actually, also don't use thewindowValidfrequency).currentWindow.endis only used for the db console metrics page.scaleChangedanduseTimeRangeare also only used for the db console metrics page, and help with bookkeeping around updatingcurrentWindow.end.This commit thus
currentWindow,scaleChanged, anduseTimeRangestates in cluster-uicurrentWindow,scaleChanged, anduseTimeRangestates under themetricsTimeproperty in db-consoleRelease 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->fixedWindowEndin the managed-service repo.The following renames were made:
scaleChanged->shouldUpdateMetricsWindowFromScaleuseTimeRange->isFixedWindowSET_WINDOW->SET_METRICS_MOVING_WINDOW,setTimeWindow->setMetricsMovingWindowSET_RANGE->SET_METRICS_FIXED_WINDOW,setTimeRange->setMetricsFixedWindowwindowEnd->fixedWindowEnd, with its type changed frommoment.Moment | nulltomoment.Moment | false.falseindicates 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 thescaleproperty is the state to use, and thatTimeWindowis 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 theTimeWindowtype interface, name it for the main manager component in the file, and make it clear that this is just for the metrics pagedb-console/src/views/cluster/containers/timescale/index.tsx->db-console/src/views/cluster/containers/timeScaleDropdownWithSearchParams/index.tsx, and the sibling.stylfileRelease note: None