From 751562694b94ef8eb0b6a8262004da7a0a30a19b Mon Sep 17 00:00:00 2001 From: Bryan English Date: Wed, 11 Nov 2020 14:40:47 -0500 Subject: [PATCH 1/2] return a thenable from flush --- .../dd-trace/src/exporters/agent/writer.js | 26 ++++++++- .../test/exporters/agent/writer.spec.js | 56 ++++++++++--------- 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/packages/dd-trace/src/exporters/agent/writer.js b/packages/dd-trace/src/exporters/agent/writer.js index b8c3daae593..5337b422972 100644 --- a/packages/dd-trace/src/exporters/agent/writer.js +++ b/packages/dd-trace/src/exporters/agent/writer.js @@ -26,6 +26,15 @@ class Writer { _sendPayload (data, count) { platform.metrics().increment(`${METRIC_PREFIX}.requests`, true) + let done = () => {} + const thenable = { + then (fulfilled) { + // We're not ever going to reject, because users of the thenable are + // only concerned about completion, rather than success. + done = fulfilled + } + } + makeRequest(this._protocolVersion, data, count, this._url, this._lookup, true, (err, res, status) => { if (status) { platform.metrics().increment(`${METRIC_PREFIX}.responses`, true) @@ -41,7 +50,11 @@ class Writer { platform.startupLog.startupLog({ agentError: err }) - if (err) return log.error(err) + if (err) { + log.error(err) + done() + return + } log.debug(`Response from the agent: ${res}`) @@ -53,7 +66,10 @@ class Writer { platform.metrics().increment(`${METRIC_PREFIX}.errors`, true) platform.metrics().increment(`${METRIC_PREFIX}.errors.by.name`, `name:${e.name}`, true) } + done() }) + + return thenable } setUrl (url) { @@ -70,7 +86,13 @@ class Writer { if (count > 0) { const payload = this._encoderForVersion.makePayload() - this._sendPayload(payload, count) + return this._sendPayload(payload, count) + } else { + return { + then (fn) { + fn() + } + } } } } diff --git a/packages/dd-trace/test/exporters/agent/writer.spec.js b/packages/dd-trace/test/exporters/agent/writer.spec.js index 96fa45a6e06..61089c9424d 100644 --- a/packages/dd-trace/test/exporters/agent/writer.spec.js +++ b/packages/dd-trace/test/exporters/agent/writer.spec.js @@ -26,7 +26,7 @@ function describeWriter (protocolVersion) { name: sinon.stub(), version: sinon.stub(), engine: sinon.stub(), - request: sinon.stub().yields(null, response, 200), + request: sinon.stub().yieldsAsync(null, response, 200), msgpack: { prefix: sinon.stub().returns([Buffer.alloc(0)]) } @@ -107,7 +107,11 @@ function describeWriter (protocolVersion) { expect(encoder.makePayload).to.have.been.called }) - it('should flush its traces to the agent', () => { + it('should return a thenable when empty', (done) => { + writer.flush().then(done) + }) + + it('should flush its traces to the agent, and return a thenable', (done) => { platform.name.returns('lang') platform.version.returns('version') platform.engine.returns('interpreter') @@ -116,24 +120,25 @@ function describeWriter (protocolVersion) { encoder.count.returns(2) encoder.makePayload.returns([expectedData]) - writer.flush() - - expect(platform.request).to.have.been.calledWithMatch({ - protocol: url.protocol, - hostname: url.hostname, - port: url.port, - path: `/v${protocolVersion}/traces`, - method: 'PUT', - headers: { - 'Content-Type': 'application/msgpack', - 'Datadog-Meta-Lang': 'lang', - 'Datadog-Meta-Lang-Version': 'version', - 'Datadog-Meta-Lang-Interpreter': 'interpreter', - 'Datadog-Meta-Tracer-Version': 'tracerVersion', - 'X-Datadog-Trace-Count': '2' - }, - data: [expectedData], - lookup: undefined + writer.flush().then(() => { + expect(platform.request).to.have.been.calledWithMatch({ + protocol: url.protocol, + hostname: url.hostname, + port: url.port, + path: `/v${protocolVersion}/traces`, + method: 'PUT', + headers: { + 'Content-Type': 'application/msgpack', + 'Datadog-Meta-Lang': 'lang', + 'Datadog-Meta-Lang-Version': 'version', + 'Datadog-Meta-Lang-Interpreter': 'interpreter', + 'Datadog-Meta-Tracer-Version': 'tracerVersion', + 'X-Datadog-Trace-Count': '2' + }, + data: [expectedData], + lookup: undefined + }) + done() }) }) @@ -151,12 +156,13 @@ function describeWriter (protocolVersion) { }) }) - it('should update sampling rates', () => { + it('should update sampling rates', (done) => { encoder.count.returns(1) - writer.flush() - - expect(prioritySampler.update).to.have.been.calledWith({ - 'service:hello,env:test': 1 + writer.flush().then(() => { + expect(prioritySampler.update).to.have.been.calledWith({ + 'service:hello,env:test': 1 + }) + done() }) }) From b27ba9159b28e431b172bb098cf8cd658b5e667d Mon Sep 17 00:00:00 2001 From: Bryan English Date: Wed, 11 Nov 2020 15:03:41 -0500 Subject: [PATCH 2/2] switch from thenable to callback for flush --- .../dd-trace/src/exporters/agent/writer.js | 23 ++++--------------- .../test/exporters/agent/writer.spec.js | 10 ++++---- 2 files changed, 9 insertions(+), 24 deletions(-) diff --git a/packages/dd-trace/src/exporters/agent/writer.js b/packages/dd-trace/src/exporters/agent/writer.js index 5337b422972..f78380f2b43 100644 --- a/packages/dd-trace/src/exporters/agent/writer.js +++ b/packages/dd-trace/src/exporters/agent/writer.js @@ -23,18 +23,9 @@ class Writer { this._encode(spans) } - _sendPayload (data, count) { + _sendPayload (data, count, done) { platform.metrics().increment(`${METRIC_PREFIX}.requests`, true) - let done = () => {} - const thenable = { - then (fulfilled) { - // We're not ever going to reject, because users of the thenable are - // only concerned about completion, rather than success. - done = fulfilled - } - } - makeRequest(this._protocolVersion, data, count, this._url, this._lookup, true, (err, res, status) => { if (status) { platform.metrics().increment(`${METRIC_PREFIX}.responses`, true) @@ -68,8 +59,6 @@ class Writer { } done() }) - - return thenable } setUrl (url) { @@ -80,19 +69,15 @@ class Writer { this._encoderForVersion.encode(trace) } - flush () { + flush (done = () => {}) { const count = this._encoderForVersion.count() if (count > 0) { const payload = this._encoderForVersion.makePayload() - return this._sendPayload(payload, count) + this._sendPayload(payload, count, done) } else { - return { - then (fn) { - fn() - } - } + done() } } } diff --git a/packages/dd-trace/test/exporters/agent/writer.spec.js b/packages/dd-trace/test/exporters/agent/writer.spec.js index 61089c9424d..de06d37bac4 100644 --- a/packages/dd-trace/test/exporters/agent/writer.spec.js +++ b/packages/dd-trace/test/exporters/agent/writer.spec.js @@ -107,11 +107,11 @@ function describeWriter (protocolVersion) { expect(encoder.makePayload).to.have.been.called }) - it('should return a thenable when empty', (done) => { - writer.flush().then(done) + it('should call callback when empty', (done) => { + writer.flush(done) }) - it('should flush its traces to the agent, and return a thenable', (done) => { + it('should flush its traces to the agent, and call callback', (done) => { platform.name.returns('lang') platform.version.returns('version') platform.engine.returns('interpreter') @@ -120,7 +120,7 @@ function describeWriter (protocolVersion) { encoder.count.returns(2) encoder.makePayload.returns([expectedData]) - writer.flush().then(() => { + writer.flush(() => { expect(platform.request).to.have.been.calledWithMatch({ protocol: url.protocol, hostname: url.hostname, @@ -158,7 +158,7 @@ function describeWriter (protocolVersion) { it('should update sampling rates', (done) => { encoder.count.returns(1) - writer.flush().then(() => { + writer.flush(() => { expect(prioritySampler.update).to.have.been.calledWith({ 'service:hello,env:test': 1 })