Restores temporal ordering of spans in trace view (openzipkin#2312)

adriancole · shakuzen · commit 0afbcd9766c8 · 2018-12-04T16:49:53.000+09:00
diff --git a/zipkin-ui/js/component_ui/traceToMustache.js b/zipkin-ui/js/component_ui/traceToMustache.js
@@ -106,6 +106,19 @@ export function traceToMustache(root, logsUrl) {
     modelview.spans.push(spanRow);
   }
 
+  // Sort by timestamp, root first in case of skew. This makes the trace diagram display in
+  // increasing indent (temporal) order, which is a better experience than causal (topological)
+  // order in the current UI.
+  modelview.spans.sort((a, b) => {
+    if (!a.parentId && b.parentId) { // a is root
+      return -1;
+    } else if (a.parentId && !b.parentId) { // b is root
+      return 1;
+    }
+
+    return a.timestamp - b.timestamp;
+  });
+
   modelview.serviceNameAndSpanCounts = Object.keys(serviceNameToCount).sort().map(serviceName =>
     ({serviceName, spanCount: serviceNameToCount[serviceName]})
   );
diff --git a/zipkin-ui/test/component_ui/traceToMustache.test.js b/zipkin-ui/test/component_ui/traceToMustache.test.js
@@ -153,5 +153,43 @@ describe('traceToMustache', () => {
       {serviceName: 'frontend', spanCount: 1}
     ]);
   });
+
+  /*
+   * The following tree should not traverse in alphabetical order, as the UI intends to order by
+   * timstamp, not by cause. The reason is that the UI is row based, and people are used to seeing
+   * data descending to the right.
+   *
+   *          a
+   *        / | \
+   *       b  c  d
+   *      /|\
+   *     e f 1
+   *          \
+   *           2
+   */
+  it('should order spans by timestamp, root first, not by cause', () => {
+    const a = new SpanNode(clean({traceId: '1', id: 'a', timestamp: 8}));
+    const b = new SpanNode(clean({traceId: '1', id: 'b', parentId: 'a', timestamp: 7}));
+    const c = new SpanNode(clean({traceId: '1', id: 'c', parentId: 'a', timestamp: 6}));
+    const d = new SpanNode(clean({traceId: '1', id: 'd', parentId: 'a', timestamp: 5}));
+    // root(a) has children b, c, d
+    a.addChild(b);
+    a.addChild(c);
+    a.addChild(d);
+    const e = new SpanNode(clean({traceId: '1', id: 'e', parentId: 'b', timestamp: 4}));
+    const f = new SpanNode(clean({traceId: '1', id: 'f', parentId: 'b', timestamp: 3}));
+    const g = new SpanNode(clean({traceId: '1', id: '1', parentId: 'b', timestamp: 2}));
+    // child(b) has children e, f, g
+    b.addChild(e);
+    b.addChild(f);
+    b.addChild(g);
+    const h = new SpanNode(clean({traceId: '1', id: '2', parentId: '1', timestamp: 1}));
+    // f has no children
+    // child(g) has child h
+    g.addChild(h);
+
+    const {spans} = traceToMustache(a);
+    expect(spans.map(s => s.timestamp)).to.eql([8, 1, 2, 3, 4, 5, 6, 7]);
+  });
 });
 
