ui: add missing filters for workload insights and active execution pages #94002
Conversation
|
This change is |
There was a problem hiding this comment.
Nice! Would you mind checking if you select more than one value, if it's also working?
Reviewed 19 of 21 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @gtr)
pkg/ui/workspaces/cluster-ui/src/insights/utils.ts line 64 at r1 (raw file):
if (filters.workloadInsightType) { const workloadInsightTypes = filters.workloadInsightType.toString().split(","); if (workloadInsightTypes.includes(unset)) {
you don't need this. This is used on app name, when no app name was set, so we want to all the "unset"value there. This is not the case for insights type
pkg/ui/workspaces/cluster-ui/src/insights/utils.ts line 219 at r1 (raw file):
if (filters.workloadInsightType) { const workloadInsightTypes = filters.workloadInsightType.toString().split(","); if (workloadInsightTypes.includes(unset)) {
same comment as above
pkg/ui/workspaces/cluster-ui/src/recentExecutions/recentStatementUtils.ts line 71 at r1 (raw file):
filteredStatements = filteredStatements.filter((statement: RecentStatement) => { const executionStatuses = filters.executionStatus.toString().split(","); if (executionStatuses.includes(unset)) {
ditto
pkg/ui/workspaces/cluster-ui/src/recentExecutions/recentStatementUtils.ts line 223 at r1 (raw file):
filteredTxns = filteredTxns.filter((txn: RecentTransaction) => { const executionStatuses = filters.executionStatus.toString().split(","); if (executionStatuses.includes(unset)) {
ditto
pkg/ui/workspaces/cluster-ui/src/selectors/recentExecutions.selectors.ts line 49 at r1 (raw file):
selectRecentStatements, (statements: RecentStatement[]) => Array.from(new Set(statements.map(statement => statement.status))),
should we list only options that have results or all options available? wdyt @kevin-v-ngo (same comment for the recent executions and for the insights types)
There was a problem hiding this comment.
No problem, I updated the README with an additional demo showing multiple selections for both pages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/insights/utils.ts line 64 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you don't need this. This is used on app name, when no app name was set, so we want to all the "unset"value there. This is not the case for insights type
Done.
pkg/ui/workspaces/cluster-ui/src/insights/utils.ts line 219 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
same comment as above
Done.
pkg/ui/workspaces/cluster-ui/src/recentExecutions/recentStatementUtils.ts line 71 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
ditto
Done.
pkg/ui/workspaces/cluster-ui/src/recentExecutions/recentStatementUtils.ts line 223 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
ditto
Done.
pkg/ui/workspaces/cluster-ui/src/selectors/recentExecutions.selectors.ts line 49 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
should we list only options that have results or all options available? wdyt @kevin-v-ngo (same comment for the recent executions and for the insights types)
It looks like there are only three possible states for the execution status.
export type ExecutionStatus = "Waiting" | "Executing" | "Preparing";We could easily import this type into the Filter component. We could also display a message when the user selects a status for which there are no recent statements.
Same thing with the insight types, only six possible states:
export enum InsightNameEnum {
failedExecution = "FailedExecution",
highContention = "HighContention",
highRetryCount = "HighRetryCount",
planRegression = "PlanRegression",
suboptimalPlan = "SuboptimalPlan",
slowExecution = "SlowExecution",
} A lot of the other options for the filter components (such as app name) only list options for which we have results but since the options for execution status and insight types are pre-determined from the types, I think it makes sense to list all options.
gtr
force-pushed
the
missing-filters
branch
2 times, most recently
from
a509ee1
to
7ba4110
Compare
It would be great if we can list all the options (even if there aren't results). For instance, for troubleshooting a slow active workload, it's a more explicit/clear experience that tells user that they currently have no "Waiting" executions with contention. They don't need to infer based on the lack of the option. FYI @dongniwang to review this PR as well if she has any feedback |
|
This can be optimized by moving the length check up to the top with the initial if. This avoids the overhead of iterating the list when there is nothing to filter. This can be optimized further by doing the following. Why are we doing includes to find the string instead of just comparing the array values? |
|
Previously, kevin-v-ngo wrote…
+1 to always listing all options. |
There was a problem hiding this comment.
Reviewed 4 of 7 files at r2, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/store/insights/transactionInsights/transactionInsights.selectors.ts line 28 at r2 (raw file):
state.adminUI.transactionInsights?.lastError; export const selectTransactionInsightTypes = createSelector(
same comment here as the recent executions, list all existing types, not just the ones that have results for
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/insights/utils.ts line 225 at r2 (raw file):
Previously, j82w (Jake) wrote…
This can be optimized by moving the length check up to the top with the initial if. This avoids the overhead of iterating the list when there is nothing to filter.
if (filters.workloadInsightType && filters.workloadInsightType.Length > 0) { ....This can be optimized further by doing the following.
filteredStatements = filteredStatements.filter( statementInsight => // avoid the map and includes if there are no insights for this statement. if (statementInsight.insights.Length === 0){ return false; } // Move generating the string outside of the includes to avoid recomputing for each workloadInsightType. const stmtInsights = statementInsight.insights.map(insight => insight.label).toString(); workloadInsightTypes.includes(stmtInsights),Why are we doing includes to find the string instead of just comparing the array values?
filteredStatements = filteredStatements.filter(statementInsight => statementInsight.insights.some(stmtInsight => workloadInsightTypes.some( workloadType => workloadType === stmtInsight.label, ), ), );
Good catch, I updated this part of the filtering. TFTR!
pkg/ui/workspaces/cluster-ui/src/selectors/recentExecutions.selectors.ts line 49 at r1 (raw file):
Previously, j82w (Jake) wrote…
+1 to always listing all options.
Done. I also updated the conditions so that we get a "No transactions/statements match your search." message when we pick a status/insight type for which we don't have entries.
pkg/ui/workspaces/cluster-ui/src/store/insights/transactionInsights/transactionInsights.selectors.ts line 28 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
same comment here as the recent executions, list all existing types, not just the ones that have results for
Done! FYI for ExecutionStatus, I made it an enum so that it would be easier to turn into an array and adjusted the usages accordingly.
Code quote (from pkg/ui/workspaces/cluster-ui/src/insights/utils.ts):
if (filters.workloadInsightType)
|
Thanks @gtr ! Looks good to me. Just a nit picking - would it be possible to bring the dropdown selection menu on top of the filter dropdown? See attached screenshot. The menu for selecting "Insights" is one layer above the "Filter" dropdown per initial design. Let me know if you have any questions. Thanks again! |
|
Updated to fix CI build errors.
Good catch, I think we might need to open a new PR for this since we'd have to change the behavior of all dropdown menus in the UI but I'll look into it, thanks! |
|
Updated to add the definition for the "waiting" execution status in the active executions pages' status tooltip: |
There was a problem hiding this comment.
Looks great! Can you just add a demo with this changes working on CC Console?
Reviewed 10 of 20 files at r3, 5 of 7 files at r4, 6 of 6 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status:
-- commits line 13 at r6:
nit: in the future try keeping separate changes (specially on different pages) to different PRs, which makes it easier to review. You can keep this one as is
pkg/ui/workspaces/cluster-ui/src/recentExecutions/recentStatementsSection.tsx line 104 at r6 (raw file):
renderNoResult={ <EmptyStatementsPlaceholder isEmptySearchResults={search?.length > 0 || activeFilters > 0}
you still need the check of statements.length, but the correct is statements.length === 0. What this is doing is checking if you have no results because there is a search/filter, so you need to check if there is a search or if there are active filters, and you also need to check if indeed there is no results
pkg/ui/workspaces/cluster-ui/src/recentExecutions/recentTransactionsSection.tsx line 103 at r6 (raw file):
renderNoResult={ <EmptyTransactionsPlaceholder isEmptySearchResults={search?.length > 0 || activeFilters > 0}
same thing as the other comment
There was a problem hiding this comment.
Updated description to add demos for CC console.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/recentExecutions/recentStatementsSection.tsx line 104 at r6 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you still need the check of statements.length, but the correct is
statements.length === 0. What this is doing is checking if you have no results because there is a search/filter, so you need to check if there is a search or if there are active filters, and you also need to check if indeed there is no results
Thanks for catching this. Updated.
pkg/ui/workspaces/cluster-ui/src/recentExecutions/recentTransactionsSection.tsx line 103 at r6 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
same thing as the other comment
Done.
|
This is looking great! @gtr, we don't have "PlanRegression" yet? And nit: "High Waiting Time" - > High Contention. It should match with the Insights column values. And where is Suboptimal plan? We don't have Full Scan as an insight either. CC @dongniwang for another set of eyes |
Thanks! I updated the demo video link in the description (here again for convenience). |
gtr
force-pushed
the
missing-filters
branch
4 times, most recently
from
4e49fea
to
631cea4
Compare
There was a problem hiding this comment.
Just a small nit from me.
Also the link for the demo on filtering by execution status on transactions tab is giving 404.
Assuming this test is also working and the small nit:
Reviewed 10 of 13 files at r7, 1 of 2 files at r8, 3 of 3 files at r9, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/statementInsights/statementInsightsView.tsx line 306 at r9 (raw file):
filteredStatements?.length === 0) || (countActiveFilters > 0 && filteredStatements?.length === 0)
nit you can update to
(search?.length > 0 || countActiveFilters > 0 ) && filteredStatements?.length === 0
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/transactionInsights/transactionInsightsView.tsx line 277 at r9 (raw file):
filteredTransactions?.length === 0) || (countActiveFilters > 0 && filteredTransactions?.length === 0)
nit you can update to
(search?.length > 0 || countActiveFilters > 0 ) && filteredStatements?.length === 0
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/statementInsights/statementInsightsView.tsx line 306 at r9 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit you can update to
(search?.length > 0 || countActiveFilters > 0 ) && filteredStatements?.length === 0
Done.
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/transactionInsights/transactionInsightsView.tsx line 277 at r9 (raw file):
search?.length > 0 || countActiveFilters > 0
Done.
Fixes cockroachdb#87741, cockroachdb#87783. Previously, the workload insights page was missing a filter for insights type for both the statement and transaction tabs. Furthermore, the active execution pages for statement and transactions was also missing a status filter. This commit adds the ability to filter by insights type for the workload insights page and the ability to filter by execution status for the active execution pages. Release note (ui change): added an insights type filter for the workload insights page, added a execution status filter for the active execution pages.
|
bors r+ |
|
Build succeeded: |
Fixes #87741, #87783.
Previously, the workload insights page was missing a filter for insights
type for both the statement and transaction tabs. Furthermore, the
active execution pages for statement and transactions was also missing a
status filter.
This commit adds the ability to filter by insights type for the workload
insights page and the ability to filter by execution status for the
active execution pages.
DB Console video demos:
CC Console video demos
Release note (ui change): added an insights type filter for the workload
insights page, added a execution status filter for the active execution
pages. Updated the tooltip for execution status to include the definition
for the "Waiting" status.