From 740300087469af0c427205840be807594f8206d5 Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Tue, 22 Dec 2020 05:24:26 +0000 Subject: [PATCH 1/8] fix: remove fulfilled promises correctly --- .../src/CollectorExporterNodeBase.ts | 26 ++++++----------- .../src/CollectorExporterNodeBase.ts | 26 ++++++----------- .../browser/CollectorExporterBrowserBase.ts | 29 +++++++------------ .../node/CollectorExporterNodeBase.ts | 27 +++++++---------- .../src/zipkin.ts | 7 +++-- 5 files changed, 44 insertions(+), 71 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts index 9c7048c188..469dab3a4d 100644 --- a/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts @@ -55,25 +55,17 @@ export abstract class CollectorExporterNodeBase< onSuccess: () => void, onError: (error: collectorTypes.CollectorExporterError) => void ): void { - const promise = new Promise(resolve => { - const _onSuccess = (): void => { - onSuccess(); - _onFinish(); - }; - const _onError = (error: collectorTypes.CollectorExporterError): void => { - onError(error); - _onFinish(); - }; - const _onFinish = () => { - resolve(); - const index = this._sendingPromises.indexOf(promise); - this._sendingPromises.splice(index, 1); - }; - - this._send(this, objects, _onSuccess, _onError); - }); + const promise = new Promise((resolve, reject) => { + this._send(this, objects, resolve, reject); + }) + .then(onSuccess) + .catch(onError); this._sendingPromises.push(promise); + promise.finally(() => { + const index = this._sendingPromises.indexOf(promise); + this._sendingPromises.splice(index, 1); + }); } onInit(config: CollectorExporterConfigNode): void { diff --git a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts index 3a8bc2dfa6..d316c928d9 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts @@ -36,25 +36,17 @@ export abstract class CollectorExporterNodeBase< onSuccess: () => void, onError: (error: collectorTypes.CollectorExporterError) => void ): void { - const promise = new Promise(resolve => { - const _onSuccess = (): void => { - onSuccess(); - _onFinish(); - }; - const _onError = (error: collectorTypes.CollectorExporterError): void => { - onError(error); - _onFinish(); - }; - const _onFinish = () => { - resolve(); - const index = this._sendingPromises.indexOf(promise); - this._sendingPromises.splice(index, 1); - }; - - this._send(this, objects, _onSuccess, _onError); - }); + const promise = new Promise((resolve, reject) => { + this._send(this, objects, resolve, reject); + }) + .then(onSuccess) + .catch(onError); this._sendingPromises.push(promise); + promise.finally(() => { + const index = this._sendingPromises.indexOf(promise); + this._sendingPromises.splice(index, 1); + }); } onInit(config: CollectorExporterNodeConfigBase): void { diff --git a/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts b/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts index 00a4364127..a1e5cfdf82 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts @@ -69,27 +69,20 @@ export abstract class CollectorExporterBrowserBase< const serviceRequest = this.convert(items); const body = JSON.stringify(serviceRequest); - const promise = new Promise(resolve => { - const _onSuccess = (): void => { - onSuccess(); - _onFinish(); - }; - const _onError = (error: collectorTypes.CollectorExporterError): void => { - onError(error); - _onFinish(); - }; - const _onFinish = () => { - resolve(); - const index = this._sendingPromises.indexOf(promise); - this._sendingPromises.splice(index, 1); - }; - + const promise = new Promise((resolve, reject) => { if (this._useXHR) { - sendWithXhr(body, this.url, this._headers, _onSuccess, _onError); + sendWithXhr(body, this.url, this._headers, resolve, reject); } else { - sendWithBeacon(body, this.url, _onSuccess, _onError); + sendWithBeacon(body, this.url, resolve, reject); } - }); + }) + .then(onSuccess) + .catch(onError); + this._sendingPromises.push(promise); + promise.finally(() => { + const index = this._sendingPromises.indexOf(promise); + this._sendingPromises.splice(index, 1); + }); } } diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts index a57e362d0d..d93f941c02 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts @@ -62,30 +62,23 @@ export abstract class CollectorExporterNodeBase< } const serviceRequest = this.convert(objects); - const promise = new Promise(resolve => { - const _onSuccess = (): void => { - onSuccess(); - _onFinish(); - }; - const _onError = (error: collectorTypes.CollectorExporterError): void => { - onError(error); - _onFinish(); - }; - const _onFinish = () => { - resolve(); - const index = this._sendingPromises.indexOf(promise); - this._sendingPromises.splice(index, 1); - }; + const promise = new Promise((resolve, reject) => { sendWithHttp( this, JSON.stringify(serviceRequest), 'application/json', - _onSuccess, - _onError + resolve, + reject ); - }); + }) + .then(onSuccess) + .catch(onError); this._sendingPromises.push(promise); + promise.finally(() => { + const index = this._sendingPromises.indexOf(promise); + this._sendingPromises.splice(index, 1); + }); } onShutdown(): void {} diff --git a/packages/opentelemetry-exporter-zipkin/src/zipkin.ts b/packages/opentelemetry-exporter-zipkin/src/zipkin.ts index 0f6e84ebc1..b69bc55a70 100644 --- a/packages/opentelemetry-exporter-zipkin/src/zipkin.ts +++ b/packages/opentelemetry-exporter-zipkin/src/zipkin.ts @@ -76,11 +76,14 @@ export class ZipkinExporter implements SpanExporter { this._sendSpans(spans, this._serviceName!, result => { resolve(); resultCallback(result); - const index = this._sendingPromises.indexOf(promise); - this._sendingPromises.splice(index, 1); }); }); + this._sendingPromises.push(promise); + promise.finally(() => { + const index = this._sendingPromises.indexOf(promise); + this._sendingPromises.splice(index, 1); + }); } /** From 915156f0a844460adb27ba5cc93dcf82d91ba609 Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Sat, 27 Feb 2021 00:06:58 +0000 Subject: [PATCH 2/8] chore: add unit test for CollectorExporterNodeBase --- .../test/CollectorExporterNodeBase.test.ts | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts diff --git a/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts new file mode 100644 index 0000000000..f878c7e3b6 --- /dev/null +++ b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts @@ -0,0 +1,153 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { collectorTypes } from '@opentelemetry/exporter-collector'; +import { ReadableSpan } from '@opentelemetry/tracing'; + +import * as assert from 'assert'; +import { CollectorExporterNodeBase } from '../src/CollectorExporterNodeBase'; +import { CollectorExporterConfigNode, ServiceClientType } from '../src/types'; +import { mockedReadableSpan } from './helper'; + +class MockCollectorExporter extends CollectorExporterNodeBase< + ReadableSpan, + ReadableSpan[] +> { + /** + * Callbacks passed to _send() + */ + sendCallbacks: { + onSuccess: () => void; + onError: (error: collectorTypes.CollectorExporterError) => void; + }[] = []; + + getDefaultUrl(config: CollectorExporterConfigNode): string { + return ''; + } + + getDefaultServiceName(config: CollectorExporterConfigNode): string { + return ''; + } + + convert(spans: ReadableSpan[]): ReadableSpan[] { + return spans; + } + + getServiceClientType() { + return ServiceClientType.SPANS; + } + + getServiceProtoPath(): string { + return 'opentelemetry/proto/collector/trace/v1/trace_service.proto'; + } +} + +// Mocked _send which just saves the callbacks for later +MockCollectorExporter.prototype['_send'] = function _sendMock( + self: MockCollectorExporter, + objects: ReadableSpan[], + onSuccess: () => void, + onError: (error: collectorTypes.CollectorExporterError) => void +): void { + self.sendCallbacks.push({ onSuccess, onError }); +}; + +describe('CollectorExporterNodeBase', () => { + let exporter: MockCollectorExporter; + const concurrencyLimit = 5; + + beforeEach(done => { + exporter = new MockCollectorExporter({ concurrencyLimit }); + done(); + }); + + describe('export', () => { + it('should export requests concurrently', async () => { + const spans = [Object.assign({}, mockedReadableSpan)]; + const numToExport = concurrencyLimit; + + for (let i = 0; i < numToExport; ++i) { + exporter.export(spans, () => {}); + } + + assert.strictEqual(exporter['_sendingPromises'].length, numToExport); + const promisesAllDone = Promise.all(exporter['_sendingPromises']); + // Mock that all requests finish sending + exporter.sendCallbacks.forEach(({ onSuccess }) => onSuccess()); + + // All finished promises should be dropped off + await promisesAllDone; + assert.strictEqual(exporter['_sendingPromises'].length, 0); + }); + + it('should drop export requests when already sending concurrencyLimit', async () => { + const spans = [Object.assign({}, mockedReadableSpan)]; + const numToExport = concurrencyLimit + 5; + + for (let i = 0; i < numToExport; ++i) { + exporter.export(spans, () => {}); + } + + assert.strictEqual(exporter['_sendingPromises'].length, concurrencyLimit); + const promisesAllDone = Promise.all(exporter['_sendingPromises']); + // Mock that all requests finish sending + exporter.sendCallbacks.forEach(({ onSuccess }) => onSuccess()); + + // All finished promises should be dropped off + await promisesAllDone; + assert.strictEqual(exporter['_sendingPromises'].length, 0); + }); + + it('should drop export requests even if they failed', async () => { + const spans = [Object.assign({}, mockedReadableSpan)]; + + exporter.export(spans, () => {}); + assert.strictEqual(exporter['_sendingPromises'].length, 1); + const promisesAllDone = Promise.all(exporter['_sendingPromises']); + // Mock that all requests fail sending + exporter.sendCallbacks.forEach(({ onError }) => + onError(new Error('Failed to send!!')) + ); + + // All finished promises should be dropped off + await promisesAllDone; + assert.strictEqual(exporter['_sendingPromises'].length, 0); + }); + + it('should drop export requests even if success callback throws error', async () => { + const spans = [Object.assign({}, mockedReadableSpan)]; + + exporter['_sendPromise']( + spans, + () => { + throw new Error('Oops'); + }, + () => {} + ); + + assert.strictEqual(exporter['_sendingPromises'].length, 1); + const promisesAllDone = Promise.all(exporter['_sendingPromises']); + // Mock that the request finishes sending + exporter.sendCallbacks.forEach(({ onSuccess }) => { + onSuccess(); + }); + + // All finished promises should be dropped off + await promisesAllDone; + assert.strictEqual(exporter['_sendingPromises'].length, 0); + }); + }); +}); From bb0e43622f1fe866fe217d35cbec140e50a1f665 Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Wed, 16 Jun 2021 17:52:11 +0000 Subject: [PATCH 3/8] chore: reword test descriptions --- .../test/CollectorExporterNodeBase.test.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts index f878c7e3b6..b9d920b62f 100644 --- a/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts +++ b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts @@ -88,12 +88,12 @@ describe('CollectorExporterNodeBase', () => { // Mock that all requests finish sending exporter.sendCallbacks.forEach(({ onSuccess }) => onSuccess()); - // All finished promises should be dropped off + // All finished promises should be popped off await promisesAllDone; assert.strictEqual(exporter['_sendingPromises'].length, 0); }); - it('should drop export requests when already sending concurrencyLimit', async () => { + it('should drop new export requests when already sending at concurrencyLimit', async () => { const spans = [Object.assign({}, mockedReadableSpan)]; const numToExport = concurrencyLimit + 5; @@ -106,12 +106,12 @@ describe('CollectorExporterNodeBase', () => { // Mock that all requests finish sending exporter.sendCallbacks.forEach(({ onSuccess }) => onSuccess()); - // All finished promises should be dropped off + // All finished promises should be popped off await promisesAllDone; assert.strictEqual(exporter['_sendingPromises'].length, 0); }); - it('should drop export requests even if they failed', async () => { + it('should pop export request promises even if they failed', async () => { const spans = [Object.assign({}, mockedReadableSpan)]; exporter.export(spans, () => {}); @@ -122,12 +122,12 @@ describe('CollectorExporterNodeBase', () => { onError(new Error('Failed to send!!')) ); - // All finished promises should be dropped off + // All finished promises should be popped off await promisesAllDone; assert.strictEqual(exporter['_sendingPromises'].length, 0); }); - it('should drop export requests even if success callback throws error', async () => { + it('should pop export request promises even if success callback throws error', async () => { const spans = [Object.assign({}, mockedReadableSpan)]; exporter['_sendPromise']( @@ -145,7 +145,7 @@ describe('CollectorExporterNodeBase', () => { onSuccess(); }); - // All finished promises should be dropped off + // All finished promises should be popped off await promisesAllDone; assert.strictEqual(exporter['_sendingPromises'].length, 0); }); From 6072ca12b9952550f87ebe6ffe511c5ea345a61e Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Fri, 25 Jun 2021 11:25:08 -0400 Subject: [PATCH 4/8] Apply suggestions from code review Co-authored-by: Daniel Dyla --- .../src/CollectorExporterNodeBase.ts | 3 +-- .../src/CollectorExporterNodeBase.ts | 3 +-- .../src/platform/browser/CollectorExporterBrowserBase.ts | 3 +-- .../src/platform/node/CollectorExporterNodeBase.ts | 3 +-- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts index 5368e1f882..dec79008fc 100644 --- a/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts @@ -58,8 +58,7 @@ export abstract class CollectorExporterNodeBase< const promise = new Promise((resolve, reject) => { this._send(this, objects, resolve, reject); }) - .then(onSuccess) - .catch(onError); + .then(onSuccess, onError); this._sendingPromises.push(promise); promise.finally(() => { diff --git a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts index 30e85b588c..eede81daab 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts @@ -39,8 +39,7 @@ export abstract class CollectorExporterNodeBase< const promise = new Promise((resolve, reject) => { this._send(this, objects, resolve, reject); }) - .then(onSuccess) - .catch(onError); + .then(onSuccess, onError); this._sendingPromises.push(promise); promise.finally(() => { diff --git a/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts b/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts index 042dc46f8f..2448fe322e 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts @@ -83,8 +83,7 @@ export abstract class CollectorExporterBrowserBase< sendWithBeacon(body, this.url, resolve, reject); } }) - .then(onSuccess) - .catch(onError); + .then(onSuccess, onError); this._sendingPromises.push(promise); promise.finally(() => { diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts index 778a7c90b9..1b0a0517d9 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts @@ -77,8 +77,7 @@ export abstract class CollectorExporterNodeBase< reject ); }) - .then(onSuccess) - .catch(onError); + .then(onSuccess, onError); this._sendingPromises.push(promise); promise.finally(() => { From 8084b2c9d3bc2564d1d19f42c2f626c5c1efd18e Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Wed, 21 Jul 2021 19:12:43 +0000 Subject: [PATCH 5/8] chore: add catch in test case --- .../test/CollectorExporterNodeBase.test.ts | 67 ++++++++++--------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts index b9d920b62f..79e07361bf 100644 --- a/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts +++ b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts @@ -14,13 +14,13 @@ * limitations under the License. */ -import { collectorTypes } from '@opentelemetry/exporter-collector'; -import { ReadableSpan } from '@opentelemetry/tracing'; +import { collectorTypes } from "@opentelemetry/exporter-collector"; +import { ReadableSpan } from "@opentelemetry/tracing"; -import * as assert from 'assert'; -import { CollectorExporterNodeBase } from '../src/CollectorExporterNodeBase'; -import { CollectorExporterConfigNode, ServiceClientType } from '../src/types'; -import { mockedReadableSpan } from './helper'; +import * as assert from "assert"; +import { CollectorExporterNodeBase } from "../src/CollectorExporterNodeBase"; +import { CollectorExporterConfigNode, ServiceClientType } from "../src/types"; +import { mockedReadableSpan } from "./helper"; class MockCollectorExporter extends CollectorExporterNodeBase< ReadableSpan, @@ -35,11 +35,11 @@ class MockCollectorExporter extends CollectorExporterNodeBase< }[] = []; getDefaultUrl(config: CollectorExporterConfigNode): string { - return ''; + return ""; } getDefaultServiceName(config: CollectorExporterConfigNode): string { - return ''; + return ""; } convert(spans: ReadableSpan[]): ReadableSpan[] { @@ -51,12 +51,12 @@ class MockCollectorExporter extends CollectorExporterNodeBase< } getServiceProtoPath(): string { - return 'opentelemetry/proto/collector/trace/v1/trace_service.proto'; + return "opentelemetry/proto/collector/trace/v1/trace_service.proto"; } } // Mocked _send which just saves the callbacks for later -MockCollectorExporter.prototype['_send'] = function _sendMock( +MockCollectorExporter.prototype["_send"] = function _sendMock( self: MockCollectorExporter, objects: ReadableSpan[], onSuccess: () => void, @@ -65,17 +65,17 @@ MockCollectorExporter.prototype['_send'] = function _sendMock( self.sendCallbacks.push({ onSuccess, onError }); }; -describe('CollectorExporterNodeBase', () => { +describe("CollectorExporterNodeBase", () => { let exporter: MockCollectorExporter; const concurrencyLimit = 5; - beforeEach(done => { + beforeEach((done) => { exporter = new MockCollectorExporter({ concurrencyLimit }); done(); }); - describe('export', () => { - it('should export requests concurrently', async () => { + describe("export", () => { + it("should export requests concurrently", async () => { const spans = [Object.assign({}, mockedReadableSpan)]; const numToExport = concurrencyLimit; @@ -83,17 +83,17 @@ describe('CollectorExporterNodeBase', () => { exporter.export(spans, () => {}); } - assert.strictEqual(exporter['_sendingPromises'].length, numToExport); - const promisesAllDone = Promise.all(exporter['_sendingPromises']); + assert.strictEqual(exporter["_sendingPromises"].length, numToExport); + const promisesAllDone = Promise.all(exporter["_sendingPromises"]); // Mock that all requests finish sending exporter.sendCallbacks.forEach(({ onSuccess }) => onSuccess()); // All finished promises should be popped off await promisesAllDone; - assert.strictEqual(exporter['_sendingPromises'].length, 0); + assert.strictEqual(exporter["_sendingPromises"].length, 0); }); - it('should drop new export requests when already sending at concurrencyLimit', async () => { + it("should drop new export requests when already sending at concurrencyLimit", async () => { const spans = [Object.assign({}, mockedReadableSpan)]; const numToExport = concurrencyLimit + 5; @@ -101,45 +101,48 @@ describe('CollectorExporterNodeBase', () => { exporter.export(spans, () => {}); } - assert.strictEqual(exporter['_sendingPromises'].length, concurrencyLimit); - const promisesAllDone = Promise.all(exporter['_sendingPromises']); + assert.strictEqual(exporter["_sendingPromises"].length, concurrencyLimit); + const promisesAllDone = Promise.all(exporter["_sendingPromises"]); // Mock that all requests finish sending exporter.sendCallbacks.forEach(({ onSuccess }) => onSuccess()); // All finished promises should be popped off await promisesAllDone; - assert.strictEqual(exporter['_sendingPromises'].length, 0); + assert.strictEqual(exporter["_sendingPromises"].length, 0); }); - it('should pop export request promises even if they failed', async () => { + it("should pop export request promises even if they failed", async () => { const spans = [Object.assign({}, mockedReadableSpan)]; exporter.export(spans, () => {}); - assert.strictEqual(exporter['_sendingPromises'].length, 1); - const promisesAllDone = Promise.all(exporter['_sendingPromises']); + assert.strictEqual(exporter["_sendingPromises"].length, 1); + const promisesAllDone = Promise.all(exporter["_sendingPromises"]); // Mock that all requests fail sending exporter.sendCallbacks.forEach(({ onError }) => - onError(new Error('Failed to send!!')) + onError(new Error("Failed to send!!")) ); // All finished promises should be popped off await promisesAllDone; - assert.strictEqual(exporter['_sendingPromises'].length, 0); + assert.strictEqual(exporter["_sendingPromises"].length, 0); }); - it('should pop export request promises even if success callback throws error', async () => { + it("should pop export request promises even if success callback throws error", async () => { const spans = [Object.assign({}, mockedReadableSpan)]; - exporter['_sendPromise']( + exporter["_sendPromise"]( spans, () => { - throw new Error('Oops'); + throw new Error("Oops"); }, () => {} ); - assert.strictEqual(exporter['_sendingPromises'].length, 1); - const promisesAllDone = Promise.all(exporter['_sendingPromises']); + assert.strictEqual(exporter["_sendingPromises"].length, 1); + const promisesAllDone = Promise.all(exporter["_sendingPromises"]) + // catch expected unhandled exception + .catch(() => {}); + // Mock that the request finishes sending exporter.sendCallbacks.forEach(({ onSuccess }) => { onSuccess(); @@ -147,7 +150,7 @@ describe('CollectorExporterNodeBase', () => { // All finished promises should be popped off await promisesAllDone; - assert.strictEqual(exporter['_sendingPromises'].length, 0); + assert.strictEqual(exporter["_sendingPromises"].length, 0); }); }); }); From c830c20c406b83dab323100d78883b6928d2d170 Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Mon, 9 Aug 2021 19:59:27 +0000 Subject: [PATCH 6/8] chore: use new sdk name --- .../test/CollectorExporterNodeBase.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts index 79e07361bf..a50c01a6fb 100644 --- a/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts +++ b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts @@ -15,7 +15,7 @@ */ import { collectorTypes } from "@opentelemetry/exporter-collector"; -import { ReadableSpan } from "@opentelemetry/tracing"; +import { ReadableSpan } from "@opentelemetry/sdk-trace-base"; import * as assert from "assert"; import { CollectorExporterNodeBase } from "../src/CollectorExporterNodeBase"; From 3945af07a5feffe3844e3d409444787a831af941 Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Mon, 9 Aug 2021 20:44:23 +0000 Subject: [PATCH 7/8] chore: ran lint:fix --- .../test/CollectorExporterNodeBase.test.ts | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts index a50c01a6fb..84042f6ed5 100644 --- a/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts +++ b/packages/opentelemetry-exporter-collector-grpc/test/CollectorExporterNodeBase.test.ts @@ -14,13 +14,13 @@ * limitations under the License. */ -import { collectorTypes } from "@opentelemetry/exporter-collector"; -import { ReadableSpan } from "@opentelemetry/sdk-trace-base"; +import { collectorTypes } from '@opentelemetry/exporter-collector'; +import { ReadableSpan } from '@opentelemetry/sdk-trace-base'; -import * as assert from "assert"; -import { CollectorExporterNodeBase } from "../src/CollectorExporterNodeBase"; -import { CollectorExporterConfigNode, ServiceClientType } from "../src/types"; -import { mockedReadableSpan } from "./helper"; +import * as assert from 'assert'; +import { CollectorExporterNodeBase } from '../src/CollectorExporterNodeBase'; +import { CollectorExporterConfigNode, ServiceClientType } from '../src/types'; +import { mockedReadableSpan } from './helper'; class MockCollectorExporter extends CollectorExporterNodeBase< ReadableSpan, @@ -35,11 +35,11 @@ class MockCollectorExporter extends CollectorExporterNodeBase< }[] = []; getDefaultUrl(config: CollectorExporterConfigNode): string { - return ""; + return ''; } getDefaultServiceName(config: CollectorExporterConfigNode): string { - return ""; + return ''; } convert(spans: ReadableSpan[]): ReadableSpan[] { @@ -51,12 +51,12 @@ class MockCollectorExporter extends CollectorExporterNodeBase< } getServiceProtoPath(): string { - return "opentelemetry/proto/collector/trace/v1/trace_service.proto"; + return 'opentelemetry/proto/collector/trace/v1/trace_service.proto'; } } // Mocked _send which just saves the callbacks for later -MockCollectorExporter.prototype["_send"] = function _sendMock( +MockCollectorExporter.prototype['_send'] = function _sendMock( self: MockCollectorExporter, objects: ReadableSpan[], onSuccess: () => void, @@ -65,17 +65,17 @@ MockCollectorExporter.prototype["_send"] = function _sendMock( self.sendCallbacks.push({ onSuccess, onError }); }; -describe("CollectorExporterNodeBase", () => { +describe('CollectorExporterNodeBase', () => { let exporter: MockCollectorExporter; const concurrencyLimit = 5; - beforeEach((done) => { + beforeEach(done => { exporter = new MockCollectorExporter({ concurrencyLimit }); done(); }); - describe("export", () => { - it("should export requests concurrently", async () => { + describe('export', () => { + it('should export requests concurrently', async () => { const spans = [Object.assign({}, mockedReadableSpan)]; const numToExport = concurrencyLimit; @@ -83,17 +83,17 @@ describe("CollectorExporterNodeBase", () => { exporter.export(spans, () => {}); } - assert.strictEqual(exporter["_sendingPromises"].length, numToExport); - const promisesAllDone = Promise.all(exporter["_sendingPromises"]); + assert.strictEqual(exporter['_sendingPromises'].length, numToExport); + const promisesAllDone = Promise.all(exporter['_sendingPromises']); // Mock that all requests finish sending exporter.sendCallbacks.forEach(({ onSuccess }) => onSuccess()); // All finished promises should be popped off await promisesAllDone; - assert.strictEqual(exporter["_sendingPromises"].length, 0); + assert.strictEqual(exporter['_sendingPromises'].length, 0); }); - it("should drop new export requests when already sending at concurrencyLimit", async () => { + it('should drop new export requests when already sending at concurrencyLimit', async () => { const spans = [Object.assign({}, mockedReadableSpan)]; const numToExport = concurrencyLimit + 5; @@ -101,45 +101,45 @@ describe("CollectorExporterNodeBase", () => { exporter.export(spans, () => {}); } - assert.strictEqual(exporter["_sendingPromises"].length, concurrencyLimit); - const promisesAllDone = Promise.all(exporter["_sendingPromises"]); + assert.strictEqual(exporter['_sendingPromises'].length, concurrencyLimit); + const promisesAllDone = Promise.all(exporter['_sendingPromises']); // Mock that all requests finish sending exporter.sendCallbacks.forEach(({ onSuccess }) => onSuccess()); // All finished promises should be popped off await promisesAllDone; - assert.strictEqual(exporter["_sendingPromises"].length, 0); + assert.strictEqual(exporter['_sendingPromises'].length, 0); }); - it("should pop export request promises even if they failed", async () => { + it('should pop export request promises even if they failed', async () => { const spans = [Object.assign({}, mockedReadableSpan)]; exporter.export(spans, () => {}); - assert.strictEqual(exporter["_sendingPromises"].length, 1); - const promisesAllDone = Promise.all(exporter["_sendingPromises"]); + assert.strictEqual(exporter['_sendingPromises'].length, 1); + const promisesAllDone = Promise.all(exporter['_sendingPromises']); // Mock that all requests fail sending exporter.sendCallbacks.forEach(({ onError }) => - onError(new Error("Failed to send!!")) + onError(new Error('Failed to send!!')) ); // All finished promises should be popped off await promisesAllDone; - assert.strictEqual(exporter["_sendingPromises"].length, 0); + assert.strictEqual(exporter['_sendingPromises'].length, 0); }); - it("should pop export request promises even if success callback throws error", async () => { + it('should pop export request promises even if success callback throws error', async () => { const spans = [Object.assign({}, mockedReadableSpan)]; - exporter["_sendPromise"]( + exporter['_sendPromise']( spans, () => { - throw new Error("Oops"); + throw new Error('Oops'); }, () => {} ); - assert.strictEqual(exporter["_sendingPromises"].length, 1); - const promisesAllDone = Promise.all(exporter["_sendingPromises"]) + assert.strictEqual(exporter['_sendingPromises'].length, 1); + const promisesAllDone = Promise.all(exporter['_sendingPromises']) // catch expected unhandled exception .catch(() => {}); @@ -150,7 +150,7 @@ describe("CollectorExporterNodeBase", () => { // All finished promises should be popped off await promisesAllDone; - assert.strictEqual(exporter["_sendingPromises"].length, 0); + assert.strictEqual(exporter['_sendingPromises'].length, 0); }); }); }); From 9ba634e3edaa173c71e2d44a9d224ac0ee02b9a7 Mon Sep 17 00:00:00 2001 From: Aaron Abbott Date: Mon, 9 Aug 2021 22:52:09 +0000 Subject: [PATCH 8/8] chore: replace Promise.finally with two param .then --- .../src/CollectorExporterNodeBase.ts | 5 +++-- .../src/CollectorExporterNodeBase.ts | 5 +++-- .../src/platform/browser/CollectorExporterBrowserBase.ts | 5 +++-- .../src/platform/node/CollectorExporterNodeBase.ts | 5 +++-- packages/opentelemetry-exporter-zipkin/src/zipkin.ts | 6 ++++-- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts index d27bf879f5..da9a1b9d76 100644 --- a/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts @@ -67,10 +67,11 @@ export abstract class CollectorExporterNodeBase< .then(onSuccess, onError); this._sendingPromises.push(promise); - promise.finally(() => { + const popPromise = () => { const index = this._sendingPromises.indexOf(promise); this._sendingPromises.splice(index, 1); - }); + } + promise.then(popPromise, popPromise); } onInit(config: CollectorExporterConfigNode): void { diff --git a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts index fcd2c7c94c..9ff36306b7 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts @@ -53,10 +53,11 @@ export abstract class CollectorExporterNodeBase< .then(onSuccess, onError); this._sendingPromises.push(promise); - promise.finally(() => { + const popPromise = () => { const index = this._sendingPromises.indexOf(promise); this._sendingPromises.splice(index, 1); - }); + } + promise.then(popPromise, popPromise); } override onInit(config: CollectorExporterNodeConfigBase): void { diff --git a/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts b/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts index 2448fe322e..57f581161a 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporterBrowserBase.ts @@ -86,9 +86,10 @@ export abstract class CollectorExporterBrowserBase< .then(onSuccess, onError); this._sendingPromises.push(promise); - promise.finally(() => { + const popPromise = () => { const index = this._sendingPromises.indexOf(promise); this._sendingPromises.splice(index, 1); - }); + } + promise.then(popPromise, popPromise); } } diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts index 4b98681e94..a60993e84c 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts @@ -82,10 +82,11 @@ export abstract class CollectorExporterNodeBase< .then(onSuccess, onError); this._sendingPromises.push(promise); - promise.finally(() => { + const popPromise = () => { const index = this._sendingPromises.indexOf(promise); this._sendingPromises.splice(index, 1); - }); + } + promise.then(popPromise, popPromise); } onShutdown(): void {} diff --git a/packages/opentelemetry-exporter-zipkin/src/zipkin.ts b/packages/opentelemetry-exporter-zipkin/src/zipkin.ts index f57ea08061..557c0faef3 100644 --- a/packages/opentelemetry-exporter-zipkin/src/zipkin.ts +++ b/packages/opentelemetry-exporter-zipkin/src/zipkin.ts @@ -87,11 +87,13 @@ export class ZipkinExporter implements SpanExporter { }); }); + this._sendingPromises.push(promise); - promise.finally(() => { + const popPromise = () => { const index = this._sendingPromises.indexOf(promise); this._sendingPromises.splice(index, 1); - }); + } + promise.then(popPromise, popPromise); } /**