-
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: improves explanation of log rate spike/dip #186342
[ML] AIOps Log Rate Analysis: improves explanation of log rate spike/dip #186342
Conversation
Pinging @elastic/ml-ui (:ml) |
? `${Math.round((deviationBucketRate / baselineBucketRate) * 100) / 100}x higher` | ||
: `${Math.round((baselineBucketRate / deviationBucketRate) * 100) / 100}x lower` |
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.
Guess we also need to add i18n to those strings.
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.
Can you use https://github.com/elastic/kibana/blob/main/x-pack/plugins/ml/common/util/metric_change_description.ts for these, as used for the anomalies table? That will use 'More than 100x higher' if the factor is > 100
, which is probably appropriate to use here too? Or if you can't call that same code, then use the same rules around precision so that e.g. x14.23 higher
becomes x14 higher
.
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 to round and to add localization in ff861eb
) { | ||
const logRateChange = | ||
baselineBucketRate > 0 | ||
? analysisType === LOG_RATE_ANALYSIS_TYPE.SPIKE |
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.
Our style guide recommends not doing nested ternaries. I'd suggest just using if/else in this function and return early if something matches.
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 ff861eb
const { | ||
analysisType, | ||
windowParameters, | ||
documentStats: { documentCountStats }, | ||
} = useAppSelector((s) => s.logRateAnalysis); |
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.
Since useColumns
is a hook, you could move this inside the hook itself so doesn't need to get passed on as arguments to useColumns
.
const { | ||
analysisType, | ||
windowParameters, | ||
documentStats: { documentCountStats }, | ||
} = useAppSelector((s) => s.logRateAnalysis); |
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.
Since useColumns
is a hook, you could move this inside the hook itself so doesn't need to get passed on as arguments to useColumns
.
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 - moved in ff861eb
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_results_table/use_columns.tsx
Show resolved
Hide resolved
sortable: false, | ||
valign: 'middle', | ||
}, | ||
['Log rate change']: { |
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.
Can the Log rate change
column be made sortable? I think the similar column in the anomalies table sorts by size of factor of increase / decrease.
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 ff861eb
Since we have the pValue - which is an indication of the impact - I used that to sort 👍
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.
Dosen't look like pValue
always correlates to the size of the change.
I think the sortable
prop can be ((item: T) => Primitive)
as well as a simple boolean and then you can use logic like https://github.com/elastic/kibana/blob/main/x-pack/plugins/ml/server/models/results_service/build_anomaly_table_items.js#L105 to return the factor difference (or maybe you already know the change up front?).
const [skippedColumns, setSkippedColumns] = useState<ColumnNames[]>(['p-value']); | ||
const [skippedColumns, setSkippedColumns] = useState<ColumnNames[]>([ | ||
'p-value', | ||
'Baseline rate', |
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.
@alvarezmelissa87 @walterra what do you think about showing the Baseline and Deviation rate columns by default, and hiding the Doc count? I like to see these columns in combination with the 'Log rate change' column. Although in the grouped view, Doc count makes sense as the new columns don't have values.
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.
🤔 I think that could work but we'd need all 3 showing up since the groups table and expanded row hare the same column controls. Might need to think about it a bit more if we want to make them independent.
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.
I'm happy to leave it as it is, with baseline and deviation rate hidden by default. The more I think about it, the less I like the idea of adding two extra numeric columns into the table in the default view.
.../aiops/public/components/log_rate_analysis_results_table/get_baseline_and_deviation_rates.ts
Outdated
Show resolved
Hide resolved
@@ -186,7 +186,11 @@ export const LogRateAnalysisResults: FC<LogRateAnalysisResultsProps> = ({ | |||
); | |||
const [shouldStart, setShouldStart] = useState(false); | |||
const [toggleIdSelected, setToggleIdSelected] = useState(resultsGroupedOffId); | |||
const [skippedColumns, setSkippedColumns] = useState<ColumnNames[]>(['p-value']); | |||
const [skippedColumns, setSkippedColumns] = useState<ColumnNames[]>([ |
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.
Not related to the changes in this PR, but I think it would be a nice enhancement to persist the user's choice of selected columns to e.g. local storage. I found myself adding in Baseline and Deviation rates for display every time I reran an analysis.
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.
Added as an item to #181111.
@elasticmachine merge upstream |
This has been updated and is ready for another look when you get a chance 🙏 cc @peteharverson, @walterra |
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.
Gave the latest changes a test. All looks good - only item to look at is the way sorting on the change column works.
.../aiops/public/components/log_rate_analysis_results_table/get_baseline_and_deviation_rates.ts
Outdated
Show resolved
Hide resolved
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.
Thanks for taking a look, @peteharverson 🙏 Adding sorting for that computed Log rate change is a can of worms as it requires us to make some changes to the custom sorting we've got going on now and will likely require a bit of code churn. |
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
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
Related issue: #182714
This PR adds a
Log rate change
column to Log rate analysis results table.The log rate change is calculated by getting the number of buckets for baseline/deviation using the timerange and interval and then comparing the average rates per bucket for baseline vs deviation.
Checklist
Delete any items that are not applicable to this PR.