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

[ResponseOps][BUG] - UI fixes for Alerts Summary chart #137476

Merged
merged 17 commits into from
Aug 2, 2022
Merged

[ResponseOps][BUG] - UI fixes for Alerts Summary chart #137476

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
94ebd0c
Use more efficient code/query, and add the 3rd bar (base one)
fkanout Jul 28, 2022
0c170d2
Update comment
fkanout Jul 28, 2022
5e2c8fd
Update comment
fkanout Jul 28, 2022
08c7e23
Add the shade color to the base bar
fkanout Jul 28, 2022
2fd2fc9
Upddate comment
fkanout Jul 28, 2022
cbef6f9
Remove unused else
fkanout Jul 28, 2022
2720e2e
add tootlip and fix base bar
fkanout Jul 29, 2022
5c944e0
Update design
fkanout Jul 29, 2022
10068aa
Add error handling message
fkanout Jul 29, 2022
1a27103
Merge branch 'main' into 137292-ui-bug-fix--alert-summary-chart
kibanamachine Jul 29, 2022
2c68d83
Merge branch 'main' into 137292-ui-bug-fix--alert-summary-chart
kibanamachine Aug 1, 2022
a2b98d4
review idea
XavierM Jul 29, 2022
1c96337
fix no alerts + graph to get greyed out
XavierM Aug 1, 2022
1fca5af
review I
XavierM Aug 1, 2022
a7f4ca0
Merge pull request #3 from XavierM/pr/137476
fkanout Aug 2, 2022
88ab8b5
Fix the order of alertsChartData
fkanout Aug 2, 2022
2771ab6
Fix design and color scheme
fkanout Aug 2, 2022
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
108 changes: 81 additions & 27 deletions ...plugins/triggers_actions_ui/public/application/hooks/use_load_rule_alerts_aggregations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,6 @@ interface RuleAlertsAggs {
error?: string;
alertsChartData: AlertChartData[];
}
interface BucketAggsPerDay {
key_as_string: string;
doc_count: number;
}

export async function fetchRuleAlertsAggByTimeRange({
http,
Expand Down Expand Up @@ -146,51 +142,109 @@ export async function fetchRuleAlertsAggByTimeRange({
{
range: {
'@timestamp': {
// When needed, we can make this range configurable via a function argument.
gte: 'now-30d',
lt: 'now',
},
},
},
{
bool: {
should: [
{
term: {
'kibana.alert.status': 'active',
},
},
{
term: {
'kibana.alert.status': 'recovered',
},
},
],
},
},
],
},
},
aggs: {
filterAggs: {
total: {
filters: {
filters: {
alert_active: { term: { 'kibana.alert.status': 'active' } },
alert_recovered: { term: { 'kibana.alert.status': 'recovered' } },
totalActiveAlerts: {
term: {
'kibana.alert.status': 'active',
},
},
totalRecoveredAlerts: {
term: {
'kibana.alert.status': 'recovered',
},
},
},
},
},
statusPerDay: {
date_histogram: {
field: '@timestamp',
fixed_interval: '1d',
extended_bounds: {
min: 'now-30d',
max: 'now',
},
},
aggs: {
status_per_day: {
date_histogram: {
field: '@timestamp',
fixed_interval: '1d',
alertStatus: {
terms: {
field: 'kibana.alert.status',
},
},
},
},
},
}),
});
const active = res?.aggregations?.filterAggs.buckets.alert_active?.doc_count ?? 0;
const recovered = res?.aggregations?.filterAggs.buckets.alert_recovered?.doc_count ?? 0;

const active = res?.aggregations?.total.buckets.totalActiveAlerts?.doc_count ?? 0;
const recovered = res?.aggregations?.total.buckets.totalRecoveredAlerts?.doc_count ?? 0;
// Total alerts count in the last 30 days per status
const totalAlerts = active + recovered;

