Skip to content

Commit

Permalink
[Security Solution] Alerts Treemap fixes (#137796)
Browse files Browse the repository at this point in the history
## [Security Solution] Alerts Treemap fixes

This PR contains fixes for the following issues:

- #136377 was fixed by using the `key_as_string` value in the search strategy response to format timestamps for the Treemap and Table views, per the screenshots below:

**Treemap with timestamp formatting:**

![treemap_with_timestamp_formatting](https://user-images.githubusercontent.com/4459398/182256532-7df8d39b-4e09-440f-aae7-c6e3720e0906.png)

**Table with timestamp formatting:**

![table_with_timestamp_formatting](https://user-images.githubusercontent.com/4459398/182256817-ee8605a7-d9d3-4127-8753-aaad65832308.png)

- #136387 was fixed by reducing the mininum font size to `4px` for group names, per [this example](https://elastic.github.io/elastic-charts/?path=/story/treemap--two-layers-stress-test&globals=theme:light;background:white) from Elastic charts. This change:
  - Reduces the chances of a label getting hidden when the browser is resized, but cannot eliminate it, because at a certain point, Elastic Charts must hide the label in order to (still) render the group
  - Will result in some groups having very small labels where the group previously would not have them, per the screenshot below:

![4px_labels](https://user-images.githubusercontent.com/4459398/182255505-bc77c184-df5a-4ff6-830a-c7c5b6127d25.png)

- #136372 was fixed by adding the word `Trend`, `Table`, and `Treemap` to the inspect modal for each of the respective chart types, per the screenshots below:

**Inspect Trend:**

![inspect_trend](https://user-images.githubusercontent.com/4459398/182257336-5d988e6a-a436-48b6-ad59-72b64e86d89b.png)

**Inspect Table:**
![inspect_table](https://user-images.githubusercontent.com/4459398/182257396-6e0d01e3-e493-4765-ac60-fee790a52633.png)

**Inspect Treemap:**
![inspect_treemap](https://user-images.githubusercontent.com/4459398/182257457-d3b58f18-e393-4307-b319-a2e1019b0329.png)

- [review feedback](#126896 (review)) where the `Tree map isn't displayed for certain group by top fields, like Ransomeware.version`, was fixed as suggested by displaying an empty state message with a reason why the treemap cannot be displayed:

**Before:**
![empty-state-before](https://user-images.githubusercontent.com/2946766/178567785-82d03c51-3e3c-49f6-ac86-d13ecaf3aef0.png)

**After:**
![empty-state-after](https://user-images.githubusercontent.com/4459398/182254376-437d3ce8-c4bd-4b43-a456-2a4460db4565.png)

- [review feedback](#126896 (review)) where there were `Extra left margins when the Table view is selected` was fixed by removing the extra margin:

**Before:**
![left-margin-before](https://user-images.githubusercontent.com/2946766/178568473-c8162631-4340-4858-8d93-3466461f97d0.png)

**After:**
![left-margin-after](https://user-images.githubusercontent.com/4459398/182253474-1684094f-4f8d-43fe-860b-addb599ed4f1.png)

(cherry picked from commit a5a7d74)
  • Loading branch information
andrew-goldstein committed Aug 2, 2022
1 parent 51eb588 commit e2abee6
Show file tree
Hide file tree
Showing 19 changed files with 425 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export interface Hits<T, U> {

export interface GenericBuckets {
key: string;
key_as_string?: string; // contains, for example, formatted dates
doc_count: number;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@
*/

import { render, screen } from '@testing-library/react';
import { Settings } from '@elastic/charts';
import React from 'react';

import { TestProviders } from '../../mock';
import {
mockAlertSearchResponse,
mockNoDataAlertSearchResponse,
mockNoStackByField1Response,
mockOnlyStackByField0Response,
} from './lib/mocks/mock_alert_search_response';
import * as i18n from './translations';
import type { Props } from '.';
Expand All @@ -25,9 +28,19 @@ const defaultProps: Props = {
stackByField1: 'host.name',
};

jest.mock('@elastic/charts', () => {
const actual = jest.requireActual('@elastic/charts');
return {
...actual,
Settings: jest.fn().mockReturnValue(null),
};
});

describe('AlertsTreemap', () => {
describe('when the response has data', () => {
beforeEach(() => {
jest.clearAllMocks();

render(
<TestProviders>
<AlertsTreemap {...defaultProps} />
Expand All @@ -42,6 +55,10 @@ describe('AlertsTreemap', () => {
test('it renders the legend with the expected overflow-y style', () => {
expect(screen.getByTestId('draggable-legend')).toHaveClass('eui-yScroll');
});

test('it uses a theme with the expected `minFontSize` to show more labels at various screen resolutions', () => {
expect((Settings as jest.Mock).mock.calls[0][0].theme[0].partition.minFontSize).toEqual(4);
});
});

describe('when the response does NOT have data', () => {
Expand All @@ -65,4 +82,64 @@ describe('AlertsTreemap', () => {
expect(screen.getByText(i18n.NO_DATA_LABEL)).toBeInTheDocument();
});
});

describe('when the user has specified a `stackByField1`, and the response has data for `stackByField0`, but `stackByField1` is not present in the response', () => {
const stackByField1 = 'Ransomware.version'; // this non-ECS field is requested

beforeEach(() => {
render(
<TestProviders>
<AlertsTreemap
{...defaultProps}
stackByField1={stackByField1}
data={mockNoStackByField1Response} // the response has values for stackByField0, but does NOT contain values for Ransomware.version
/>
</TestProviders>
);
});

test('it renders the "no data" message', () => {
expect(screen.getByText(i18n.NO_DATA_LABEL)).toBeInTheDocument();
});

test('it renders an additional reason label with the `stackByField1` field', () => {
expect(screen.getByText(i18n.NO_DATA_REASON_LABEL(stackByField1))).toBeInTheDocument();
});

test('it still renders the legend, because we have values for stackByField0', () => {
expect(screen.getByTestId('draggable-legend')).toBeInTheDocument();
});
});

describe('when the user has NOT specified a `stackByField1`, and the response has data for `stackByField0`', () => {
const stackByField1 = ''; // the user has NOT specified a `stackByField1`

beforeEach(() => {
render(
<TestProviders>
<AlertsTreemap
{...defaultProps}
stackByField1={stackByField1}
data={mockOnlyStackByField0Response} // the response has values for stackByField0, but stackByField1 was NOT requested
/>
</TestProviders>
);
});

test('it renders the treemap', () => {
expect(screen.getByTestId('treemap').querySelector('.echChart')).toBeInTheDocument();
});

test('it does NOT render the "no data" message', () => {
expect(screen.queryByText(i18n.NO_DATA_LABEL)).not.toBeInTheDocument();
});

test('it does NOT render an additional reason label with the `stackByField1` field, which was not requested', () => {
expect(screen.queryByText(i18n.NO_DATA_REASON_LABEL(stackByField1))).not.toBeInTheDocument();
});

test('it renders the legend, because we have values for stackByField0', () => {
expect(screen.getByTestId('draggable-legend')).toBeInTheDocument();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
import { getLayersMultiDimensional, getLayersOneDimension } from './lib/layers';
import { getFirstGroupLegendItems } from './lib/legend';
import { NoData } from './no_data';
import { NO_DATA_REASON_LABEL } from './translations';
import type { AlertsTreeMapAggregation, FlattenedBucket, RawBucket } from './types';

export const DEFAULT_MIN_CHART_HEIGHT = 370; // px
Expand Down Expand Up @@ -68,7 +69,7 @@ const AlertsTreemapComponent: React.FC<Props> = ({
fillLabel: { valueFont: { fontWeight: 700 } },
idealFontSizeJump: 1.15,
maxFontSize: 16,
minFontSize: 8,
minFontSize: 4,
sectorLineStroke: fillColor, // draws the light or dark "lines" between partitions
sectorLineWidth: 1.5,
},
Expand Down Expand Up @@ -167,21 +168,25 @@ const AlertsTreemapComponent: React.FC<Props> = ({
<div data-test-subj="treemap">
<EuiFlexGroup gutterSize="none">
<ChartFlexItem grow={true} $minChartHeight={minChartHeight}>
<Chart>
<Settings
baseTheme={theme}
showLegend={false}
theme={treemapTheme}
onElementClick={onElementClick}
/>
<Partition
data={normalizedData}
id="spec_1"
layers={layers}
layout={PartitionLayout.treemap}
valueAccessor={valueAccessor}
/>
</Chart>
{stackByField1 != null && !isEmpty(stackByField1) && normalizedData.length === 0 ? (
<NoData reason={NO_DATA_REASON_LABEL(stackByField1)} />
) : (
<Chart>
<Settings
baseTheme={theme}
showLegend={false}
theme={treemapTheme}
onElementClick={onElementClick}
/>
<Partition
data={normalizedData}
id="spec_1"
layers={layers}
layout={PartitionLayout.treemap}
valueAccessor={valueAccessor}
/>
</Chart>
)}
</ChartFlexItem>

<EuiFlexItem grow={false}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
bucketsWithoutStackByField1,
maxRiskSubAggregations,
} from './mocks/mock_buckets';
import type { RawBucket } from '../../types';

describe('flattenBucket', () => {
it(`returns the expected flattened buckets when stackByField1 has buckets`, () => {
Expand Down Expand Up @@ -46,6 +47,63 @@ describe('flattenBucket', () => {
]);
});

it(`it prefers to populate 'key' using the RawBucket's 'key_as_string' when available, because it contains formatted dates`, () => {
const bucketWithOptionalKeyAsString: RawBucket = {
key: '1658955590866',
key_as_string: '2022-07-27T20:59:50.866Z', // <-- should be preferred over `key` when present
doc_count: 1,
maxRiskSubAggregation: { value: 21 },
stackByField1: {
doc_count_error_upper_bound: 0,
sum_other_doc_count: 0,
buckets: [{ key: 'Host-vmdx1cnu3m', doc_count: 1 }],
},
};

expect(
flattenBucket({ bucket: bucketWithOptionalKeyAsString, maxRiskSubAggregations })
).toEqual([
{
doc_count: 1,
key: bucketWithOptionalKeyAsString.key_as_string, // <-- uses the preferred `key_as_string`
maxRiskSubAggregation: { value: 21 },
stackByField1DocCount: 1,
stackByField1Key: 'Host-vmdx1cnu3m',
},
]);
});

it(`it prefers to populate 'stackByField1Key' using the 'stackByField1.buckets[n].key_as_string' when available, because it contains formatted dates`, () => {
const keyAsString = '2022-07-27T09:33:19.329Z';

const bucketWithKeyAsString: RawBucket = {
key: 'Threshold rule',
doc_count: 1,
maxRiskSubAggregation: { value: 99 },
stackByField1: {
doc_count_error_upper_bound: 0,
sum_other_doc_count: 0,
buckets: [
{
key: '1658914399329',
key_as_string: keyAsString, // <-- should be preferred over `stackByField1.buckets[n].key` when present
doc_count: 1,
},
],
},
};

expect(flattenBucket({ bucket: bucketWithKeyAsString, maxRiskSubAggregations })).toEqual([
{
doc_count: 1,
key: 'Threshold rule',
maxRiskSubAggregation: { value: 99 },
stackByField1DocCount: 1,
stackByField1Key: keyAsString, // <-- uses the preferred `stackByField1.buckets[n].key_as_string`
},
]);
});

it(`returns an empty array when there's nothing to flatten, because stackByField1 is undefined`, () => {
expect(
flattenBucket({ bucket: bucketsWithoutStackByField1[0], maxRiskSubAggregations })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ export const flattenBucket = ({
}): FlattenedBucket[] =>
bucket.stackByField1?.buckets?.map<FlattenedBucket>((x) => ({
doc_count: bucket.doc_count,
key: bucket.key,
key: bucket.key_as_string ?? bucket.key, // prefer key_as_string when available, because it contains a formatted date
maxRiskSubAggregation: bucket.maxRiskSubAggregation,
stackByField1Key: x.key,
stackByField1Key: x.key_as_string ?? x.key,
stackByField1DocCount: x.doc_count,
})) ?? [];
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,24 @@ import {
getLegendItemFromFlattenedBucket,
getLegendMap,
} from '.';
import type { FlattenedBucket } from '../../types';
import type { FlattenedBucket, RawBucket } from '../../types';

describe('legend', () => {
const colorPalette = getRiskScorePalette(RISK_SCORE_STEPS);

describe('getLegendItemFromRawBucket', () => {
const bucket: RawBucket = {
key: '1658955590866',
key_as_string: '2022-07-27T20:59:50.866Z', // <-- should be preferred over `key` when present
doc_count: 1,
maxRiskSubAggregation: { value: 21 },
stackByField1: {
doc_count_error_upper_bound: 0,
sum_other_doc_count: 0,
buckets: [{ key: 'Host-vmdx1cnu3m', doc_count: 1 }],
},
};

it('returns an undefined color when showColor is false', () => {
expect(
getLegendItemFromRawBucket({
Expand Down Expand Up @@ -70,6 +82,30 @@ describe('legend', () => {
).toContain('draggable-legend-item-treemap-kibana_alert_rule_name-matches everything-');
});

it('renders the expected label', () => {
const item = getLegendItemFromRawBucket({
bucket: bucketsWithStackByField1[0],
colorPalette,
maxRiskSubAggregations,
showColor: true,
stackByField0: 'kibana.alert.rule.name',
});

expect(item.render != null && item.render()).toEqual(`matches everything (Risk 21)`);
});

it('prefers `key_as_string` over `key` when rendering the label', () => {
const item = getLegendItemFromRawBucket({
bucket,
colorPalette,
maxRiskSubAggregations,
showColor: true,
stackByField0: '@timestamp',
});

expect(item.render != null && item.render()).toEqual(`${bucket.key_as_string} (Risk 21)`);
});

it('returns the expected field', () => {
expect(
getLegendItemFromRawBucket({
Expand All @@ -93,6 +129,18 @@ describe('legend', () => {
}).value
).toEqual('matches everything');
});

it('prefers `key_as_string` over `key` when populating value', () => {
expect(
getLegendItemFromRawBucket({
bucket,
colorPalette,
maxRiskSubAggregations,
showColor: true,
stackByField0: '@timestamp',
}).value
).toEqual(bucket.key_as_string);
});
});

describe('getLegendItemFromFlattenedBucket', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@ export const getLegendItemFromRawBucket = ({
),
render: () =>
getLabel({
baseLabel: bucket.key,
baseLabel: bucket.key_as_string ?? bucket.key, // prefer key_as_string when available, because it contains a formatted date
riskScore: bucket.maxRiskSubAggregation?.value,
}),
field: stackByField0,
value: bucket.key,
value: bucket.key_as_string ?? bucket.key,
});

export const getLegendItemFromFlattenedBucket = ({
Expand Down
Loading

0 comments on commit e2abee6

Please sign in to comment.