From 5c68ca62afda29fe1da326cbe10429230beb3a95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Wed, 17 Jun 2020 22:50:14 +0200 Subject: [PATCH] [APM] Only add decimals for numbers below 10 (#69334) (#69374) --- .../apm/e2e/cypress/integration/snapshots.js | 6 +- .../ServiceList/__test__/List.test.js | 2 +- .../__snapshots__/CustomPlot.test.js.snap | 6 +- .../Histogram/__test__/Histogram.test.js | 10 +- .../__snapshots__/Histogram.test.js.snap | 24 ++-- .../Timeline/Marker/ErrorMarker.test.tsx | 2 +- .../formatters/__test__/duration.test.ts | 12 +- .../apm/public/utils/formatters/duration.ts | 119 +++++++++++------- 8 files changed, 102 insertions(+), 79 deletions(-) diff --git a/x-pack/plugins/apm/e2e/cypress/integration/snapshots.js b/x-pack/plugins/apm/e2e/cypress/integration/snapshots.js index 2fae8bdab2b30..d4c8ba4910850 100644 --- a/x-pack/plugins/apm/e2e/cypress/integration/snapshots.js +++ b/x-pack/plugins/apm/e2e/cypress/integration/snapshots.js @@ -1,9 +1,9 @@ module.exports = { APM: { 'Transaction duration charts': { - '1': '350.0 ms', - '2': '175.0 ms', - '3': '0.0 ms', + '1': '350 ms', + '2': '175 ms', + '3': '0 ms', }, }, __version: '4.5.0', diff --git a/x-pack/plugins/apm/public/components/app/ServiceOverview/ServiceList/__test__/List.test.js b/x-pack/plugins/apm/public/components/app/ServiceOverview/ServiceList/__test__/List.test.js index 09fef5da16ae7..927779b571fd8 100644 --- a/x-pack/plugins/apm/public/components/app/ServiceOverview/ServiceList/__test__/List.test.js +++ b/x-pack/plugins/apm/public/components/app/ServiceOverview/ServiceList/__test__/List.test.js @@ -40,7 +40,7 @@ describe('ServiceOverview -> List', () => { expect(renderedColumns[0]).toMatchSnapshot(); expect(renderedColumns.slice(2)).toEqual([ 'python', - '91.5 ms', + '92 ms', '86.9 tpm', '12.6 err.', ]); diff --git a/x-pack/plugins/apm/public/components/shared/charts/CustomPlot/test/__snapshots__/CustomPlot.test.js.snap b/x-pack/plugins/apm/public/components/shared/charts/CustomPlot/test/__snapshots__/CustomPlot.test.js.snap index ba3174156d5d4..6c5a200fb6f27 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/CustomPlot/test/__snapshots__/CustomPlot.test.js.snap +++ b/x-pack/plugins/apm/public/components/shared/charts/CustomPlot/test/__snapshots__/CustomPlot.test.js.snap @@ -9,7 +9,7 @@ Array [ "text": Avg. - 467.6 ms + 468 ms , }, @@ -2744,7 +2744,7 @@ Array [ - 467.6 ms + 468 ms @@ -5923,7 +5923,7 @@ Array [ - 467.6 ms + 468 ms diff --git a/x-pack/plugins/apm/public/components/shared/charts/Histogram/__test__/Histogram.test.js b/x-pack/plugins/apm/public/components/shared/charts/Histogram/__test__/Histogram.test.js index f84b0cfeda369..7eaeecca31b36 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/Histogram/__test__/Histogram.test.js +++ b/x-pack/plugins/apm/public/components/shared/charts/Histogram/__test__/Histogram.test.js @@ -11,8 +11,8 @@ import d3 from 'd3'; import { HistogramInner } from '../index'; import response from './response.json'; import { - asDecimal, getDurationFormatter, + asInteger, } from '../../../../../utils/formatters'; import { toJson } from '../../../../../utils/testHelpers'; import { getFormattedBuckets } from '../../../../app/TransactionDetails/Distribution/index'; @@ -33,8 +33,8 @@ describe('Histogram', () => { transactionId="myTransactionId" onClick={onClick} formatX={(time) => timeFormatter(time).formatted} - formatYShort={(t) => `${asDecimal(t)} occ.`} - formatYLong={(t) => `${asDecimal(t)} occurrences`} + formatYShort={(t) => `${asInteger(t)} occ.`} + formatYLong={(t) => `${asInteger(t)} occurrences`} tooltipHeader={(bucket) => { const xFormatted = timeFormatter(bucket.x); const x0Formatted = timeFormatter(bucket.x0); @@ -78,9 +78,9 @@ describe('Histogram', () => { const tooltips = wrapper.find('Tooltip'); expect(tooltips.length).toBe(1); - expect(tooltips.prop('header')).toBe('811.1 - 926.9 ms'); + expect(tooltips.prop('header')).toBe('811 - 927 ms'); expect(tooltips.prop('tooltipPoints')).toEqual([ - { value: '49.0 occurrences' }, + { value: '49 occurrences' }, ]); expect(tooltips.prop('x')).toEqual(869010); expect(tooltips.prop('y')).toEqual(27.5); diff --git a/x-pack/plugins/apm/public/components/shared/charts/Histogram/__test__/__snapshots__/Histogram.test.js.snap b/x-pack/plugins/apm/public/components/shared/charts/Histogram/__test__/__snapshots__/Histogram.test.js.snap index ff38ae4b563dd..1f935af7c8999 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/Histogram/__test__/__snapshots__/Histogram.test.js.snap +++ b/x-pack/plugins/apm/public/components/shared/charts/Histogram/__test__/__snapshots__/Histogram.test.js.snap @@ -127,7 +127,7 @@ exports[`Histogram Initially should have default markup 1`] = ` textAnchor="middle" transform="translate(0, 18)" > - 0.0 ms + 0 ms - 500.0 ms + 500 ms - 1,000.0 ms + 1,000 ms - 1,500.0 ms + 1,500 ms - 2,000.0 ms + 2,000 ms - 2,500.0 ms + 2,500 ms - 3,000.0 ms + 3,000 ms @@ -371,7 +371,7 @@ exports[`Histogram Initially should have default markup 1`] = ` textAnchor="end" transform="translate(-8, 0)" > - 0.0 occ. + 0 occ. - 27.5 occ. + 28 occ. - 55.0 occ. + 55 occ. @@ -1477,7 +1477,7 @@ exports[`Histogram when hovering over a non-empty bucket should have correct mar
- 811.1 - 926.9 ms + 811 - 927 ms
@@ -1488,7 +1488,7 @@ exports[`Histogram when hovering over a non-empty bucket should have correct mar
- 49.0 occurrences + 49 occurrences
diff --git a/x-pack/plugins/apm/public/components/shared/charts/Timeline/Marker/ErrorMarker.test.tsx b/x-pack/plugins/apm/public/components/shared/charts/Timeline/Marker/ErrorMarker.test.tsx index 6a28155813766..2a86f6ddbfc1a 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/Timeline/Marker/ErrorMarker.test.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/Timeline/Marker/ErrorMarker.test.tsx @@ -31,7 +31,7 @@ describe('ErrorMarker', () => { act(() => { fireEvent.click(component.getByTestId('popover')); }); - expectTextsInDocument(component, ['10.0 ms']); + expectTextsInDocument(component, ['10 ms']); return component; } function getKueryDecoded(url: string) { diff --git a/x-pack/plugins/apm/public/utils/formatters/__test__/duration.test.ts b/x-pack/plugins/apm/public/utils/formatters/__test__/duration.test.ts index 6d4b65d2aa9b4..c4d59beb4b7a2 100644 --- a/x-pack/plugins/apm/public/utils/formatters/__test__/duration.test.ts +++ b/x-pack/plugins/apm/public/utils/formatters/__test__/duration.test.ts @@ -14,14 +14,14 @@ describe('duration formatters', () => { expect(asDuration(1)).toEqual('1 μs'); expect(asDuration(toMicroseconds(1, 'milliseconds'))).toEqual('1,000 μs'); expect(asDuration(toMicroseconds(1000, 'milliseconds'))).toEqual( - '1,000.0 ms' + '1,000 ms' ); expect(asDuration(toMicroseconds(10000, 'milliseconds'))).toEqual( - '10,000.0 ms' + '10,000 ms' ); - expect(asDuration(toMicroseconds(20, 'seconds'))).toEqual('20.0 s'); - expect(asDuration(toMicroseconds(10, 'minutes'))).toEqual('10.0 min'); - expect(asDuration(toMicroseconds(1, 'hours'))).toEqual('60.0 min'); + expect(asDuration(toMicroseconds(20, 'seconds'))).toEqual('20 s'); + expect(asDuration(toMicroseconds(10, 'minutes'))).toEqual('10 min'); + expect(asDuration(toMicroseconds(1, 'hours'))).toEqual('60 min'); expect(asDuration(toMicroseconds(1.5, 'hours'))).toEqual('1.5 h'); }); @@ -41,7 +41,7 @@ describe('duration formatters', () => { describe('asMilliseconds', () => { it('converts to formatted decimal milliseconds', () => { - expect(asMillisecondDuration(0)).toEqual('0.0 ms'); + expect(asMillisecondDuration(0)).toEqual('0 ms'); }); }); }); diff --git a/x-pack/plugins/apm/public/utils/formatters/duration.ts b/x-pack/plugins/apm/public/utils/formatters/duration.ts index a603faab37538..64a9e3b952b98 100644 --- a/x-pack/plugins/apm/public/utils/formatters/duration.ts +++ b/x-pack/plugins/apm/public/utils/formatters/duration.ts @@ -18,13 +18,6 @@ interface FormatterOptions { type DurationTimeUnit = TimeUnit | 'microseconds'; -interface DurationUnit { - [unit: string]: { - label: string; - convert: (value: number) => string; - }; -} - interface ConvertedDuration { value: string; unit?: string; @@ -38,42 +31,69 @@ export type TimeFormatter = ( type TimeFormatterBuilder = (max: number) => TimeFormatter; -const durationUnit: DurationUnit = { - hours: { - label: i18n.translate('xpack.apm.formatters.hoursTimeUnitLabel', { - defaultMessage: 'h', - }), - convert: (value: number) => - asDecimal(moment.duration(value / 1000).asHours()), - }, - minutes: { - label: i18n.translate('xpack.apm.formatters.minutesTimeUnitLabel', { - defaultMessage: 'min', - }), - convert: (value: number) => - asDecimal(moment.duration(value / 1000).asMinutes()), - }, - seconds: { - label: i18n.translate('xpack.apm.formatters.secondsTimeUnitLabel', { - defaultMessage: 's', - }), - convert: (value: number) => - asDecimal(moment.duration(value / 1000).asSeconds()), - }, - milliseconds: { - label: i18n.translate('xpack.apm.formatters.millisTimeUnitLabel', { - defaultMessage: 'ms', - }), - convert: (value: number) => - asDecimal(moment.duration(value / 1000).asMilliseconds()), - }, - microseconds: { - label: i18n.translate('xpack.apm.formatters.microsTimeUnitLabel', { - defaultMessage: 'μs', - }), - convert: (value: number) => asInteger(value), - }, -}; +function asDecimalOrInteger(value: number) { + // exact 0 or above 10 should not have decimal + if (value === 0 || value >= 10) { + return asInteger(value); + } + return asDecimal(value); +} + +function getUnitLabelAndConvertedValue( + unitKey: DurationTimeUnit, + value: number +) { + switch (unitKey) { + case 'hours': { + return { + unitLabel: i18n.translate('xpack.apm.formatters.hoursTimeUnitLabel', { + defaultMessage: 'h', + }), + convertedValue: asDecimalOrInteger( + moment.duration(value / 1000).asHours() + ), + }; + } + case 'minutes': { + return { + unitLabel: i18n.translate('xpack.apm.formatters.minutesTimeUnitLabel', { + defaultMessage: 'min', + }), + convertedValue: asDecimalOrInteger( + moment.duration(value / 1000).asMinutes() + ), + }; + } + case 'seconds': { + return { + unitLabel: i18n.translate('xpack.apm.formatters.secondsTimeUnitLabel', { + defaultMessage: 's', + }), + convertedValue: asDecimalOrInteger( + moment.duration(value / 1000).asSeconds() + ), + }; + } + case 'milliseconds': { + return { + unitLabel: i18n.translate('xpack.apm.formatters.millisTimeUnitLabel', { + defaultMessage: 'ms', + }), + convertedValue: asDecimalOrInteger( + moment.duration(value / 1000).asMilliseconds() + ), + }; + } + case 'microseconds': { + return { + unitLabel: i18n.translate('xpack.apm.formatters.microsTimeUnitLabel', { + defaultMessage: 'μs', + }), + convertedValue: asInteger(value), + }; + } + } +} /** * Converts a microseconds value into the unit defined. @@ -87,16 +107,19 @@ function convertTo({ microseconds: Maybe; defaultValue?: string; }): ConvertedDuration { - const duration = durationUnit[unit]; - if (!duration || microseconds == null) { + if (microseconds == null) { return { value: defaultValue, formatted: defaultValue }; } - const convertedValue = duration.convert(microseconds); + const { convertedValue, unitLabel } = getUnitLabelAndConvertedValue( + unit, + microseconds + ); + return { value: convertedValue, - unit: duration.label, - formatted: `${convertedValue} ${duration.label}`, + unit: unitLabel, + formatted: `${convertedValue} ${unitLabel}`, }; }