Skip to content

Commit

Permalink
[Lens] Various fixes for Heatmap (elastic#172602)
Browse files Browse the repository at this point in the history 
## Summary

Fixes elastic#172433
Fixes elastic#172574
Fixes elastic#170240

For elastic#172433 the bands values are now passing thru the value formatter to
match users' expectations:

<img width="1227" alt="Screenshot 2023-12-05 at 16 52 39"
src="https://github.com/elastic/kibana/assets/924948/42d2c4ba-1b6b-4785-84e7-0ad73670ecdc">

When the default formatter is selected something complex happens there,
which might look wrong but it is still respecting Kibana's advanced
settings formatter pattern (in this example `0.[000]`):

<img width="1234" alt="Screenshot 2023-12-05 at 16 52 57"
src="https://github.com/elastic/kibana/assets/924948/7fe7dd1d-eff1-40fa-9e52-8a7ff20d0faf">

As for elastic#170240 the problem was due to an unnecessary safe guard which
was forcing the first bucket to be `1` when it was open:

<img width="1227" alt="Screenshot 2023-12-05 at 16 52 11"
src="https://github.com/elastic/kibana/assets/924948/a3ac437d-7b04-489d-b0fc-6c2b456971de">

As for elastic#172574 I just fixed at root level the problem...

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
  • Loading branch information
@dej611
dej611 authored Dec 12, 2023
1 parent a0631cf commit 9f4c220
Show file tree
Hide file tree
Showing 9 changed files with 172 additions and 192 deletions.
4 changes: 2 additions & 2 deletions packages/shared-ux/button_toolbar/src/buttons/toolbar_button/toolbar_button.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type ButtonRenderStyle = 'standard' | 'iconButton';
interface ToolbarButtonCommonProps
extends Pick<
EuiButtonPropsForButton,
'onClick' | 'iconType' | 'size' | 'data-test-subj' | 'isDisabled'
'onClick' | 'iconType' | 'size' | 'data-test-subj' | 'isDisabled' | 'aria-label'
> {
/**
* Render style of the toolbar button
Expand Down Expand Up @@ -162,7 +162,7 @@ const ToolbarIconButton = ({
<EuiButtonIcon
{...rest}
disabled={isDisabled}
aria-label={label}
aria-label={label ?? rest['aria-label']}
size={size}
iconSize={size}
css={cssProps}
Expand Down
57 changes: 57 additions & 0 deletions ...plugins/chart_expressions/expression_heatmap/public/components/heatmap_component.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,63 @@ describe('HeatmapComponent', function () {
});
});

it('should keep the minimum open end', () => {
const newData: Datatable = {
type: 'datatable',
rows: [{ 'col-0-1': -3 }],
columns: [{ id: 'col-0-1', name: 'Count', meta: { type: 'number' } }],
};
const newProps = {
...wrapperProps,
data: newData,
args: {
...wrapperProps.args,
palette: {
params: {
colors: ['#6092c0', '#a8bfda', '#ebeff5', '#ecb385', '#e7664c'],
stops: [1, 2, 3, 4, 5],
range: 'number',
gradient: true,
continuity: 'above',
rangeMin: -Infinity,
rangeMax: null,
},
},
},
} as unknown as HeatmapRenderProps;
const component = mountWithIntl(<HeatmapComponent {...newProps} />);
expect(component.find(Heatmap).prop('colorScale')).toEqual({
bands: [
{
start: -Infinity,
end: 1,
color: '#6092c0',
},
{
start: 1,
end: 2,
color: '#a8bfda',
},
{
start: 2,
end: 3,
color: '#ebeff5',
},
{
start: 3,
end: 4,
color: '#ecb385',
},
{
start: 4,
end: Infinity,
color: '#e7664c',
},
],
type: 'bands',
});
});

it('renders the axis titles', () => {
const component = shallowWithIntl(<HeatmapComponent {...wrapperProps} />);
expect(component.find(Heatmap).prop('xAxisTitle')).toEqual('Dest');
Expand Down
12 changes: 3 additions & 9 deletions src/plugins/chart_expressions/expression_heatmap/public/components/heatmap_component.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,7 @@ function shiftAndNormalizeStops(
if (params.range === 'percent') {
result = min + ((max - min) * value) / 100;
}
// a division by zero safeguard
if (!Number.isFinite(result)) {
return 1;
}
return Number(result.toFixed(2));
return result;
}
);
}
Expand Down Expand Up @@ -515,11 +511,9 @@ export const HeatmapComponent: FC<HeatmapRenderProps> = memo(
let overwriteArrayIdx;

if (endValue === Number.POSITIVE_INFINITY) {
overwriteArrayIdx = `≥ ${metricFormatter.convert(startValue)}`;
overwriteArrayIdx = `≥ ${valueFormatter(startValue)}`;
} else {
overwriteArrayIdx = `${metricFormatter.convert(start)} - ${metricFormatter.convert(
endValue
)}`;
overwriteArrayIdx = `${valueFormatter(start)} - ${valueFormatter(endValue)}`;
}

const overwriteColor = overwriteColors?.[overwriteArrayIdx];
Expand Down
38 changes: 17 additions & 21 deletions x-pack/test/functional/apps/lens/group4/chart_data.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,19 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
{ name: '__other__', value: 5722.77 },
];

function assertMatchesExpectedData(state: DebugState) {
function assertMatchesExpectedData(state: DebugState | undefined) {
expect(
state.bars![0].bars.map((bar) => ({
state?.bars![0].bars.map((bar) => ({
x: bar.x,
y: Math.floor(bar.y * 100) / 100,
}))
).to.eql(expectedData);
}

function assertMatchesExpectedPieData(state: DebugState) {
function assertMatchesExpectedPieData(state: DebugState | undefined) {
expect(
state
.partition![0].partitions.map((partition) => ({
?.partition![0].partitions.map((partition) => ({
name: partition.name,
value: Math.floor(partition.value * 100) / 100,
}))
Expand All @@ -74,56 +74,52 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

it('should render xy chart', async () => {
const data = await PageObjects.lens.getCurrentChartDebugState('xyVisChart');
assertMatchesExpectedData(data!);
assertMatchesExpectedData(data);
});

it('should render pie chart', async () => {
await PageObjects.lens.switchToVisualization('pie');
const data = await PageObjects.lens.getCurrentChartDebugState('partitionVisChart');
assertMatchesExpectedPieData(data!);
assertMatchesExpectedPieData(data);
});

it('should render donut chart', async () => {
await PageObjects.lens.switchToVisualization('donut');
const data = await PageObjects.lens.getCurrentChartDebugState('partitionVisChart');
assertMatchesExpectedPieData(data!);
assertMatchesExpectedPieData(data);
});

it('should render treemap chart', async () => {
await PageObjects.lens.switchToVisualization('treemap', 'treemap');
const data = await PageObjects.lens.getCurrentChartDebugState('partitionVisChart');
assertMatchesExpectedPieData(data!);
assertMatchesExpectedPieData(data);
});

it('should render heatmap chart', async () => {
await PageObjects.lens.switchToVisualization('heatmap', 'heat');
const debugState = await PageObjects.lens.getCurrentChartDebugState('heatmapChart');

if (!debugState) {
throw new Error('Debug state is not available');
}

// assert axes
expect(debugState.axes!.x[0].labels).to.eql([
expect(debugState?.axes!.x[0].labels).to.eql([
'97.220.3.248',
'169.228.188.120',
'78.83.247.30',
'226.82.228.233',
'93.28.27.24',
'Other',
]);
expect(debugState.axes!.y[0].labels).to.eql(['']);
expect(debugState?.axes!.y[0].labels).to.eql(['']);

// assert cells
expect(debugState.heatmap!.cells.length).to.eql(6);
expect(debugState?.heatmap!.cells.length).to.eql(6);

// assert legend
expect(debugState.legend!.items).to.eql([
{ color: '#6092c0', key: '5,722.77 - 8,529.22', name: '5,722.77 - 8,529.22' },
{ color: '#a8bfda', key: '8,529.22 - 11,335.66', name: '8,529.22 - 11,335.66' },
{ color: '#ebeff5', key: '11,335.66 - 14,142.11', name: '11,335.66 - 14,142.11' },
{ color: '#ecb385', key: '14,142.11 - 16,948.55', name: '14,142.11 - 16,948.55' },
{ color: '#e7664c', key: '≥ 16,948.55', name: '≥ 16,948.55' },
expect(debugState?.legend!.items).to.eql([
{ key: '5,722.775 - 8,529.22', name: '5,722.775 - 8,529.22', color: '#6092c0' },
{ key: '8,529.22 - 11,335.665', name: '8,529.22 - 11,335.665', color: '#a8bfda' },
{ key: '11,335.665 - 14,142.11', name: '11,335.665 - 14,142.11', color: '#ebeff5' },
{ key: '14,142.11 - 16,948.555', name: '14,142.11 - 16,948.555', color: '#ecb385' },
{ key: '≥ 16,948.555', name: '≥ 16,948.555', color: '#e7664c' },
]);
});

Expand Down
34 changes: 17 additions & 17 deletions x-pack/test/functional/apps/lens/group4/tsdb.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,13 @@ function getDataMapping(
return dataStreamMapping;
}

function sumFirstNValues(n: number, bars: Array<{ y: number }>): number {
function sumFirstNValues(n: number, bars: Array<{ y: number }> | undefined): number {
const indexes = Array(n)
.fill(1)
.map((_, i) => i);
let countSum = 0;
for (const index of indexes) {
if (bars[index]) {
if (bars?.[index]) {
countSum += bars[index].y;
}
}
Expand Down Expand Up @@ -816,29 +816,29 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

await PageObjects.lens.waitForVisualization('xyVisChart');
const data = await PageObjects.lens.getCurrentChartDebugState('xyVisChart');
const counterBars = data.bars![0].bars;
const countBars = data.bars![1].bars;
const counterBars = data?.bars![0].bars;
const countBars = data?.bars![1].bars;

log.info('Check counter data before the upgrade');
// check there's some data before the upgrade
expect(counterBars[0].y).to.eql(5000);
expect(counterBars?.[0].y).to.eql(5000);
log.info('Check counter data after the upgrade');
// check there's some data after the upgrade
expect(counterBars[counterBars.length - 1].y).to.eql(5000);
expect(counterBars?.[counterBars.length - 1].y).to.eql(5000);

// due to the flaky nature of exact check here, we're going to relax it
// as long as there's data before and after it is ok
log.info('Check count before the upgrade');
const columnsToCheck = countBars.length / 2;
const columnsToCheck = countBars ? countBars.length / 2 : 0;
// Before the upgrade the count is N times the indexes
expect(sumFirstNValues(columnsToCheck, countBars)).to.be.greaterThan(
indexes.length * TEST_DOC_COUNT - 1
);
log.info('Check count after the upgrade');
// later there are only documents for the upgraded stream
expect(sumFirstNValues(columnsToCheck, [...countBars].reverse())).to.be.greaterThan(
TEST_DOC_COUNT - 1
);
expect(
sumFirstNValues(columnsToCheck, [...(countBars ?? [])].reverse())
).to.be.greaterThan(TEST_DOC_COUNT - 1);
});
});
});
Expand Down Expand Up @@ -911,8 +911,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

await PageObjects.lens.waitForVisualization('xyVisChart');
const data = await PageObjects.lens.getCurrentChartDebugState('xyVisChart');
const bars = data.bars![0].bars;
const columnsToCheck = bars.length / 2;
const bars = data?.bars![0].bars;
const columnsToCheck = bars ? bars.length / 2 : 0;
// due to the flaky nature of exact check here, we're going to relax it
// as long as there's data before and after it is ok
log.info('Check count before the downgrade');
Expand All @@ -922,7 +922,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
);
log.info('Check count after the downgrade');
// later there are only documents for the upgraded stream
expect(sumFirstNValues(columnsToCheck, [...bars].reverse())).to.be.greaterThan(
expect(sumFirstNValues(columnsToCheck, [...(bars ?? [])].reverse())).to.be.greaterThan(
TEST_DOC_COUNT - 1
);
});
Expand Down Expand Up @@ -952,8 +952,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

await PageObjects.lens.waitForVisualization('xyVisChart');
const dataBefore = await PageObjects.lens.getCurrentChartDebugState('xyVisChart');
const barsBefore = dataBefore.bars![0].bars;
expect(barsBefore.some(({ y }) => y)).to.eql(true);
const barsBefore = dataBefore?.bars![0].bars;
expect(barsBefore?.some(({ y }) => y)).to.eql(true);

// check after the downgrade
await PageObjects.lens.goToTimeRange(
Expand All @@ -969,8 +969,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

await PageObjects.lens.waitForVisualization('xyVisChart');
const dataAfter = await PageObjects.lens.getCurrentChartDebugState('xyVisChart');
const barsAfter = dataAfter.bars![0].bars;
expect(barsAfter.some(({ y }) => y)).to.eql(true);
const barsAfter = dataAfter?.bars![0].bars;
expect(barsAfter?.some(({ y }) => y)).to.eql(true);
});
});
});
Expand Down
Loading

0 comments on commit 9f4c220

Please sign in to comment.