Skip to content

[APM] Fix gap in timeline and fallback when traceroot is missing #25346

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

Merged
sorenlouv merged 6 commits into from
Nov 8, 2018
Merged

[APM] Fix gap in timeline and fallback when traceroot is missing #25346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e54dab8
Fix gap in timeline
sorenlouv Nov 7, 2018
d39042a
Show fallback when traceroot is unavailable
sorenlouv Nov 7, 2018
8b21924
Fix test
sorenlouv Nov 7, 2018
4f88257
Fix rendering issues with styled-components
sorenlouv Nov 8, 2018
6c9dda0
Remove `parentTransactionSkew `
sorenlouv Nov 8, 2018
5aa08cf
Minor test fixes
sorenlouv Nov 8, 2018
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
2 changes: 1 addition & 1 deletion .../apm/public/components/app/TransactionDetails/Transaction/StickyTransactionProperties.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export function StickyTransactionProperties({
},
{
label: '% of trace',
val: asPercent(duration, totalDuration),
val: asPercent(duration, totalDuration, 'N/A'),
Copy link
Copy Markdown
Member

@jasonrhodes jasonrhodes Nov 8, 2018

Choose a reason for hiding this comment

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

Code review is a place where destructuring is nice IMO, e.g.

asPercent({ numerator: duration, denominator: totalDuration, fallback: 'N/A' })

mostly for the fallback value to have some context ... but let's not change that right now at the release deadline :)

Copy link
Copy Markdown
Contributor Author

@sorenlouv sorenlouv Nov 8, 2018

Choose a reason for hiding this comment

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

I can see how it makes it more readable here.

width: '25%'
},
{
Expand Down
18 changes: 11 additions & 7 deletions ...ponents/app/TransactionDetails/Transaction/WaterfallContainer/Waterfall/WaterfallItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,20 +31,16 @@ const ItemBar = styled<ItemBarProps, any>('div')`
box-sizing: border-box;
position: relative;
height: ${px(unit)};
left: ${props => props.left}%;
width: ${props => props.width}%;
min-width: 2px;
background-color: ${props => props.color};
`;

// Note: "direction: rtl;" is here to prevent text from running off of
// the right edge and instead pushing it to the left. For an example of
// how this works, see here: https://codepen.io/sqren/pen/JrXNjY
const SpanLabel = styled<{ left: number }, any>('div')`
const SpanLabel = styled.div`
white-space: nowrap;
position: relative;
left: ${props => `${props.left}%`};
width: ${props => `${Math.max(100 - props.left, 0)}%`};
direction: rtl;
text-align: left;
margin: ${px(units.quarter)} 0 0;
Expand Down Expand Up @@ -133,8 +129,16 @@ export function WaterfallItem({
isSelected={isSelected}
onClick={onClick}
>
<ItemBar left={left} width={width} color={color} type={item.docType} />
<Label left={left}>
<ItemBar
// using inline styles instead of props to avoid generating a css class for each item
style={{ left: `${left}%`, width: `${width}%` }}
color={color}
type={item.docType}
/>
<Label
// using inline styles instead of props to avoid generating a css class for each item
style={{ left: `${left}%`, width: `${Math.max(100 - left, 0)}%` }}
>
{item.name} <Prefix item={item} />
</Label>
</Container>
Expand Down
61 changes: 29 additions & 32 deletions ...ails/Transaction/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { groupBy, indexBy } from 'lodash';
import { groupBy } from 'lodash';
import { Span } from 'x-pack/plugins/apm/typings/Span';
import { Transaction } from 'x-pack/plugins/apm/typings/Transaction';
import {
getClockSkew,
getWaterfallItems,
IWaterfallIndex,
IWaterfallItem
} from './waterfall_helpers';

Expand Down Expand Up @@ -95,86 +94,84 @@ describe('waterfall_helpers', () => {
items,
hit => (hit.parentId ? hit.parentId : 'root')
);
const itemsById: IWaterfallIndex = indexBy(items, 'id');
const entryTransactionItem = childrenByParentId.root[0];
expect(
getWaterfallItems(childrenByParentId, itemsById, entryTransactionItem)
getWaterfallItems(childrenByParentId, entryTransactionItem)
).toMatchSnapshot();
});
});

describe('getClockSkew', () => {
it('should adjust when child starts before parent', () => {
const item = {
const child = {
docType: 'transaction',
timestamp: 0,
duration: 50
} as IWaterfallItem;

const parentItem = {
const parent = {
timestamp: 100,
skew: 5,
duration: 100
duration: 100,
skew: 5
} as IWaterfallItem;
const parentTransactionSkew = 1337;

expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(130);
expect(getClockSkew(child, parent)).toBe(130);
});

it('should adjust when child starts after parent has ended', () => {
const item = {
const child = {
docType: 'transaction',
timestamp: 250,
duration: 50
} as IWaterfallItem;

const parentItem = {
const parent = {
timestamp: 100,
skew: 5,
duration: 100
duration: 100,
skew: 5
} as IWaterfallItem;
const parentTransactionSkew = 1337;

expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(-120);
expect(getClockSkew(child, parent)).toBe(-120);
});

it('should not adjust when child starts within parent duration', () => {
const item = {
const child = {
docType: 'transaction',
timestamp: 150,
duration: 50
} as IWaterfallItem;

const parentItem = {
const parent = {
timestamp: 100,
skew: 5,
duration: 100
duration: 100,
skew: 5
} as IWaterfallItem;
const parentTransactionSkew = 1337;

expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(0);
expect(getClockSkew(child, parent)).toBe(0);
});

it('should return parentTransactionSkew for spans', () => {
const item = {
it('should return parent skew for spans', () => {
const child = {
docType: 'span'
} as IWaterfallItem;

const parentItem = {} as IWaterfallItem;
const parentTransactionSkew = 1337;
const parent = {
timestamp: 100,
duration: 100,
skew: 5
} as IWaterfallItem;

expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(1337);
expect(getClockSkew(child, parent)).toBe(5);
});

it('should handle missing parentItem', () => {
const item = {
it('should handle missing parent', () => {
const child = {
docType: 'transaction'
} as IWaterfallItem;

const parentItem = undefined;
const parentTransactionSkew = 1337;
const parent = undefined;

expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(0);
expect(getClockSkew(child, parent)).toBe(0);
});
});
});
41 changes: 16 additions & 25 deletions ...onDetails/Transaction/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export interface IWaterfallGroup {

export interface IWaterfall {
traceRoot?: Transaction;
traceRootDuration: number;
traceRootDuration?: number;

/**
* Duration in us
Expand Down Expand Up @@ -145,18 +145,19 @@ function getSpanItem(span: Span): IWaterfallItemSpan {

export function getClockSkew(
item: IWaterfallItem,
parentItem: IWaterfallItem | undefined,
parentTransactionSkew: number
parentItem?: IWaterfallItem
Copy link
Copy Markdown
Member

@jasonrhodes jasonrhodes Nov 8, 2018

Choose a reason for hiding this comment

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

this function really just needs the parentItem at this point, right?

Down on line 160 you even reference it as parentItem.skew, could you do the same on line 153 and remove parentTransactionSkew altogether?

Copy link
Copy Markdown
Contributor Author

@sorenlouv sorenlouv Nov 8, 2018

Choose a reason for hiding this comment

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

I don't think so. parentItem can both be a span and a transaction. parentTransactionSkew is for the parent transaction (which is not necessarily the parent).

Maybe we can simplify it though.

Copy link
Copy Markdown
Contributor Author

@sorenlouv sorenlouv Nov 8, 2018

Choose a reason for hiding this comment

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

... or you might actually be right.

Copy link
Copy Markdown
Contributor Author

@sorenlouv sorenlouv Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Removed parentTransactionSkew. Good idea!

) {
if (!parentItem) {
return 0;
}

switch (item.docType) {
// don't calculate skew for spans. Just use parent's skew
case 'span':
return parentTransactionSkew;
case 'transaction': {
// For some reason the parent span and related transactions might be missing.
if (!parentItem) {
return 0;
}
return parentItem.skew;

// transaction is the inital entry in a service. Calculate skew for this, and it will be propogated to all child spans
case 'transaction': {
const parentStart = parentItem.timestamp + parentItem.skew;
const parentEnd = parentStart + parentItem.duration;

Expand All @@ -182,28 +183,25 @@ export function getClockSkew(

export function getWaterfallItems(
childrenByParentId: IWaterfallGroup,
itemsById: IWaterfallIndex,
entryTransactionItem: IWaterfallItem
) {
function getSortedChildren(
item: IWaterfallItem,
parentTransactionSkew: number
parentItem?: IWaterfallItem
): IWaterfallItem[] {
const parentItem = item.parentId ? itemsById[item.parentId] : undefined;
const skew = getClockSkew(item, parentItem, parentTransactionSkew);
const children = sortBy(childrenByParentId[item.id] || [], 'timestamp');

item.childIds = children.map(child => child.id);
item.offset = item.timestamp - entryTransactionItem.timestamp;
item.skew = skew;
item.skew = getClockSkew(item, parentItem);

const deepChildren = flatten(
children.map(child => getSortedChildren(child, skew))
children.map(child => getSortedChildren(child, item))
);
return [item, ...deepChildren];
}

return getSortedChildren(entryTransactionItem, 0);
return getSortedChildren(entryTransactionItem);
}

function getTraceRoot(childrenByParentId: IWaterfallGroup) {
Expand Down Expand Up @@ -265,7 +263,6 @@ export function getWaterfall(
return {
services: [],
duration: 0,
traceRootDuration: 0,
items: [],
itemsById: {},
getTransactionById: () => undefined,
Expand Down Expand Up @@ -296,16 +293,10 @@ export function getWaterfall(
);
const entryTransactionItem = getTransactionItem(entryTransaction);
const itemsById: IWaterfallIndex = indexBy(filteredHits, 'id');
const items = getWaterfallItems(
childrenByParentId,
itemsById,
entryTransactionItem
);
const items = getWaterfallItems(childrenByParentId, entryTransactionItem);
const traceRoot = getTraceRoot(childrenByParentId);
const duration = getDuration(items);
const traceRootDuration = traceRoot
? traceRoot.transaction.duration.us
: duration;
const traceRootDuration = traceRoot && traceRoot.transaction.duration.us;
const services = getServices(items);
const getTransactionById = createGetTransactionById(itemsById);
const serviceColors = getServiceColors(services);
Expand Down
50 changes: 32 additions & 18 deletions x-pack/plugins/apm/public/components/app/TransactionDetails/Transaction/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,30 +30,44 @@ function MaybeViewTraceLink({
transaction: ITransaction;
waterfall: IWaterfall;
}) {
// the traceroot cannot be found, so we cannot link to it
if (!waterfall.traceRoot) {
return (
<EuiFlexItem grow={false}>
<EuiToolTip content="The trace parent cannot be found">
<EuiButton iconType="apmApp" disabled={true}>
View full trace
</EuiButton>
</EuiToolTip>
</EuiFlexItem>
);
}

const isRoot =
transaction.transaction.id ===
(waterfall.traceRoot && waterfall.traceRoot.transaction.id);
transaction.transaction.id === waterfall.traceRoot.transaction.id;

let button;
// the user is already viewing the full trace, so don't link to it
if (isRoot) {
button = (
<EuiToolTip content="Currently viewing the full trace">
<EuiButton iconType="apmApp" disabled={true}>
View full trace
</EuiButton>
</EuiToolTip>
return (
<EuiFlexItem grow={false}>
<EuiToolTip content="Currently viewing the full trace">
<EuiButton iconType="apmApp" disabled={true}>
View full trace
</EuiButton>
</EuiToolTip>
</EuiFlexItem>
);

// the user is viewing a zoomed in version of the trace. Link to the full trace
} else {
button = <EuiButton iconType="apmApp">View full trace</EuiButton>;
return (
<EuiFlexItem grow={false}>
<TransactionLink transaction={waterfall.traceRoot}>
<EuiButton iconType="apmApp">View full trace</EuiButton>
</TransactionLink>
</EuiFlexItem>
);
}

return (
<EuiFlexItem grow={false}>
<TransactionLink transaction={waterfall.traceRoot}>
{button}
</TransactionLink>
</EuiFlexItem>
);
}

interface Props {
Expand Down
16 changes: 9 additions & 7 deletions x-pack/plugins/apm/public/components/shared/charts/Timeline/TimelineAxis.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ import { getTimeFormatter } from '../../../../utils/formatters';

// Remove any tick that is too close to traceRootDuration
const getXAxisTickValues = (tickValues, traceRootDuration) => {
if (!tickValues) {
return [];
if (traceRootDuration == null) {
return tickValues;
}

const padding = (tickValues[1] - tickValues[0]) / 2;
Expand Down Expand Up @@ -73,11 +73,13 @@ function TimelineAxis({ plotValues, agentMarks, traceRootDuration }) {
}}
/>

<LastTickValue
x={xScale(traceRootDuration)}
value={tickFormat(traceRootDuration)}
marginTop={28}
/>
{traceRootDuration && (
<LastTickValue
x={xScale(traceRootDuration)}
value={tickFormat(traceRootDuration)}
marginTop={28}
/>
)}

{agentMarks.map(agentMark => (
<AgentMarker
Expand Down
9 changes: 8 additions & 1 deletion x-pack/plugins/apm/public/components/shared/charts/Timeline/VerticalLines.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,16 @@ class VerticalLines extends PureComponent {
/>

<VerticalGridLines
tickValues={[...agentMarkTimes, traceRootDuration]}
tickValues={agentMarkTimes}
style={{ stroke: colors.gray3 }}
/>

{traceRootDuration && (
<VerticalGridLines
tickValues={[traceRootDuration]}
style={{ stroke: colors.gray3 }}
/>
)}
</XYPlot>
</div>
);
Expand Down
5 changes: 5 additions & 0 deletions ...apm/public/components/shared/charts/Timeline/__test__/__snapshots__/Timeline.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,11 @@ exports[`Timeline should render with data 1`] = `
y1={0}
y2={116}
/>
</g>
<g
className="rv-xy-plot__grid-lines"
transform="translate(50,100)"
>
<line
className="rv-xy-plot__grid-lines__line"
style={
Expand Down