ui: add selected period as part of cached key #77450
Conversation
|
This change is |
There was a problem hiding this comment.
Hmm seems like a couple unit tests are failing
Reviewed 5 of 9 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)
-- commits, line 42 at r2:
nit: dup release justification
pkg/ui/workspaces/cluster-ui/src/util/appStats/appStats.ts, line 302 at r2 (raw file):
if (apps[i].includes("$ internal")) { apps[i] = "$ internal"; }
I just realized that, wouldn't we have multiple $ internal app names in the app_names field in this case 🤔, Perhaps we can use a Set here?
pkg/ui/workspaces/db-console/src/redux/apiReducers.ts, line 15 at r2 (raw file):
import moment from "moment"; import { util } from "@cockroachlabs/cluster-ui"; const { generateStmtDetailsToID } = util;
Hmm, can we directly import generateStmtDetailsToID here? Since appStats package does have a top-level index.ts for package wide export
maryliag
force-pushed
the
changetime
branch
2 times, most recently
from
562d368 to
6f287bb
Compare
There was a problem hiding this comment.
Other than test failures,
Reviewed 5 of 9 files at r2, 30 of 30 files at r3, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
Previously, Azhng (Archer Zhang) wrote…
nit: dup release justification
Fixed
pkg/ui/workspaces/cluster-ui/src/util/appStats/appStats.ts, line 302 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
I just realized that, wouldn't we have multiple
$ internalapp names in theapp_namesfield in this case 🤔, Perhaps we can use aSethere?
Fixed
pkg/ui/workspaces/db-console/src/redux/apiReducers.ts, line 15 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Hmm, can we directly import
generateStmtDetailsToIDhere? SinceappStatspackage does have a top-levelindex.tsfor package wide export
The util is imported as its own name, and that one has its own imported files, so for the utils and api we need to import this way
Code quote:
const { generateStmtDetailsToID } = util;
Previously, the fingerprint id and the app names were used as a key for a statement details cache. This commits adds the start and end time (when existing) to the key, so the details are correctly assigned to the selected period. This commit also rounds the selected value period to the hour, since that is what is used on the persisted statistics, with the start value keeping the hour and the end value adding one hour, for example: start: 17:45:23 -> 17:00:00 end: 20:14:32 -> 21:00:00 Partially addresses cockroachdb#72129 Release note: None Release Justification: Low risk, high benefit change
There was a problem hiding this comment.
Reviewed 1 of 3 files at r4.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/util/dataFromServer.ts, line 20 at r5 (raw file):
OIDCAutoLogin: boolean; OIDCLoginEnabled: boolean; OIDCButtonText: string;
Hmm, what are these new fields for 🤔, are they related to the CI test failures somehow ?
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/util/dataFromServer.ts, line 20 at r5 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Hmm, what are these new fields for 🤔, are they related to the CI test failures somehow ?
related to test failures
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/util/dataFromServer.ts, line 20 at r5 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
related to test failures
Out of curiosity, how did this fix the CI failure ?
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/util/dataFromServer.ts, line 20 at r5 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Out of curiosity, how did this fix the CI failure ?
This interface exists on both db console and cluster-ui, and the cluster-ui was missing those fields. Our guess is that it was always importing the one from db console first (for tests, etc), but I added a new dependency on package.json and that must have changed some order of import and was getting the one from here first, which was missing those fields and creating the failure.
It's weird that never happened before but it was bound to happen any day and my Pr was the "lucky" one
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/util/dataFromServer.ts, line 20 at r5 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
This interface exists on both db console and cluster-ui, and the cluster-ui was missing those fields. Our guess is that it was always importing the one from db console first (for tests, etc), but I added a new dependency on package.json and that must have changed some order of import and was getting the one from here first, which was missing those fields and creating the failure.
It's weird that never happened before but it was bound to happen any day and my Pr was the "lucky" one
that's pretty interesting :D
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.ts, line 32 at r5 (raw file):
action: PayloadAction<StatementDetailsRequest>, ): any { const key = action?.payload
So I'm suspecting that in your #77215 PR which added this function, that this action?.payload ... : "" was added in order to mimic the getStatements vs getCombinedStatements forking in sqlStats.sagas.ts's requestSQLStatsSaga.
Now that we know what that forking was about, should there actually be a forking here? Is there actually a use case where the key should be ""? I'm thinking not since action in the function signature is required, and because I'm not sure what a "" key would mean.
Code quote:
const key = action?.payload
? generateStmtDetailsToID(
action.payload.fingerprint_id,
action.payload.app_names.toString(),
action.payload.start,
action.payload.end,
)
: "";pkg/ui/workspaces/cluster-ui/src/timeScaleDropdown/utils.ts, line 90 at r5 (raw file):
}; export const toRoundedDateRange = (
Could you add a comment explaining the cluster setting in the back end that this is tied to?
Separately, I know we don't currently expose the setting to users. But does this front end assumption of the aggregation interval (I think it's aggregation interval?) limit our ability to develop and test front end functionality? E.g., if we want to generate data to test that aggregation intervals are correctly combined, one way would be to lower the aggregation interval so that you don't actually have to wait an hour. Or if you want to generate data to test that the time picker works correctly to retrieve different data.
If this rounding does affect dev-ability, one could in theory modify this function to change the number when developing. But that becomes one more thing to have to know to change, and runs the risk of accidentally being checked in + administrative back and forth if PRs accidentally check it in.
|
bors r+ |
|
Build succeeded: |
Previously, the fingerprint id and the app names were used
as a key for a statement details cache. This commits adds
the start and end time (when existing) to the key, so
the details are correctly assigned to the selected period.
This commit also rounds the selected value period to the hour,
since that is what is used on the persisted statistics, with
the start value keeping the hour and the end value adding one
hour, for example:
start: 17:45:23 -> 17:00:00
end: 20:14:32 -> 21:00:00
Partially addresses #72129
Release note: None
Release Justification: Low risk, high benefit change