Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,17 @@ import styled from 'styled-components';
import { RelativeLink } from '../../../../utils/url';
import { fontSizes, truncate } from '../../../../style/variables';
import TooltipOverlay from '../../../shared/TooltipOverlay';
import { asMillisWithDefault, asDecimal } from '../../../../utils/formatters';
import { asMillis, asDecimal } from '../../../../utils/formatters';
import { ManagedTable } from '../../../shared/ManagedTable';

// TODO: Consolidate these formatting helpers centrally
function formatNumber(value) {
if (value === 0) {
return '0';
} else if (value <= 0.1) {
return '< 0.1';
} else {
return asDecimal(value);
}
const formatted = asDecimal(value);
return formatted <= 0.1 ? '< 0.1' : formatted;
}

function formatString(value) {
Expand Down Expand Up @@ -56,7 +57,7 @@ const SERVICE_COLUMNS = [
name: 'Avg. response time',
sortable: true,
dataType: 'number',
render: value => asMillisWithDefault(value)
render: value => asMillis(value)
},
{
field: 'transactionsPerMinute',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import React from 'react';
import styled from 'styled-components';
import { ITransactionGroup } from '../../../../typings/TransactionGroup';
import { fontSizes, truncate } from '../../../style/variables';
import { asMillisWithDefault } from '../../../utils/formatters';
import { asMillis } from '../../../utils/formatters';
import { ImpactBar } from '../../shared/ImpactBar';
import { ITableColumn, ManagedTable } from '../../shared/ManagedTable';
// @ts-ignore
Expand Down Expand Up @@ -50,7 +50,7 @@ const traceListColumns: ITableColumn[] = [
name: 'Avg. response time',
sortable: true,
dataType: 'number',
render: (value: number) => asMillisWithDefault(value)
render: (value: number) => asMillis(value)
},
{
field: 'transactionsPerMinute',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,9 @@ export class Distribution extends Component<Props> {
bucket.y > 0 && bucket.sample
}
tooltipHeader={(bucket: IChartPoint) =>
`${timeFormatter(bucket.x0, false)} - ${timeFormatter(
`${timeFormatter(bucket.x0, { withUnit: false })} - ${timeFormatter(
bucket.x,
false
{ withUnit: false }
)} ${unit}`
}
tooltipFooter={(bucket: IChartPoint) =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export function StickyTransactionProperties({
{
label: 'Duration',
fieldName: TRANSACTION_DURATION,
val: duration ? asTime(duration) : 'N/A',
val: asTime(duration),
width: '25%'
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,7 @@ import React from 'react';
import styled from 'styled-components';
import TooltipOverlay from '../../../shared/TooltipOverlay';
import { RelativeLink, legacyEncodeURIComponent } from '../../../../utils/url';
import {
asMillisWithDefault,
asDecimal,
tpmUnit
} from '../../../../utils/formatters';
import { asMillis, asDecimal, tpmUnit } from '../../../../utils/formatters';
import { ImpactBar } from '../../../shared/ImpactBar';

import { fontFamilyCode, truncate } from '../../../../style/variables';
Expand Down Expand Up @@ -63,14 +59,14 @@ export default function TransactionList({
name: avgLabel(agentName),
sortable: true,
dataType: 'number',
render: value => asMillisWithDefault(value)
render: value => asMillis(value)
},
{
field: 'p95',
name: '95th percentile',
sortable: true,
dataType: 'number',
render: value => asMillisWithDefault(value)
render: value => asMillis(value)
},
{
field: 'transactionsPerMinute',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ describe('Histogram', () => {
formatYShort={t => `${asDecimal(t)} occ.`}
formatYLong={t => `${asDecimal(t)} occurrences`}
tooltipHeader={bucket =>
`${timeFormatter(bucket.x0, false)} - ${timeFormatter(
`${timeFormatter(bucket.x0, { withUnit: false })} - ${timeFormatter(
bucket.x,
false
{ withUnit: false }
)} ${unit}`
}
width={800}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ exports[`Histogram Initially should have default markup 1`] = `
textAnchor="middle"
transform="translate(0, 18)"
>
0
0 ms
</text>
</g>
<g
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ exports[`Timeline should render with data 1`] = `
textAnchor="middle"
transform="translate(0, -20)"
>
0
0 ms
</text>
</g>
<g
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,7 @@ export class Charts extends Component {
};

getResponseTimeTooltipFormatter = (p = {}) => {
if (this.props.charts.noHits) {
return '- ms';
} else {
return p.y == null ? 'N/A' : asMillis(p.y);
}
return this.props.charts.noHits ? '- ms' : asMillis(p.y);
};

getTPMFormatter = t => {
Expand Down
8 changes: 2 additions & 6 deletions x-pack/plugins/apm/public/store/selectors/chartSelectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,7 @@
import d3 from 'd3';
import { last, zipObject, difference, memoize, get, isEmpty } from 'lodash';
import { colors } from '../../style/variables';
import {
asMillisWithDefault,
asDecimal,
tpmUnit
} from '../../utils/formatters';
import { asMillis, asDecimal, tpmUnit } from '../../utils/formatters';
import { rgba } from 'polished';

export const getEmptySerie = memoize(
Expand Down Expand Up @@ -59,7 +55,7 @@ export function getResponseTimeSeries(chartsData) {
{
title: 'Avg.',
data: getChartValues(dates, avg),
legendValue: `${asMillisWithDefault(overallAvgDuration)}`,
legendValue: asMillis(overallAvgDuration),
type: 'line',
color: colors.apmBlue
},
Expand Down
24 changes: 19 additions & 5 deletions x-pack/plugins/apm/public/utils/__test__/formatters.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,25 @@
import { asPercent, asTime } from '../formatters';

describe('formatters', () => {
it('asTime', () => {
expect(asTime(1000)).toBe('1 ms');
expect(asTime(1000 * 1000)).toBe('1,000 ms');
expect(asTime(1000 * 1000 * 10)).toBe('10,000 ms');
expect(asTime(1000 * 1000 * 20)).toBe('20.0 s');
describe('asTime', () => {
it('formats correctly with defaults', () => {
expect(asTime(null)).toBe('N/A');
expect(asTime(undefined)).toBe('N/A');
expect(asTime(0)).toBe('0 μs');
expect(asTime(1)).toBe('1 μs');
expect(asTime(1000)).toBe('1,000 μs');
expect(asTime(1000 * 1000)).toBe('1,000 ms');
expect(asTime(1000 * 1000 * 10)).toBe('10,000 ms');
expect(asTime(1000 * 1000 * 20)).toBe('20.0 s');
});

it('formats without unit', () => {
expect(asTime(1000, { withUnit: false })).toBe('1,000');
});

it('falls back to default value', () => {
expect(asTime(undefined, { defaultValue: 'nope' })).toBe('nope');
});
});

describe('asPercent', () => {
Expand Down
95 changes: 71 additions & 24 deletions x-pack/plugins/apm/public/utils/formatters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,60 +4,107 @@
* you may not use this file except in compliance with the Elastic License.
*/

import numeral from '@elastic/numeral';
import { memoize } from 'lodash';

// tslint:disable-next-line no-var-requires
const numeral: (input: number) => Numeral = require('@elastic/numeral');
const SECONDS_CUT_OFF = 10 * 1000000; // 10 seconds (in microseconds)
const MILLISECONDS_CUT_OFF = 10 * 1000; // 10 milliseconds (in microseconds)

interface Numeral {
format: (pattern: string) => string;
/*
* value: time in microseconds
* withUnit: add unit suffix
* defaultValue: value to use if the specified is null/undefined
*/
type FormatterValue = number | undefined | null;
interface FormatterOptions {
withUnit?: boolean;
defaultValue?: string;
}

const UNIT_CUT_OFF = 10 * 1000000; // 10 seconds in microseconds

export function asSeconds(value: number, withUnit = true) {
export function asSeconds(
value: FormatterValue,
{ withUnit = true, defaultValue = 'N/A' }: FormatterOptions = {}
) {
if (value == null) {
return defaultValue;
}
const formatted = asDecimal(value / 1000000);
return `${formatted}${withUnit ? ' s' : ''}`;
}

export function asMillis(value: number, withUnit = true) {
export function asMillis(
value: FormatterValue,
{ withUnit = true, defaultValue = 'N/A' }: FormatterOptions = {}
) {
if (value == null) {
return defaultValue;
}

const formatted = asInteger(value / 1000);
return `${formatted}${withUnit ? ' ms' : ''}`;
}

export function asMillisWithDefault(value?: number) {
export function asMicros(
value: FormatterValue,
{ withUnit = true, defaultValue = 'N/A' }: FormatterOptions = {}
) {
if (value == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i know this isn't part of the PR, but just wanted to point out that value == null will never be reached since TS only allows number | undefined type for value

Copy link
Contributor

Choose a reason for hiding this comment

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

my mistake, this line doesn't use strict equality, so the == null evaluates to true for undefined values...we should probably add a linting rule to enforce strict equality

Copy link
Member Author

@sorenlouv sorenlouv Nov 1, 2018

Choose a reason for hiding this comment

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

Oh, good catch. I was going to remove asMillisWithDefault and was going to use the approach suggested in #24974 (comment).

I see two approaches: either I make value optional or I remove defaultValue (and then consumers have to handle that themselves). What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

my mistake, this line doesn't use strict equality, so the == null evaluates to true for undefined values

Yes, that was also the intention. So whenever the string is null or undefined we use the default.

we should probably add a linting rule to enforce strict equality

We are already using this rule that disallows this:

const a = 0 == undefined;

AFAICT the default is to allow double equals specifically for null and undefined. Imho this makes sense as a shorthand for myvar === undefined && myvar === null.

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 with TS, these checks for different kinds of 'nil' values become less necessary. Just something to consider as we migrate more toward full TS source.

Copy link
Member Author

@sorenlouv sorenlouv Nov 1, 2018

Choose a reason for hiding this comment

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

TS will definitely eliminate a lot of runtime checks. Looking forward to that!
In this case I'm not 100% sure. Most of the usages of the formatters are still in js-land, which makes this change both easier and harder. Easier because I don't get any compile errors from the current consumers; harder because I might be breaking their contract without knowing.

Let's say that the consumers are currently calling asTime with a value that can be null, undefined and the actual value. Then we either need to:

  • rewrite the formatters to accept null and undefined, and keep the "nil" check in place
  • rewrite the consumers so they never call the formatters with null or undefined. Then we can remove the "nil check"

return `N/A`;
return defaultValue;
}
return asMillis(value);

const formatted = asInteger(value);
return `${formatted}${withUnit ? ' μs' : ''}`;
}

export const getTimeFormatter: (
type TimeFormatter = (
max: number
) => (value: number, withUnit?: boolean) => string = memoize(
(max: number) => (max > UNIT_CUT_OFF ? asSeconds : asMillis)
);
) => (
value: FormatterValue,
{ withUnit, defaultValue }: FormatterOptions
) => string;

export const getTimeFormatter: TimeFormatter = memoize((max: number) => {
const unit = timeUnit(max);
switch (unit) {
case 's':
return asSeconds;
case 'ms':
return asMillis;
case 'us':
return asMicros;
}
});

export function timeUnit(max: number) {
return max > UNIT_CUT_OFF ? 's' : 'ms';
if (max > SECONDS_CUT_OFF) {
return 's';
} else if (max > MILLISECONDS_CUT_OFF) {
return 'ms';
} else {
return 'us';
}
}

/*
* value: time in microseconds
*/
export function asTime(value: number): string {
return getTimeFormatter(value)(value);
export function asTime(
value: FormatterValue,
{ withUnit = true, defaultValue = 'N/A' }: FormatterOptions = {}
) {
if (value == null) {
return defaultValue;
}
const formatter = getTimeFormatter(value);
return formatter(value, { withUnit, defaultValue });
}

export function asDecimal(value: number): string {
export function asDecimal(value: number) {
return numeral(value).format('0,0.0');
}

export function asInteger(value: number): string {
export function asInteger(value: number) {
return numeral(value).format('0,0');
}

export function tpmUnit(type: string): string {
export function tpmUnit(type: string) {
return type === 'request' ? 'rpm' : 'tpm';
}

Expand Down
16 changes: 16 additions & 0 deletions x-pack/plugins/apm/typings/numeral.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

interface Numeral {
(value?: any): Numeral;
format: (pattern: string) => string;
}

declare var numeral: Numeral;

declare module '@elastic/numeral' {
export = numeral;
}