const alertsChartData = [
XavierM marked this conversation as resolved.
Show resolved Hide resolved
...res?.aggregations?.filterAggs.buckets.alert_active.status_per_day.buckets.map(
(bucket: BucketAggsPerDay) => ({
date: bucket.key_as_string,
status: 'active',
count: bucket.doc_count,
})
),
...res?.aggregations?.filterAggs.buckets.alert_recovered.status_per_day.buckets.map(
(bucket: BucketAggsPerDay) => ({
date: bucket.key_as_string,
status: 'recovered',
count: bucket.doc_count,
})
...res?.aggregations?.statusPerDay.buckets.reduce(
(
acc: AlertChartData[],
dayAlerts: {
key_as_string: string;
doc_count: number;
alertStatus: {
buckets: Array<{
key: string;
doc_count: number;
}>;
};
}
) => {
// We are adding this to each day to construct the 30 days bars (background bar) when there is no data for a given day or to show the delta today alerts/total alerts.
const totalDayAlerts = {
date: dayAlerts.key_as_string,
count: totalAlerts,
status: 'total',
};

if (dayAlerts.doc_count > 0) {
// If there are alerts in this day, we construct the chart data.
return [
...acc,
...dayAlerts.alertStatus.buckets.map((alert) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Tell me if I am wrong here, but I feel that you should also add the totalDayAlert too so you can avoid the white background when you have data.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XavierM, excellent point. Actually, totalDayAlert was added to each day and calculated based on the number of alerts for that day.
But then once the 2nd of alerts kicked in. The chart flipped upside down means the totalDayAlert (grey color) becomes at the bottom and the recovered and active at the top. I tried to fix that in many ways this morning but with no success and I don't want to spend more time on it as we all agreed.

BTW, when we hover over the bar the gray bar appears again.

I think this limitation is ok for this version, what do you think @maciejforcone?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can do it just like that -> https://github.com/fkanout/kibana/pull/3/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @XavierM. But it has exactly the same issue I mentioned -the chart is flipped. You need at least two days of data to see the issue.

Screenshot 2022-07-29 at 17 11 28

Here is my commit where I deleted my solution after I saw the issue

Copy link
Contributor

@XavierM XavierM Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I failed.... 😞 but I was also looking everywhere in kibana, and it looks like that nobody is doing the fake background like we do, maybe for good reason. I am wondering if we should remove it because if we are having alerts/data, we most likely not going to see this grey background anyway. It is better to be simplistic sometime.

what do you think?

Copy link
Contributor Author

@fkanout fkanout Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@XavierM, no worries! BTW, the only way I found to make it works is by making chart reading starting from the left, which means the today bar will be at the very left bar.

To answer your question. I don't mind removing the background/reference bars.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, after talking to @angorayc, we found a cool solution about it and now she might have to fix some other graph in security solution LOL.

The PR is re-open @fkanout

date: dayAlerts.key_as_string,
count: alert.doc_count,
status: alert.key,
})),
];
}
return [...acc, { ...totalDayAlerts }];
},
[]
),
];

Expand Down
2 changes: 2 additions & 0 deletions ..._actions_ui/public/application/sections/rule_details/components/alert_summary/helpers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ export const getColorSeries = ({ seriesKeys }: XYChartSeriesIdentifier) => {
return LIGHT_THEME.colors.vizColors[1];
case 'recovered':
return LIGHT_THEME.colors.vizColors[2];
case 'total':
return '#f5f7fa';
default:
return null;
}
Expand Down
58 changes: 49 additions & 9 deletions ...public/application/sections/rule_details/components/alert_summary/rule_alerts_summary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,18 @@
* 2.0.
*/

import { BarSeries, Chart, ScaleType, Settings, TooltipType } from '@elastic/charts';
import {
BarSeries,
Chart,
FilterPredicate,
ScaleType,
Settings,
TooltipType,
} from '@elastic/charts';
import {
EuiEmptyPrompt,
EuiFlexGroup,
EuiFlexItem,
EuiHorizontalRule,
EuiLoadingSpinner,
EuiPanel,
EuiSpacer,
Expand Down Expand Up @@ -66,8 +73,33 @@ export const RuleAlertsSummary = ({ rule, filteredRuleTypes }: RuleAlertsSummary
}, [rule, ruleTypes]);

