Corrects clock-skew on single host spans

See #1480
openzipkin · Nov 8, 2018 · 2504cee · 2504cee
1 parent d589ca3
commit 2504cee
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 8 deletions.
diff --git a/zipkin-ui/js/skew.js b/zipkin-ui/js/skew.js
@@ -241,7 +241,7 @@ function oneWaySkew(serverRecv, clientSend) {
 }
 
 // Uses client/server annotations to determine if there's clock skew.
-function getClockSkew(span) {
+function getClockSkew(parent, span) {
   let clientSend;
   let serverRecv;
   let serverSend;
@@ -265,6 +265,21 @@ function getClockSkew(span) {
     }
   });
 
+  // This may be a single-host span. Look for the parent
+  if (serverRecv && !clientSend && parent) {
+    (parent.annotations || []).forEach((a) => {
+      switch (a.value) {
+        case 'cs':
+          clientSend = a;
+          break;
+        case 'cr':
+          clientRecv = a;
+          break;
+        default:
+      }
+    });
+  }
+
   let oneWay = false;
   if (!clientSend || !serverRecv) {
     return undefined;
@@ -351,8 +366,11 @@ function adjust(node, skewFromParent) {
     node.setValue(adjustTimestamps(node.value, skewFromParent));
   }
 
-  // Is there any skew in the current span?
-  let skew = getClockSkew(node.value);
+  // An RPC span can share an ID (have the same ID) or be split across parent and child
+  // We only look for skew between a client and the server
+  const parentVal = node.parent ? node.parent.value : undefined;
+  let skew = getClockSkew(parentVal, node.value);
+
   if (skew) {
     // the current span's skew may be a different endpoint than its parent, so adjust again.
     node.setValue(adjustTimestamps(node.value, skew));

diff --git a/zipkin-ui/test/skew.test.js b/zipkin-ui/test/skew.test.js
@@ -283,7 +283,7 @@ describe('correctForClockSkew', () => {
         {timestamp: 40, value: 'cr', endpoint: frontend}
       ]
     };
-    should.equal(getClockSkew(skewedSameHost), undefined);
+    should.equal(getClockSkew(undefined, skewedSameHost), undefined);
   });
 
   /*
@@ -297,7 +297,7 @@ describe('correctForClockSkew', () => {
     const cr = {timestamp: 40, value: 'cr', endpoint: frontend};
     const skewedRpc = {traceId: '1', id: '1', annotations: [cs, sr, ss, cr]};
 
-    const skew = getClockSkew(skewedRpc);
+    const skew = getClockSkew(undefined, skewedRpc);
 
     // Skew correction pushes the server side forward, so the skew endpoint is the server
     expect(skew.endpoint).to.equal(sr.endpoint);
@@ -311,13 +311,26 @@ describe('correctForClockSkew', () => {
     );
   });
 
+  it('corrects skew on single-host spans', () => {
+    const cs = {timestamp: 20, value: 'cs', endpoint: frontend};
+    const sr = {timestamp: 10 /* skew */, value: 'sr', endpoint: backend};
+    const ss = {timestamp: 20, value: 'ss', endpoint: backend};
+    const cr = {timestamp: 40, value: 'cr', endpoint: frontend};
+
+    const skewedRpc = {traceId: '1', id: '1', annotations: [cs, sr, ss, cr]};
+    const skewedClient = {traceId: '1', id: '1', annotations: [cs, cr]};
+    const skewedServer = {traceId: '1', id: '2', annotations: [sr, ss]};
+
+    expect(getClockSkew(skewedClient, skewedServer)).to.eql(getClockSkew(undefined, skewedRpc));
+  });
+
   // Sets the server to 1us past the client
   it('skew on one-way spans assumes latency is at least 1us', () => {
     const cs = {timestamp: 20, value: 'cs', endpoint: frontend};
     const sr = {timestamp: 10 /* skew */, value: 'sr', endpoint: backend};
     const skewedOneWay = {traceId: '1', id: '1', annotations: [cs, sr]};
 
-    const skew = getClockSkew(skewedOneWay);
+    const skew = getClockSkew(undefined, skewedOneWay);
 
     expect(skew.skew).to.equal(
       sr.timestamp - cs.timestamp // how much sr is behind
@@ -333,7 +346,7 @@ describe('correctForClockSkew', () => {
     const cr = {timestamp: 25, value: 'cr', endpoint: frontend};
     const skewedAsyncRpc = {traceId: '1', id: '1', annotations: [cs, sr, ss, cr]};
 
-    const skew = getClockSkew(skewedAsyncRpc);
+    const skew = getClockSkew(undefined, skewedAsyncRpc);
 
     expect(skew.skew).to.equal(
       sr.timestamp - cs.timestamp // how much sr is behind
@@ -352,7 +365,7 @@ describe('correctForClockSkew', () => {
         {timestamp: 40, value: 'cr'}
       ]
     };
-    should.equal(getClockSkew(skewedButNoendpoints), undefined);
+    should.equal(getClockSkew(undefined, skewedButNoendpoints), undefined);
   });
 
   it('span with the mixed endpoints is not single-host', () => {
@@ -413,6 +426,30 @@ describe('correctForClockSkew', () => {
     ]);
   });
 
+  it('should correct single-host one-way RPC spans', () => {
+    const trace = [
+      {
+        traceId: '1',
+        id: '1',
+        annotations: [{timestamp: 20, value: 'cs', endpoint: frontend}]
+      },
+      {
+        traceId: '1',
+        parentId: '1',
+        id: '2',
+        annotations: [{timestamp: 10 /* skew */, value: 'sr', endpoint: backend}]
+      }
+    ];
+
+    const adjusted = correctForClockSkew(trace);
+
+    expect(adjusted.length).to.equal(2);
+    expect(adjusted[0]).to.eql(trace[0]);
+    expect(adjusted[1].annotations).to.deep.equal([
+      {timestamp: 21 /* pushed 1us later */, value: 'sr', endpoint: backend}
+    ]);
+  });
+
   function assertClockSkewIsCorrectlyApplied(skew) {
     const rpcClientSendTs = 50;
     const dbClientSendTimestamp = 60 + skew;