Skip to content
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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Jun 24, 2024
Merged

[ML] AIOps Log Rate Analysis: improves explanation of log rate spike/dip #186342

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5dac06b
add log rate change column
alvarezmelissa87 Jun 17, 2024
ee5e116
remove median calculation. calculate rate based on interval and range
alvarezmelissa87 Jun 18, 2024
383fc67
fix translation
alvarezmelissa87 Jun 18, 2024
8f0a4ee
Merge branch 'main' into ml-aiops-additional-columns
kibanamachine Jun 20, 2024
ff861eb
fix translations for log rate message and add sorting
alvarezmelissa87 Jun 20, 2024
2686407
fix log rate calculation for dip
alvarezmelissa87 Jun 20, 2024
37471c4
ensure baseline/deviation rate columns show up for dip
alvarezmelissa87 Jun 20, 2024
31ac8ab
update functional tests
alvarezmelissa87 Jun 21, 2024
51abcd4
add singular/plural for translation and remove sorting from log rate …
alvarezmelissa87 Jun 21, 2024
d59e594
Merge branch 'main' into ml-aiops-additional-columns
kibanamachine Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion x-pack/plugins/aiops/public/components/log_rate_analysis/log_rate_analysis_results.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[]>([
Copy link
Contributor

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.

Copy link
Contributor

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.

'p-value',
'Baseline rate',
Copy link
Contributor

@peteharverson peteharverson Jun 19, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

'Deviation rate',
]);

const onGroupResultsToggle = (optionId: string) => {
setToggleIdSelected(optionId);
Expand Down
58 changes: 58 additions & 0 deletions ...ops/public/components/log_rate_analysis_results_table/get_baseline_and_deviation_rates.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { i18n } from '@kbn/i18n';
import { LOG_RATE_ANALYSIS_TYPE } from '@kbn/aiops-log-rate-analysis';

export function getLogRateChange(
analysisType: typeof LOG_RATE_ANALYSIS_TYPE[keyof typeof LOG_RATE_ANALYSIS_TYPE],
baselineBucketRate: number,
deviationBucketRate: number
) {
const logRateChange =
baselineBucketRate > 0
? analysisType === LOG_RATE_ANALYSIS_TYPE.SPIKE
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in ff861eb

? `${Math.round((deviationBucketRate / baselineBucketRate) * 100) / 100}x higher`
: `${Math.round((baselineBucketRate / deviationBucketRate) * 100) / 100}x lower`
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

: analysisType === LOG_RATE_ANALYSIS_TYPE.SPIKE
? i18n.translate(
'xpack.aiops.logRateAnalysis.resultsTableGroups.logRateIncreaseLabelColumnTooltip',
{
defaultMessage: '{deviationBucketRate} docs from 0 in baseline',
values: { deviationBucketRate },
}
)
: i18n.translate(
'xpack.aiops.logRateAnalysis.resultsTableGroups.logRateDecreaseLabelColumnTooltip',
{
defaultMessage: '0 docs from ${baselineBucketRate} in baseline',
peteharverson marked this conversation as resolved.
Show resolved Hide resolved
values: { baselineBucketRate },
}
);

return logRateChange;
}

export function getBaselineAndDeviationRates(
baselineBuckets: number,
deviationBuckets: number,
docCount: number | undefined,
bgCount: number | undefined
) {
let baselineBucketRate;
let deviationBucketRate;

if (bgCount !== undefined) {
baselineBucketRate = Math.round(bgCount / baselineBuckets);
}

if (docCount !== undefined) {
deviationBucketRate = Math.round(docCount / deviationBuckets);
}

return { baselineBucketRate, deviationBucketRate };
}
8 changes: 8 additions & 0 deletions ...ops/public/components/log_rate_analysis_results_table/log_rate_analysis_results_table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ export const LogRateAnalysisResultsTable: FC<LogRateAnalysisResultsTableProps> =
const selectedSignificantItem = useAppSelector(
(s) => s.logRateAnalysisTableRow.selectedSignificantItem
);
const {
analysisType,
windowParameters,
documentStats: { documentCountStats },
} = useAppSelector((s) => s.logRateAnalysis);
Copy link
Contributor

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 dispatch = useAppDispatch();

Expand All @@ -97,6 +102,9 @@ export const LogRateAnalysisResultsTable: FC<LogRateAnalysisResultsTableProps> =

const columns = useColumns(
LOG_RATE_ANALYSIS_RESULTS_TABLE_TYPE.SIGNIFICANT_ITEMS,
analysisType,
windowParameters,
documentCountStats?.interval ?? 0,
skippedColumns,
searchQuery,
barColorOverride,
Expand Down
9 changes: 9 additions & 0 deletions ...lic/components/log_rate_analysis_results_table/log_rate_analysis_results_table_groups.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ export const LogRateAnalysisResultsGroupsTable: FC<LogRateAnalysisResultsTablePr

const pinnedGroup = useAppSelector((s) => s.logRateAnalysisTableRow.pinnedGroup);
const selectedGroup = useAppSelector((s) => s.logRateAnalysisTableRow.selectedGroup);
const {
analysisType,
windowParameters,
documentStats: { documentCountStats },
} = useAppSelector((s) => s.logRateAnalysis);
Copy link
Contributor

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.

Copy link
Contributor Author

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

const dispatch = useAppDispatch();
const isMounted = useMountedState();

Expand Down Expand Up @@ -149,6 +154,7 @@ export const LogRateAnalysisResultsGroupsTable: FC<LogRateAnalysisResultsTablePr
{
'data-test-subj': 'aiopsLogRateAnalysisResultsGroupsTableColumnGroup',
field: 'group',
width: skippedColumns.length < 3 ? '34%' : '50%',
name: (
<>
<FormattedMessage
Expand Down Expand Up @@ -238,6 +244,9 @@ export const LogRateAnalysisResultsGroupsTable: FC<LogRateAnalysisResultsTablePr

const columns = useColumns(
LOG_RATE_ANALYSIS_RESULTS_TABLE_TYPE.GROUPS,
analysisType,
windowParameters,
documentCountStats?.interval ?? 0,
skippedColumns,
searchQuery,
barColorOverride,
Expand Down
Loading