if (isLoadingRuleAlertsAggs) return <EuiLoadingSpinner />;
if (errorRuleAlertsAggs) return <EuiFlexItem>Error</EuiFlexItem>;

if (errorRuleAlertsAggs)
return (
<EuiEmptyPrompt
iconType="alert"
color="danger"
title={
<h5>
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.alertsSummary.errorLoadingTitle"
defaultMessage="Unable to load the alerts summary"
/>
</h5>
}
body={
<p>
{
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.alertsSummary.errorLoadingBody"
defaultMessage=" There was an error loading the alerts summary. Contact your
administrator for help."
/>
}
</p>
}
/>
);
const isVisibleFunction: FilterPredicate = (series) => series.splitAccessors.get('g') !== 'total';
return (
<EuiPanel hasShadow={false} hasBorder>
<EuiFlexGroup direction="column">
Expand All @@ -82,12 +114,20 @@ export const RuleAlertsSummary = ({ rule, filteredRuleTypes }: RuleAlertsSummary
/>
</h5>
</EuiTitle>
<EuiSpacer size="s" />
XavierM marked this conversation as resolved.
Show resolved Hide resolved
<EuiText size="s" color="subdued">
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.alertsSummary.last30days"
defaultMessage="Last 30 days"
/>
</EuiText>
</EuiFlexItem>

<EuiPanel hasShadow={false}>
<EuiFlexGroup gutterSize="s" alignItems="center">
<EuiFlexGroup direction="row">
<EuiFlexItem>
<EuiText size="s" color="subdued">
<EuiText size="xs" color="subdued">
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.alertsSummary.allAlertsLabel"
defaultMessage="All alerts"
Expand All @@ -100,7 +140,7 @@ export const RuleAlertsSummary = ({ rule, filteredRuleTypes }: RuleAlertsSummary
</EuiFlexGroup>
<EuiFlexGroup direction="row">
<EuiFlexItem>
<EuiText size="s" color="subdued">
<EuiText size="xs" color="subdued">
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.alertsSummary.activeLabel"
defaultMessage="Active"
Expand All @@ -113,7 +153,7 @@ export const RuleAlertsSummary = ({ rule, filteredRuleTypes }: RuleAlertsSummary
</EuiFlexGroup>
<EuiFlexGroup direction="row">
<EuiFlexItem>
<EuiText size="s" color="subdued">
<EuiText size="xs" color="subdued">
<FormattedMessage
id="xpack.triggersActionsUI.sections.ruleDetails.rule.ruleSummary.recoveredLabel"
defaultMessage="Recovered"
Expand All @@ -128,7 +168,6 @@ export const RuleAlertsSummary = ({ rule, filteredRuleTypes }: RuleAlertsSummary
</EuiFlexGroup>
</EuiFlexGroup>
</EuiPanel>
<EuiHorizontalRule margin="none" />
</EuiFlexGroup>
</EuiFlexItem>

Expand All @@ -145,7 +184,7 @@ export const RuleAlertsSummary = ({ rule, filteredRuleTypes }: RuleAlertsSummary
</EuiFlexGroup>
<EuiSpacer size="m" />
<Chart size={{ height: 50 }}>
<Settings tooltip={TooltipType.None} theme={theme} />
<Settings tooltip={TooltipType.VerticalCursor} theme={theme} />
<BarSeries
id="bars"
xScaleType={ScaleType.Time}
Expand All @@ -156,6 +195,7 @@ export const RuleAlertsSummary = ({ rule, filteredRuleTypes }: RuleAlertsSummary
splitSeriesAccessors={G_ACCESSORS}
color={getColorSeries}
data={chartData}
filterSeriesInTooltip={isVisibleFunction}
/>
</Chart>
</EuiPanel>
Expand Down