[APM] Additional clock skew fixes (#25097)

* Refactor service colors

* Calculate duration from full waterfall

* Ensure timeline label is not truncated

* Adjust child if it starts after parent has ended

* Add mark for traceRootDuration instead of xMax

* Fix tests

x-pack/plugins/apm/public/components/app/TransactionDetails/Transaction/WaterfallContainer/ServiceLegends.tsx

@@ -10,6 +10,7 @@ import styled from 'styled-components';
 import { px, unit } from '../../../../../style/variables';
 // @ts-ignore
 import Legend from '../../../../shared/charts/Legend';
+import { IServiceColors } from './Waterfall/waterfall_helpers/waterfall_helpers';
 
 const Legends = styled.div`
   display: flex;
@@ -23,9 +24,7 @@ const Legends = styled.div`
 `;
 
 interface Props {
-  serviceColors: {
-    [key: string]: string;
-  };
+  serviceColors: IServiceColors;
 }
 
 export function ServiceLegends({ serviceColors }: Props) {

x-pack/plugins/apm/public/components/app/TransactionDetails/Transaction/WaterfallContainer/Waterfall/WaterfallItem.tsx

@@ -44,7 +44,7 @@ const SpanLabel = styled<{ left: number }, any>('div')`
   white-space: nowrap;
   position: relative;
   left: ${props => `${props.left}%`};
-  width: ${props => `${100 - props.left}%`};
+  width: ${props => `${Math.max(100 - props.left, 0)}%`};
   direction: rtl;
   text-align: left;
   margin: ${px(units.quarter)} 0 0;

x-pack/plugins/apm/public/components/app/TransactionDetails/Transaction/WaterfallContainer/Waterfall/index.tsx

@@ -19,6 +19,7 @@ import { AgentMark } from '../get_agent_marks';
 import { SpanFlyout } from './SpanFlyout';
 import { TransactionFlyout } from './TransactionFlyout';
 import {
+  IServiceColors,
   IWaterfall,
   IWaterfallItem
 } from './waterfall_helpers/waterfall_helpers';
@@ -42,9 +43,7 @@ interface Props {
   urlParams: IUrlParams;
   waterfall: IWaterfall;
   location: any;
-  serviceColors: {
-    [key: string]: string;
-  };
+  serviceColors: IServiceColors;
 }
 
 export class Waterfall extends Component<Props> {
@@ -129,6 +128,7 @@ export class Waterfall extends Component<Props> {
           <Timeline
             agentMarks={this.props.agentMarks}
             duration={waterfall.duration}
+            traceRootDuration={waterfall.traceRootDuration}
             height={waterfallHeight}
             margins={TIMELINE_MARGINS}
           />
@@ -157,8 +157,3 @@ export class Waterfall extends Component<Props> {
     });
   }
 }
-
-// TODO: the agent marks and note about dropped spans were removed. Need to be re-added
-//   agentMarks: PropTypes.array,
-//   agentName: PropTypes.string.isRequired,
-//   droppedSpans: PropTypes.number.isRequired,

x-pack/plugins/apm/public/components/app/TransactionDetails/Transaction/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.test.ts

@@ -8,6 +8,7 @@ import { groupBy, indexBy } 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
@@ -101,4 +102,79 @@ describe('waterfall_helpers', () => {
       ).toMatchSnapshot();
     });
   });
+
+  describe('getClockSkew', () => {
+    it('should adjust when child starts before parent', () => {
+      const item = {
+        docType: 'transaction',
+        timestamp: 0,
+        duration: 50
+      } as IWaterfallItem;
+
+      const parentItem = {
+        timestamp: 100,
+        skew: 5,
+        duration: 100
+      } as IWaterfallItem;
+      const parentTransactionSkew = 1337;
+
+      expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(130);
+    });
+
+    it('should adjust when child starts after parent has ended', () => {
+      const item = {
+        docType: 'transaction',
+        timestamp: 250,
+        duration: 50
+      } as IWaterfallItem;
+
+      const parentItem = {
+        timestamp: 100,
+        skew: 5,
+        duration: 100
+      } as IWaterfallItem;
+      const parentTransactionSkew = 1337;
+
+      expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(-120);
+    });
+
+    it('should not adjust when child starts within parent duration', () => {
+      const item = {
+        docType: 'transaction',
+        timestamp: 150,
+        duration: 50
+      } as IWaterfallItem;
+
+      const parentItem = {
+        timestamp: 100,
+        skew: 5,
+        duration: 100
+      } as IWaterfallItem;
+      const parentTransactionSkew = 1337;
+
+      expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(0);
+    });
+
+    it('should return parentTransactionSkew for spans', () => {
+      const item = {
+        docType: 'span'
+      } as IWaterfallItem;
+
+      const parentItem = {} as IWaterfallItem;
+      const parentTransactionSkew = 1337;
+
+      expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(1337);
+    });
+
+    it('should handle missing parentItem', () => {
+      const item = {
+        docType: 'transaction'
+      } as IWaterfallItem;
+
+      const parentItem = undefined;
+      const parentTransactionSkew = 1337;
+
+      expect(getClockSkew(item, parentItem, parentTransactionSkew)).toBe(0);
+    });
+  });
 });

x-pack/plugins/apm/public/components/app/TransactionDetails/Transaction/WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers.ts

@@ -11,8 +11,10 @@ import {
   indexBy,
   isEmpty,
   sortBy,
-  uniq
+  uniq,
+  zipObject
 } from 'lodash';
+import { colors } from 'x-pack/plugins/apm/public/style/variables';
 import { Span } from '../../../../../../../../typings/Span';
 import { Transaction } from '../../../../../../../../typings/Transaction';
 
