-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] AIOps Log Rate Analysis: adds controls for controlling which columns will be visible #184262
[ML] AIOps Log Rate Analysis: adds controls for controlling which columns will be visible #184262
Conversation
@elasticmachine merge upstream |
x-pack/plugins/aiops/public/components/log_rate_analysis/item_filter_popover.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/aiops/public/components/log_rate_analysis/item_filter_popover.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/aiops/public/components/log_rate_analysis/item_filter_popover.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/aiops/public/components/log_rate_analysis_results_table/use_columns.tsx
Show resolved
Hide resolved
x-pack/plugins/aiops/public/components/log_rate_analysis/log_rate_analysis_results.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/aiops/public/components/log_rate_analysis_results_table/use_columns.tsx
Show resolved
Hide resolved
x-pack/plugins/aiops/public/components/log_rate_analysis/log_rate_analysis_results.tsx
Outdated
Show resolved
Hide resolved
const disabledColumnFilterApplyButtonTooltipContent = i18n.translate( | ||
'xpack.aiops.analysis.columnSelectorNotEnoughColumnsSelected', | ||
{ | ||
defaultMessage: 'At least one column must be selected.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the 'at least one' correct? It seems to require at least two columns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - fixed in 72f120e
x-pack/plugins/aiops/public/components/log_rate_analysis/item_filter_popover.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/ml-ui (:ml) |
@elasticmachine merge upstream |
x-pack/plugins/aiops/public/components/log_rate_analysis/log_rate_analysis_results.tsx
Show resolved
Hide resolved
itemSearchAriaLabel={columnSearchAriaLabel} | ||
initialSkippedItems={skippedColumns} | ||
popoverButtonTitle={columnsButton} | ||
selectedItemLimit={1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed this issue in 7f58298
It now takes into account the incoming skipped items 👍
This is ready for a final look when you get a chance 🙏 cc @peteharverson, @qn895 |
'xpack.aiops.logRateAnalysis.resultsTable.logRateColumnTooltip', | ||
{ | ||
defaultMessage: | ||
'A visual representation of the impact of the field on the message rate difference', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Does this message need to end with a period?
const groupImpactMessage = i18n.translate( | ||
'xpack.aiops.logRateAnalysis.resultsTableGroups.impactLabelColumnTooltip', | ||
{ | ||
defaultMessage: 'The level of impact of the group on the message rate difference', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Does this message need to end with a period?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested latest changes and LGTM
@@ -157,6 +193,7 @@ export const LogRateAnalysisResults: FC<LogRateAnalysisResultsProps> = ({ | |||
); | |||
const [shouldStart, setShouldStart] = useState(false); | |||
const [toggleIdSelected, setToggleIdSelected] = useState(resultsGroupedOffId); | |||
const [skippedColumns, setSkippedColumns] = useState<string[]>(['p-value']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if this could be ColumnNames[]
rather than string. It'll probably mean changing all places where they are stored as a string array, but it'd be useful to use the types to enforce that only the correct values are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in c1876b9
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Related meta issue: #182714
This PR adds controls to the AIOps results table to show/hide columns.
Checklist
Delete any items that are not applicable to this PR.