From 1deb081c6627a8c26425d2dbae768fcb3318e5a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Sat, 20 Jul 2019 18:05:05 +0200 Subject: [PATCH 1/4] feat(#362): adds flush method to http reporter. --- packages/zipkin/index.d.ts | 1 + packages/zipkin/src/batch-recorder.js | 22 +++++++++++++++-- packages/zipkin/test/batch-recorder.test.js | 27 +++++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/packages/zipkin/index.d.ts b/packages/zipkin/index.d.ts index 54789b1b..1a9102ea 100644 --- a/packages/zipkin/index.d.ts +++ b/packages/zipkin/index.d.ts @@ -274,6 +274,7 @@ declare namespace zipkin { class BatchRecorder implements Recorder { constructor(args: { logger: Logger, timeout?: number }); record: (rec: Record) => void; + flush: () => void; } class ConsoleRecorder implements Recorder { diff --git a/packages/zipkin/src/batch-recorder.js b/packages/zipkin/src/batch-recorder.js index 0f062dcb..fcab58ed 100644 --- a/packages/zipkin/src/batch-recorder.js +++ b/packages/zipkin/src/batch-recorder.js @@ -7,6 +7,12 @@ const {Span, Endpoint} = require('./model'); */ const defaultTimeout = 60 * 1000000; +/** + * default batch interval 1 second (in miliseconds) + * @type {number} + */ +const defaultBatchInterval = 1000; + /** * defaultTags property name * @type {symbol} @@ -63,13 +69,19 @@ class BatchRecorder { * @param {Object} options * @property {Logger} logger logs the data to zipkin server * @property {number} timeout timeout for span in microseconds + * @property {number} batchInterval interval for reporting in miliseconds. + * The value -1 means that there won't be batch reporting on interval basis. */ - constructor({logger, timeout = defaultTimeout}) { + constructor({logger, timeout = defaultTimeout, batchInterval = defaultBatchInterval}) { this.logger = logger; this.timeout = timeout; this.partialSpans = new Map(); this[defaultTagsSymbol] = {}; + if (batchInterval === -1) { + return; + } + // read through the partials spans regularly // and collect any timed-out ones const timer = setInterval(() => { @@ -82,7 +94,7 @@ class BatchRecorder { this._writeSpan(id, span); } }); - }, 1000); + }, batchInterval); if (timer.unref) { // unref might not be available in browsers timer.unref(); // Allows Node to terminate instead of blocking on timer } @@ -135,6 +147,12 @@ class BatchRecorder { } } + flush() { + this.partialSpans.forEach((span, id) => { + this._writeSpan(id, span); + }); + } + record(rec) { const id = rec.traceId; diff --git a/packages/zipkin/test/batch-recorder.test.js b/packages/zipkin/test/batch-recorder.test.js index f39c8e70..d2b5e6c6 100644 --- a/packages/zipkin/test/batch-recorder.test.js +++ b/packages/zipkin/test/batch-recorder.test.js @@ -302,8 +302,35 @@ describe('Batch Recorder - integration test', () => { 'http.status_code': '200' } }); + }); + + it('should only flush spans when calling flush method', () => { + const clock = lolex.install(); + recorder = new BatchRecorder({ + logger: { + logSpan: (span) => { + spans.push(JSON.parse(JSON_V2.encode(span))); + } + } + }); + + recorder.record(record(rootId, now(), new Annotation.ServerRecv())); expect(spans).to.be.empty; // eslint-disable-line no-unused-expressions + + clock.tick(100); // 1000 is de batching interval + + expect(spans).to.be.empty; // eslint-disable-line no-unused-expressions + + recorder.flush(); + + expect(popSpan()).to.deep.equal({ + traceId: rootId.traceId, + id: rootId.spanId, + kind: 'SERVER' + }); + + clock.uninstall(); }); it('should handle overlapping server and client', () => { From 467b9f16fa78e7d0949279361c144932f1ed3494 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Sun, 21 Jul 2019 19:52:51 +0200 Subject: [PATCH 2/4] chore(#362): refactors flush setup based on @adriancole's feedback. --- packages/zipkin/src/batch-recorder.js | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/packages/zipkin/src/batch-recorder.js b/packages/zipkin/src/batch-recorder.js index fcab58ed..e118ed95 100644 --- a/packages/zipkin/src/batch-recorder.js +++ b/packages/zipkin/src/batch-recorder.js @@ -7,12 +7,6 @@ const {Span, Endpoint} = require('./model'); */ const defaultTimeout = 60 * 1000000; -/** - * default batch interval 1 second (in miliseconds) - * @type {number} - */ -const defaultBatchInterval = 1000; - /** * defaultTags property name * @type {symbol} @@ -69,19 +63,13 @@ class BatchRecorder { * @param {Object} options * @property {Logger} logger logs the data to zipkin server * @property {number} timeout timeout for span in microseconds - * @property {number} batchInterval interval for reporting in miliseconds. - * The value -1 means that there won't be batch reporting on interval basis. */ - constructor({logger, timeout = defaultTimeout, batchInterval = defaultBatchInterval}) { + constructor({logger, timeout = defaultTimeout}) { this.logger = logger; this.timeout = timeout; this.partialSpans = new Map(); this[defaultTagsSymbol] = {}; - if (batchInterval === -1) { - return; - } - // read through the partials spans regularly // and collect any timed-out ones const timer = setInterval(() => { @@ -94,7 +82,7 @@ class BatchRecorder { this._writeSpan(id, span); } }); - }, batchInterval); + }, 1000); if (timer.unref) { // unref might not be available in browsers timer.unref(); // Allows Node to terminate instead of blocking on timer } @@ -147,6 +135,9 @@ class BatchRecorder { } } + /** + * Calling this will flush any pending spans to the transport. + */ flush() { this.partialSpans.forEach((span, id) => { this._writeSpan(id, span); From 989dffef84b3d6400713fc801ee080e51a389ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Tue, 23 Jul 2019 08:45:57 +0200 Subject: [PATCH 3/4] docs: improves documentation for flush method. Co-Authored-By: Adrian Cole --- packages/zipkin/src/batch-recorder.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/zipkin/src/batch-recorder.js b/packages/zipkin/src/batch-recorder.js index e118ed95..fa5a7fc5 100644 --- a/packages/zipkin/src/batch-recorder.js +++ b/packages/zipkin/src/batch-recorder.js @@ -137,6 +137,8 @@ class BatchRecorder { /** * Calling this will flush any pending spans to the transport. + * + * Note: the transport itself may be batching, in such case you may need to flush that also. */ flush() { this.partialSpans.forEach((span, id) => { From 458b8a49710a2ead2fc4ee45fcbd038dfb31a704 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Tue, 23 Jul 2019 10:01:24 +0200 Subject: [PATCH 4/4] tests(#362): removes lolex dependency for flush test. --- packages/zipkin/test/batch-recorder.test.js | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/packages/zipkin/test/batch-recorder.test.js b/packages/zipkin/test/batch-recorder.test.js index d2b5e6c6..944eff7c 100644 --- a/packages/zipkin/test/batch-recorder.test.js +++ b/packages/zipkin/test/batch-recorder.test.js @@ -305,7 +305,6 @@ describe('Batch Recorder - integration test', () => { }); it('should only flush spans when calling flush method', () => { - const clock = lolex.install(); recorder = new BatchRecorder({ logger: { logSpan: (span) => { @@ -314,11 +313,7 @@ describe('Batch Recorder - integration test', () => { } }); - recorder.record(record(rootId, now(), new Annotation.ServerRecv())); - - expect(spans).to.be.empty; // eslint-disable-line no-unused-expressions - - clock.tick(100); // 1000 is de batching interval + recorder.record(record(rootId, 1, new Annotation.ServerRecv())); expect(spans).to.be.empty; // eslint-disable-line no-unused-expressions @@ -327,10 +322,9 @@ describe('Batch Recorder - integration test', () => { expect(popSpan()).to.deep.equal({ traceId: rootId.traceId, id: rootId.spanId, - kind: 'SERVER' + kind: 'SERVER', + timestamp: 1 }); - - clock.uninstall(); }); it('should handle overlapping server and client', () => {