@@ -26,22 +28,43 @@ export interface IWaterfallGroup {
 
 export interface IWaterfall {
   traceRoot?: Transaction;
-  traceRootDuration?: number;
-  duration?: number;
+  traceRootDuration: number;
+
+  /**
+   * Duration in us
+   */
+  duration: number;
   services: string[];
   items: IWaterfallItem[];
   itemsById: IWaterfallIndex;
   getTransactionById: (id?: IWaterfallItem['id']) => Transaction | undefined;
+  serviceColors: IServiceColors;
 }
 
 interface IWaterfallItemBase {
   id: string | number;
   parentId?: string;
   serviceName: string;
   name: string;
+
+  /**
+   * Duration in us
+   */
   duration: number;
+
+  /**
+   * start timestamp in us
+   */
   timestamp: number;
+
+  /**
+   * offset from first item in us
+   */
   offset: number;
+
+  /**
+   * skew from timestamp in us
+   */
   skew: number;
   childIds?: Array<IWaterfallItemBase['id']>;
 }
@@ -120,38 +143,39 @@ function getSpanItem(span: Span): IWaterfallItemSpan {
   };
 }
 
-function getClockSkew(
+export function getClockSkew(
   item: IWaterfallItem,
-  itemsById: IWaterfallIndex,
+  parentItem: IWaterfallItem | undefined,
   parentTransactionSkew: number
 ) {
   switch (item.docType) {
     case 'span':
       return parentTransactionSkew;
     case 'transaction': {
-      if (!item.parentId) {
-        return 0;
-      }
-
-      const parentItem = itemsById[item.parentId];
-
       // For some reason the parent span and related transactions might be missing.
       if (!parentItem) {
         return 0;
       }
 
-      // determine if child starts before the parent, and in that case how much
-      const diff = parentItem.timestamp + parentItem.skew - item.timestamp;
+      const parentStart = parentItem.timestamp + parentItem.skew;
+      const parentEnd = parentStart + parentItem.duration;
 
-      // If child transaction starts after parent span there is no clock skew
-      if (diff < 0) {
+      // determine if child starts before the parent
+      const offsetStart = parentStart - item.timestamp;
+
+      // determine if child starts after the parent has ended
+      const offsetEnd = item.timestamp - parentEnd;
+
+      // child transaction starts before parent OR
+      // child transaction starts after parent has ended
+      if (offsetStart > 0 || offsetEnd > 0) {
+        const latency = Math.max(parentItem.duration - item.duration, 0) / 2;
+        return offsetStart + latency;
+
+        // child transaction starts withing parent duration and no adjustment is needed
+      } else {
         return 0;
       }
-
-      // latency can only be calculated if parent duration is larger than child duration
-      const latency = Math.max(parentItem.duration - item.duration, 0);
-      const skew = diff + latency / 2;
-      return skew;
     }
   }
 }
@@ -165,7 +189,8 @@ export function getWaterfallItems(
     item: IWaterfallItem,
     parentTransactionSkew: number
   ): IWaterfallItem[] {
-    const skew = getClockSkew(item, itemsById, parentTransactionSkew);
+    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);
@@ -193,16 +218,58 @@ function getServices(items: IWaterfallItem[]) {
   return uniq(serviceNames);
 }
 
+export interface IServiceColors {
+  [key: string]: string;
+}
+
+function getServiceColors(services: string[]) {
+  const assignedColors = [
+    colors.apmBlue,
+    colors.apmGreen,
+    colors.apmPurple,
+    colors.apmRed2,
+    colors.apmTan,
+    colors.apmOrange,
+    colors.apmYellow
+  ];
+
+  return zipObject(services, assignedColors) as IServiceColors;
+}
+
+function getDuration(items: IWaterfallItem[]) {
+  const timestampStart = items[0].timestamp;
+  const timestampEnd = Math.max(
+    ...items.map(item => item.timestamp + item.duration + item.skew)
+  );
+  return timestampEnd - timestampStart;
+}
+
+function createGetTransactionById(itemsById: IWaterfallIndex) {
+  return (id?: IWaterfallItem['id']) => {
+    if (!id) {
+      return;
+    }
+
+    const item = itemsById[id];
+    if (item.docType === 'transaction') {
+      return item.transaction;
+    }
+  };
+}
+
 export function getWaterfall(
   hits: Array<Span | Transaction>,
   entryTransaction: Transaction
 ): IWaterfall {
   if (isEmpty(hits)) {
     return {
       services: [],
+      duration: 0,
+      traceRootDuration: 0,
       items: [],
       itemsById: {},
-      getTransactionById: () => undefined
+      getTransactionById: () => undefined,
+      serviceColors: {}
     };
   }
 
@@ -235,25 +302,22 @@ export function getWaterfall(
     entryTransactionItem
   );
   const traceRoot = getTraceRoot(childrenByParentId);
-
-  const getTransactionById = (id?: IWaterfallItem['id']) => {
-    if (!id) {
-      return;
-    }
-
-    const item = itemsById[id];
-    if (item.docType === 'transaction') {
-      return item.transaction;
-    }
-  };
+  const duration = getDuration(items);
+  const traceRootDuration = traceRoot
+    ? traceRoot.transaction.duration.us
+    : duration;
+  const services = getServices(items);
+  const getTransactionById = createGetTransactionById(itemsById);
+  const serviceColors = getServiceColors(services);
 
   return {
     traceRoot,
-    traceRootDuration: traceRoot && traceRoot.transaction.duration.us,
-    duration: entryTransaction.transaction.duration.us,
-    services: getServices(items),
+    traceRootDuration,
+    duration,
+    services,
     items,
     itemsById,
-    getTransactionById
+    getTransactionById,
+    serviceColors
   };
 }