From 3146dbe42c018b12a2f2c88a286d0a23175adca2 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Sat, 7 Nov 2020 21:01:38 +0530 Subject: [PATCH 1/5] chore: use http keep-alive --- .../src/platform/node/CollectorExporterNodeBase.ts | 4 ++++ .../src/platform/node/util.ts | 8 +++++++- packages/opentelemetry-exporter-collector/src/types.ts | 1 + 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts index c086210850..4237a89714 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts @@ -33,6 +33,7 @@ export abstract class CollectorExporterNodeBase< > { DEFAULT_HEADERS: Record = {}; headers: Record; + keepAlive: boolean = false; constructor(config: CollectorExporterConfigBase = {}) { super(config); if ((config as any).metadata) { @@ -40,6 +41,9 @@ export abstract class CollectorExporterNodeBase< } this.headers = parseHeaders(config.headers, this.logger) || this.DEFAULT_HEADERS; + if (config.connectionReuse) { + this.keepAlive = true; + } } onInit(_config: CollectorExporterConfigBase): void { diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts index 0e20cfc8ce..a1b7496390 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts @@ -36,7 +36,7 @@ export function sendWithHttp( ): void { const parsedUrl = new url.URL(collector.url); - const options = { + const options: http.RequestOptions | https.RequestOptions = { hostname: parsedUrl.hostname, port: parsedUrl.port, path: parsedUrl.pathname, @@ -49,6 +49,12 @@ export function sendWithHttp( }; const request = parsedUrl.protocol === 'http:' ? http.request : https.request; + const Agent = parsedUrl.protocol === 'http:' ? http.Agent : https.Agent; + if (collector.keepAlive) { + const keepAliveAgent = new Agent({ keepAlive: true }); + options.agent = keepAliveAgent; + } + const req = request(options, (res: http.IncomingMessage) => { if (res.statusCode && res.statusCode < 299) { collector.logger.debug(`statusCode: ${res.statusCode}`); diff --git a/packages/opentelemetry-exporter-collector/src/types.ts b/packages/opentelemetry-exporter-collector/src/types.ts index ba8c474caf..7137d6222b 100644 --- a/packages/opentelemetry-exporter-collector/src/types.ts +++ b/packages/opentelemetry-exporter-collector/src/types.ts @@ -339,6 +339,7 @@ export interface CollectorExporterConfigBase { serviceName?: string; attributes?: Attributes; url?: string; + connectionReuse?: boolean; } /** From a255969e14494df963259c1d42ae4dad7fedf872 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Tue, 10 Nov 2020 09:28:57 +0530 Subject: [PATCH 2/5] chore: rename connectionReuse -> keepAlive --- .../src/platform/node/CollectorExporterNodeBase.ts | 2 +- packages/opentelemetry-exporter-collector/src/types.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts index 4237a89714..1a8a04152a 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts @@ -41,7 +41,7 @@ export abstract class CollectorExporterNodeBase< } this.headers = parseHeaders(config.headers, this.logger) || this.DEFAULT_HEADERS; - if (config.connectionReuse) { + if (config.keepAlive) { this.keepAlive = true; } } diff --git a/packages/opentelemetry-exporter-collector/src/types.ts b/packages/opentelemetry-exporter-collector/src/types.ts index 7137d6222b..9db9876900 100644 --- a/packages/opentelemetry-exporter-collector/src/types.ts +++ b/packages/opentelemetry-exporter-collector/src/types.ts @@ -339,7 +339,7 @@ export interface CollectorExporterConfigBase { serviceName?: string; attributes?: Attributes; url?: string; - connectionReuse?: boolean; + keepAlive?: boolean; } /** From e1ef2da55d03231f20dd59d9a00f3da5759b59fc Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Wed, 18 Nov 2020 21:49:28 +0530 Subject: [PATCH 3/5] chore: update proto and node to support keep alive and http agent options --- .../src/CollectorExporterNodeBase.ts | 2 +- .../src/CollectorMetricExporter.ts | 6 ++++-- .../src/CollectorTraceExporter.ts | 6 ++++-- .../src/util.ts | 2 +- .../test/CollectorMetricExporter.test.ts | 17 ++++++++++++++- .../test/CollectorTraceExporter.test.ts | 17 ++++++++++++++- .../node/CollectorExporterNodeBase.ts | 21 ++++++++++++------- .../platform/node/CollectorMetricExporter.ts | 9 +++++--- .../platform/node/CollectorTraceExporter.ts | 9 +++++--- .../src/platform/node/util.ts | 6 ++++-- .../src/types.ts | 10 +++++++++ .../test/node/CollectorMetricExporter.test.ts | 18 ++++++++++++++-- .../test/node/CollectorTraceExporter.test.ts | 18 ++++++++++++++-- 13 files changed, 114 insertions(+), 27 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts index ea6701ae78..5d3894875f 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts @@ -55,7 +55,7 @@ export abstract class CollectorExporterNodeBase< this._sendingPromises.push(promise); } - onInit(config: collectorTypes.CollectorExporterConfigBase): void { + onInit(config: collectorTypes.CollectorExporterNodeConfigBase): void { this._isShutdown = false; // defer to next tick and lazy load to avoid loading protobufjs too early // and making this impossible to be instrumented diff --git a/packages/opentelemetry-exporter-collector-proto/src/CollectorMetricExporter.ts b/packages/opentelemetry-exporter-collector-proto/src/CollectorMetricExporter.ts index d10611b3a9..d400f84570 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/CollectorMetricExporter.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/CollectorMetricExporter.ts @@ -47,7 +47,9 @@ export class CollectorMetricExporter ); } - getDefaultUrl(config: collectorTypes.CollectorExporterConfigBase): string { + getDefaultUrl( + config: collectorTypes.CollectorExporterNodeConfigBase + ): string { if (!config.url) { return DEFAULT_COLLECTOR_URL; } @@ -55,7 +57,7 @@ export class CollectorMetricExporter } getDefaultServiceName( - config: collectorTypes.CollectorExporterConfigBase + config: collectorTypes.CollectorExporterNodeConfigBase ): string { return config.serviceName || DEFAULT_SERVICE_NAME; } diff --git a/packages/opentelemetry-exporter-collector-proto/src/CollectorTraceExporter.ts b/packages/opentelemetry-exporter-collector-proto/src/CollectorTraceExporter.ts index 79ca82049c..9aa0a6a922 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/CollectorTraceExporter.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/CollectorTraceExporter.ts @@ -40,7 +40,9 @@ export class CollectorTraceExporter return toCollectorExportTraceServiceRequest(spans, this); } - getDefaultUrl(config: collectorTypes.CollectorExporterConfigBase): string { + getDefaultUrl( + config: collectorTypes.CollectorExporterNodeConfigBase + ): string { if (!config.url) { return DEFAULT_COLLECTOR_URL; } @@ -48,7 +50,7 @@ export class CollectorTraceExporter } getDefaultServiceName( - config: collectorTypes.CollectorExporterConfigBase + config: collectorTypes.CollectorExporterNodeConfigBase ): string { return config.serviceName || DEFAULT_SERVICE_NAME; } diff --git a/packages/opentelemetry-exporter-collector-proto/src/util.ts b/packages/opentelemetry-exporter-collector-proto/src/util.ts index 05e4ec00fc..499aaf4485 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/util.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/util.ts @@ -33,7 +33,7 @@ export function getExportRequestProto(): Type | undefined { export function onInit( collector: CollectorExporterNodeBase, - _config: collectorTypes.CollectorExporterConfigBase + _config: collectorTypes.CollectorExporterNodeConfigBase ): void { const dir = path.resolve(__dirname, '..', 'protos'); const root = new protobufjs.Root(); diff --git a/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts index 621ba2d1b9..d0cc6f8f88 100644 --- a/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts +++ b/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts @@ -53,7 +53,7 @@ const waitTimeMS = 20; describe('CollectorMetricExporter - node with proto over http', () => { let collectorExporter: CollectorMetricExporter; - let collectorExporterConfig: collectorTypes.CollectorExporterConfigBase; + let collectorExporterConfig: collectorTypes.CollectorExporterNodeConfigBase; let spyRequest: sinon.SinonSpy; let spyWrite: sinon.SinonSpy; let metrics: MetricRecord[]; @@ -70,6 +70,8 @@ describe('CollectorMetricExporter - node with proto over http', () => { serviceName: 'bar', attributes: {}, url: 'http://foo.bar.com', + keepAlive: true, + httpAgentOptions: { keepAliveMsecs: 2000 }, }; collectorExporter = new CollectorMetricExporter(collectorExporterConfig); // Overwrites the start time to make tests consistent @@ -116,6 +118,19 @@ describe('CollectorMetricExporter - node with proto over http', () => { }, waitTimeMS); }); + it('should have keep alive and keepAliveMsecs option set', done => { + collectorExporter.export(metrics, () => {}); + + setTimeout(() => { + const args = spyRequest.args[0]; + const options = args[0]; + const agent = options.agent; + assert.strictEqual(agent.keepAlive, true); + assert.strictEqual(agent.options.keepAliveMsecs, 2000); + done(); + }); + }); + it('should successfully send metrics', done => { collectorExporter.export(metrics, () => {}); diff --git a/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts index 22e6d69586..fa74402d75 100644 --- a/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts +++ b/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts @@ -50,7 +50,7 @@ const waitTimeMS = 20; describe('CollectorTraceExporter - node with proto over http', () => { let collectorExporter: CollectorTraceExporter; - let collectorExporterConfig: collectorTypes.CollectorExporterConfigBase; + let collectorExporterConfig: collectorTypes.CollectorExporterNodeConfigBase; let spyRequest: sinon.SinonSpy; let spyWrite: sinon.SinonSpy; let spans: ReadableSpan[]; @@ -67,6 +67,8 @@ describe('CollectorTraceExporter - node with proto over http', () => { serviceName: 'bar', attributes: {}, url: 'http://foo.bar.com', + keepAlive: true, + httpAgentOptions: { keepAliveMsecs: 2000 }, }; collectorExporter = new CollectorTraceExporter(collectorExporterConfig); spans = []; @@ -102,6 +104,19 @@ describe('CollectorTraceExporter - node with proto over http', () => { }, waitTimeMS); }); + it('should have keep alive and keepAliveMsecs option set', done => { + collectorExporter.export(spans, () => {}); + + setTimeout(() => { + const args = spyRequest.args[0]; + const options = args[0]; + const agent = options.agent; + assert.strictEqual(agent.keepAlive, true); + assert.strictEqual(agent.options.keepAliveMsecs, 2000); + done(); + }); + }); + it('should successfully send the spans', done => { collectorExporter.export(spans, () => {}); diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts index 1a8a04152a..e80725e1c0 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts @@ -14,8 +14,11 @@ * limitations under the License. */ +import * as http from 'http'; +import * as https from 'https'; + import { CollectorExporterBase } from '../../CollectorExporterBase'; -import { CollectorExporterConfigBase } from '../../types'; +import { CollectorExporterNodeConfigBase } from '../../types'; import * as collectorTypes from '../../types'; import { parseHeaders } from '../../util'; import { sendWithHttp } from './util'; @@ -27,26 +30,30 @@ export abstract class CollectorExporterNodeBase< ExportItem, ServiceRequest > extends CollectorExporterBase< - CollectorExporterConfigBase, + CollectorExporterNodeConfigBase, ExportItem, ServiceRequest > { DEFAULT_HEADERS: Record = {}; headers: Record; - keepAlive: boolean = false; - constructor(config: CollectorExporterConfigBase = {}) { + keepAlive: boolean = true; + httpAgentOptions: http.AgentOptions | https.AgentOptions = {}; + constructor(config: CollectorExporterNodeConfigBase = {}) { super(config); if ((config as any).metadata) { this.logger.warn('Metadata cannot be set when using http'); } this.headers = parseHeaders(config.headers, this.logger) || this.DEFAULT_HEADERS; - if (config.keepAlive) { - this.keepAlive = true; + if (config.keepAlive !== undefined && !config.keepAlive) { + this.keepAlive = config.keepAlive; + } + if (config.httpAgentOptions) { + this.httpAgentOptions = config.httpAgentOptions; } } - onInit(_config: CollectorExporterConfigBase): void { + onInit(_config: CollectorExporterNodeConfigBase): void { this._isShutdown = false; } diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorMetricExporter.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorMetricExporter.ts index f86c397fbe..53cf0a8b3d 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorMetricExporter.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorMetricExporter.ts @@ -15,7 +15,6 @@ */ import { MetricRecord, MetricExporter } from '@opentelemetry/metrics'; -import { CollectorExporterConfigBase } from '../../types'; import * as collectorTypes from '../../types'; import { CollectorExporterNodeBase } from './CollectorExporterNodeBase'; import { toCollectorExportMetricServiceRequest } from '../../transformMetrics'; @@ -45,14 +44,18 @@ export class CollectorMetricExporter ); } - getDefaultUrl(config: CollectorExporterConfigBase): string { + getDefaultUrl( + config: collectorTypes.CollectorExporterNodeConfigBase + ): string { if (!config.url) { return DEFAULT_COLLECTOR_URL; } return config.url; } - getDefaultServiceName(config: CollectorExporterConfigBase): string { + getDefaultServiceName( + config: collectorTypes.CollectorExporterNodeConfigBase + ): string { return config.serviceName || DEFAULT_SERVICE_NAME; } } diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorTraceExporter.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorTraceExporter.ts index 0d5eea66dd..86300101d6 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorTraceExporter.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorTraceExporter.ts @@ -15,7 +15,6 @@ */ import { ReadableSpan, SpanExporter } from '@opentelemetry/tracing'; -import { CollectorExporterConfigBase } from '../../types'; import { CollectorExporterNodeBase } from './CollectorExporterNodeBase'; import * as collectorTypes from '../../types'; import { toCollectorExportTraceServiceRequest } from '../../transform'; @@ -38,14 +37,18 @@ export class CollectorTraceExporter return toCollectorExportTraceServiceRequest(spans, this, true); } - getDefaultUrl(config: CollectorExporterConfigBase): string { + getDefaultUrl( + config: collectorTypes.CollectorExporterNodeConfigBase + ): string { if (!config.url) { return DEFAULT_COLLECTOR_URL; } return config.url; } - getDefaultServiceName(config: CollectorExporterConfigBase): string { + getDefaultServiceName( + config: collectorTypes.CollectorExporterNodeConfigBase + ): string { return config.serviceName || DEFAULT_SERVICE_NAME; } } diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts index a1b7496390..a3f2450bf1 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts @@ -51,8 +51,10 @@ export function sendWithHttp( const request = parsedUrl.protocol === 'http:' ? http.request : https.request; const Agent = parsedUrl.protocol === 'http:' ? http.Agent : https.Agent; if (collector.keepAlive) { - const keepAliveAgent = new Agent({ keepAlive: true }); - options.agent = keepAliveAgent; + options.agent = new Agent({ + ...collector.httpAgentOptions, + keepAlive: true, + }); } const req = request(options, (res: http.IncomingMessage) => { diff --git a/packages/opentelemetry-exporter-collector/src/types.ts b/packages/opentelemetry-exporter-collector/src/types.ts index 9db9876900..6f060993ad 100644 --- a/packages/opentelemetry-exporter-collector/src/types.ts +++ b/packages/opentelemetry-exporter-collector/src/types.ts @@ -16,6 +16,8 @@ import { SpanKind, Logger, Attributes } from '@opentelemetry/api'; import * as api from '@opentelemetry/api'; +import * as http from 'http'; +import * as https from 'https'; /* eslint-disable @typescript-eslint/no-namespace */ export namespace opentelemetryProto { @@ -339,7 +341,15 @@ export interface CollectorExporterConfigBase { serviceName?: string; attributes?: Attributes; url?: string; +} + +/** + * Collector Exporter node base config + */ +export interface CollectorExporterNodeConfigBase + extends CollectorExporterConfigBase { keepAlive?: boolean; + httpAgentOptions?: http.AgentOptions | https.AgentOptions; } /** diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts index 14d9314c79..fc83bdf933 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts @@ -20,7 +20,6 @@ import * as http from 'http'; import * as assert from 'assert'; import * as sinon from 'sinon'; import { CollectorMetricExporter } from '../../src/platform/node'; -import { CollectorExporterConfigBase } from '../../src/types'; import * as collectorTypes from '../../src/types'; import { @@ -48,7 +47,7 @@ const address = 'localhost:1501'; describe('CollectorMetricExporter - node with json over http', () => { let collectorExporter: CollectorMetricExporter; - let collectorExporterConfig: CollectorExporterConfigBase; + let collectorExporterConfig: collectorTypes.CollectorExporterNodeConfigBase; let spyRequest: sinon.SinonSpy; let spyWrite: sinon.SinonSpy; let metrics: MetricRecord[]; @@ -81,6 +80,8 @@ describe('CollectorMetricExporter - node with json over http', () => { serviceName: 'bar', attributes: {}, url: 'http://foo.bar.com', + keepAlive: true, + httpAgentOptions: { keepAliveMsecs: 2000 }, }; collectorExporter = new CollectorMetricExporter(collectorExporterConfig); // Overwrites the start time to make tests consistent @@ -128,6 +129,19 @@ describe('CollectorMetricExporter - node with json over http', () => { }); }); + it('should have keep alive and keepAliveMsecs option set', done => { + collectorExporter.export(metrics, () => {}); + + setTimeout(() => { + const args = spyRequest.args[0]; + const options = args[0]; + const agent = options.agent; + assert.strictEqual(agent.keepAlive, true); + assert.strictEqual(agent.options.keepAliveMsecs, 2000); + done(); + }); + }); + it('should successfully send metrics', done => { collectorExporter.export(metrics, () => {}); diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts index aa9076a896..7d89e1bfd3 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts @@ -21,7 +21,6 @@ import * as http from 'http'; import * as assert from 'assert'; import * as sinon from 'sinon'; import { CollectorTraceExporter } from '../../src/platform/node'; -import { CollectorExporterConfigBase } from '../../src/types'; import * as collectorTypes from '../../src/types'; import { @@ -44,7 +43,7 @@ const address = 'localhost:1501'; describe('CollectorTraceExporter - node with json over http', () => { let collectorExporter: CollectorTraceExporter; - let collectorExporterConfig: CollectorExporterConfigBase; + let collectorExporterConfig: collectorTypes.CollectorExporterNodeConfigBase; let spyRequest: sinon.SinonSpy; let spyWrite: sinon.SinonSpy; let spans: ReadableSpan[]; @@ -77,6 +76,8 @@ describe('CollectorTraceExporter - node with json over http', () => { serviceName: 'bar', attributes: {}, url: 'http://foo.bar.com', + keepAlive: true, + httpAgentOptions: { keepAliveMsecs: 2000 }, }; collectorExporter = new CollectorTraceExporter(collectorExporterConfig); spans = []; @@ -112,6 +113,19 @@ describe('CollectorTraceExporter - node with json over http', () => { }); }); + it('should have keep alive and keepAliveMsecs option set', done => { + collectorExporter.export(spans, () => {}); + + setTimeout(() => { + const args = spyRequest.args[0]; + const options = args[0]; + const agent = options.agent; + assert.strictEqual(agent.keepAlive, true); + assert.strictEqual(agent.options.keepAliveMsecs, 2000); + done(); + }); + }); + it('should successfully send the spans', done => { collectorExporter.export(spans, () => {}); From 657b694be32701c759563dff56cbb5c604939830 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Wed, 18 Nov 2020 18:56:16 +0000 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Bartlomiej Obecny --- .../src/platform/node/CollectorExporterNodeBase.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts index e80725e1c0..e52579e6c9 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts @@ -14,8 +14,8 @@ * limitations under the License. */ -import * as http from 'http'; -import * as https from 'https'; +import type * as http from 'http'; +import type * as https from 'https'; import { CollectorExporterBase } from '../../CollectorExporterBase'; import { CollectorExporterNodeConfigBase } from '../../types'; @@ -45,11 +45,11 @@ export abstract class CollectorExporterNodeBase< } this.headers = parseHeaders(config.headers, this.logger) || this.DEFAULT_HEADERS; - if (config.keepAlive !== undefined && !config.keepAlive) { + if (typeof config.keepAlive === 'boolean') { this.keepAlive = config.keepAlive; } if (config.httpAgentOptions) { - this.httpAgentOptions = config.httpAgentOptions; + this.httpAgentOptions = Object.assign({}, config.httpAgentOptions); } } From 4672a6516f77ef9367a6edafd90a864cef1fcc82 Mon Sep 17 00:00:00 2001 From: Srikanth Chekuri Date: Thu, 19 Nov 2020 02:01:23 +0530 Subject: [PATCH 5/5] chore: review changes --- .../src/CollectorExporterNodeBase.ts | 3 +- .../src/CollectorMetricExporter.ts | 9 ++---- .../src/CollectorTraceExporter.ts | 9 ++---- .../src/util.ts | 3 +- .../test/CollectorMetricExporter.test.ts | 7 +++-- .../test/CollectorTraceExporter.test.ts | 7 +++-- .../node/CollectorExporterNodeBase.ts | 7 ++++- .../platform/node/CollectorMetricExporter.ts | 9 ++---- .../platform/node/CollectorTraceExporter.ts | 9 ++---- .../src/platform/node/index.ts | 1 + .../src/platform/node/types.ts | 28 +++++++++++++++++++ .../src/types.ts | 11 -------- .../test/node/CollectorMetricExporter.test.ts | 7 +++-- .../test/node/CollectorTraceExporter.test.ts | 7 +++-- 14 files changed, 71 insertions(+), 46 deletions(-) create mode 100644 packages/opentelemetry-exporter-collector/src/platform/node/types.ts diff --git a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts index 5d3894875f..92bc64b67c 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/CollectorExporterNodeBase.ts @@ -17,6 +17,7 @@ import { CollectorExporterNodeBase as CollectorExporterBaseMain, collectorTypes, + CollectorExporterNodeConfigBase, } from '@opentelemetry/exporter-collector'; import { ServiceClientType } from './types'; @@ -55,7 +56,7 @@ export abstract class CollectorExporterNodeBase< this._sendingPromises.push(promise); } - onInit(config: collectorTypes.CollectorExporterNodeConfigBase): void { + onInit(config: CollectorExporterNodeConfigBase): void { this._isShutdown = false; // defer to next tick and lazy load to avoid loading protobufjs too early // and making this impossible to be instrumented diff --git a/packages/opentelemetry-exporter-collector-proto/src/CollectorMetricExporter.ts b/packages/opentelemetry-exporter-collector-proto/src/CollectorMetricExporter.ts index d400f84570..43618f5c06 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/CollectorMetricExporter.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/CollectorMetricExporter.ts @@ -17,6 +17,7 @@ import { collectorTypes, toCollectorExportMetricServiceRequest, + CollectorExporterNodeConfigBase, } from '@opentelemetry/exporter-collector'; import { MetricRecord, MetricExporter } from '@opentelemetry/metrics'; import { ServiceClientType } from './types'; @@ -47,18 +48,14 @@ export class CollectorMetricExporter ); } - getDefaultUrl( - config: collectorTypes.CollectorExporterNodeConfigBase - ): string { + getDefaultUrl(config: CollectorExporterNodeConfigBase): string { if (!config.url) { return DEFAULT_COLLECTOR_URL; } return config.url; } - getDefaultServiceName( - config: collectorTypes.CollectorExporterNodeConfigBase - ): string { + getDefaultServiceName(config: CollectorExporterNodeConfigBase): string { return config.serviceName || DEFAULT_SERVICE_NAME; } diff --git a/packages/opentelemetry-exporter-collector-proto/src/CollectorTraceExporter.ts b/packages/opentelemetry-exporter-collector-proto/src/CollectorTraceExporter.ts index 9aa0a6a922..49d29c5d60 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/CollectorTraceExporter.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/CollectorTraceExporter.ts @@ -19,6 +19,7 @@ import { CollectorExporterNodeBase } from './CollectorExporterNodeBase'; import { collectorTypes, toCollectorExportTraceServiceRequest, + CollectorExporterNodeConfigBase, } from '@opentelemetry/exporter-collector'; import { ServiceClientType } from './types'; @@ -40,18 +41,14 @@ export class CollectorTraceExporter return toCollectorExportTraceServiceRequest(spans, this); } - getDefaultUrl( - config: collectorTypes.CollectorExporterNodeConfigBase - ): string { + getDefaultUrl(config: CollectorExporterNodeConfigBase): string { if (!config.url) { return DEFAULT_COLLECTOR_URL; } return config.url; } - getDefaultServiceName( - config: collectorTypes.CollectorExporterNodeConfigBase - ): string { + getDefaultServiceName(config: CollectorExporterNodeConfigBase): string { return config.serviceName || DEFAULT_SERVICE_NAME; } diff --git a/packages/opentelemetry-exporter-collector-proto/src/util.ts b/packages/opentelemetry-exporter-collector-proto/src/util.ts index 499aaf4485..f944d91512 100644 --- a/packages/opentelemetry-exporter-collector-proto/src/util.ts +++ b/packages/opentelemetry-exporter-collector-proto/src/util.ts @@ -17,6 +17,7 @@ import { collectorTypes, sendWithHttp, + CollectorExporterNodeConfigBase, } from '@opentelemetry/exporter-collector'; import * as path from 'path'; @@ -33,7 +34,7 @@ export function getExportRequestProto(): Type | undefined { export function onInit( collector: CollectorExporterNodeBase, - _config: collectorTypes.CollectorExporterNodeConfigBase + _config: CollectorExporterNodeConfigBase ): void { const dir = path.resolve(__dirname, '..', 'protos'); const root = new protobufjs.Root(); diff --git a/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts index d0cc6f8f88..53e90ded78 100644 --- a/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts +++ b/packages/opentelemetry-exporter-collector-proto/test/CollectorMetricExporter.test.ts @@ -14,7 +14,10 @@ * limitations under the License. */ -import { collectorTypes } from '@opentelemetry/exporter-collector'; +import { + collectorTypes, + CollectorExporterNodeConfigBase, +} from '@opentelemetry/exporter-collector'; import * as core from '@opentelemetry/core'; import * as http from 'http'; import * as assert from 'assert'; @@ -53,7 +56,7 @@ const waitTimeMS = 20; describe('CollectorMetricExporter - node with proto over http', () => { let collectorExporter: CollectorMetricExporter; - let collectorExporterConfig: collectorTypes.CollectorExporterNodeConfigBase; + let collectorExporterConfig: CollectorExporterNodeConfigBase; let spyRequest: sinon.SinonSpy; let spyWrite: sinon.SinonSpy; let metrics: MetricRecord[]; diff --git a/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts index fa74402d75..ff35158af6 100644 --- a/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts +++ b/packages/opentelemetry-exporter-collector-proto/test/CollectorTraceExporter.test.ts @@ -14,7 +14,10 @@ * limitations under the License. */ -import { collectorTypes } from '@opentelemetry/exporter-collector'; +import { + collectorTypes, + CollectorExporterNodeConfigBase, +} from '@opentelemetry/exporter-collector'; import * as core from '@opentelemetry/core'; import { ReadableSpan } from '@opentelemetry/tracing'; @@ -50,7 +53,7 @@ const waitTimeMS = 20; describe('CollectorTraceExporter - node with proto over http', () => { let collectorExporter: CollectorTraceExporter; - let collectorExporterConfig: collectorTypes.CollectorExporterNodeConfigBase; + let collectorExporterConfig: CollectorExporterNodeConfigBase; let spyRequest: sinon.SinonSpy; let spyWrite: sinon.SinonSpy; let spans: ReadableSpan[]; diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts index e52579e6c9..2f3d2977de 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts @@ -18,7 +18,7 @@ import type * as http from 'http'; import type * as https from 'https'; import { CollectorExporterBase } from '../../CollectorExporterBase'; -import { CollectorExporterNodeConfigBase } from '../../types'; +import { CollectorExporterNodeConfigBase } from './types'; import * as collectorTypes from '../../types'; import { parseHeaders } from '../../util'; import { sendWithHttp } from './util'; @@ -49,6 +49,11 @@ export abstract class CollectorExporterNodeBase< this.keepAlive = config.keepAlive; } if (config.httpAgentOptions) { + if (!this.keepAlive) { + this.logger.warn( + 'httpAgentOptions is used only when keepAlive is true' + ); + } this.httpAgentOptions = Object.assign({}, config.httpAgentOptions); } } diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorMetricExporter.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorMetricExporter.ts index 53cf0a8b3d..30c70e9e07 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorMetricExporter.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorMetricExporter.ts @@ -16,6 +16,7 @@ import { MetricRecord, MetricExporter } from '@opentelemetry/metrics'; import * as collectorTypes from '../../types'; +import { CollectorExporterNodeConfigBase } from './types'; import { CollectorExporterNodeBase } from './CollectorExporterNodeBase'; import { toCollectorExportMetricServiceRequest } from '../../transformMetrics'; @@ -44,18 +45,14 @@ export class CollectorMetricExporter ); } - getDefaultUrl( - config: collectorTypes.CollectorExporterNodeConfigBase - ): string { + getDefaultUrl(config: CollectorExporterNodeConfigBase): string { if (!config.url) { return DEFAULT_COLLECTOR_URL; } return config.url; } - getDefaultServiceName( - config: collectorTypes.CollectorExporterNodeConfigBase - ): string { + getDefaultServiceName(config: CollectorExporterNodeConfigBase): string { return config.serviceName || DEFAULT_SERVICE_NAME; } } diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorTraceExporter.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorTraceExporter.ts index 86300101d6..4d6e4c03ff 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorTraceExporter.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorTraceExporter.ts @@ -16,6 +16,7 @@ import { ReadableSpan, SpanExporter } from '@opentelemetry/tracing'; import { CollectorExporterNodeBase } from './CollectorExporterNodeBase'; +import { CollectorExporterNodeConfigBase } from './types'; import * as collectorTypes from '../../types'; import { toCollectorExportTraceServiceRequest } from '../../transform'; @@ -37,18 +38,14 @@ export class CollectorTraceExporter return toCollectorExportTraceServiceRequest(spans, this, true); } - getDefaultUrl( - config: collectorTypes.CollectorExporterNodeConfigBase - ): string { + getDefaultUrl(config: CollectorExporterNodeConfigBase): string { if (!config.url) { return DEFAULT_COLLECTOR_URL; } return config.url; } - getDefaultServiceName( - config: collectorTypes.CollectorExporterNodeConfigBase - ): string { + getDefaultServiceName(config: CollectorExporterNodeConfigBase): string { return config.serviceName || DEFAULT_SERVICE_NAME; } } diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/index.ts b/packages/opentelemetry-exporter-collector/src/platform/node/index.ts index 6b647adc24..d2f9fa5223 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/index.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/index.ts @@ -18,3 +18,4 @@ export * from './CollectorTraceExporter'; export * from './CollectorMetricExporter'; export * from './CollectorExporterNodeBase'; export * from './util'; +export * from './types'; diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/types.ts b/packages/opentelemetry-exporter-collector/src/platform/node/types.ts new file mode 100644 index 0000000000..cee0f477a5 --- /dev/null +++ b/packages/opentelemetry-exporter-collector/src/platform/node/types.ts @@ -0,0 +1,28 @@ +/* + * 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 type * as http from 'http'; +import type * as https from 'https'; + +import { CollectorExporterConfigBase } from '../../types'; + +/** + * Collector Exporter node base config + */ +export interface CollectorExporterNodeConfigBase + extends CollectorExporterConfigBase { + keepAlive?: boolean; + httpAgentOptions?: http.AgentOptions | https.AgentOptions; +} diff --git a/packages/opentelemetry-exporter-collector/src/types.ts b/packages/opentelemetry-exporter-collector/src/types.ts index 6f060993ad..ba8c474caf 100644 --- a/packages/opentelemetry-exporter-collector/src/types.ts +++ b/packages/opentelemetry-exporter-collector/src/types.ts @@ -16,8 +16,6 @@ import { SpanKind, Logger, Attributes } from '@opentelemetry/api'; import * as api from '@opentelemetry/api'; -import * as http from 'http'; -import * as https from 'https'; /* eslint-disable @typescript-eslint/no-namespace */ export namespace opentelemetryProto { @@ -343,15 +341,6 @@ export interface CollectorExporterConfigBase { url?: string; } -/** - * Collector Exporter node base config - */ -export interface CollectorExporterNodeConfigBase - extends CollectorExporterConfigBase { - keepAlive?: boolean; - httpAgentOptions?: http.AgentOptions | https.AgentOptions; -} - /** * Mapping between api SpanKind and proto SpanKind */ diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts index fc83bdf933..a37a29c5f2 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts @@ -19,7 +19,10 @@ import * as core from '@opentelemetry/core'; import * as http from 'http'; import * as assert from 'assert'; import * as sinon from 'sinon'; -import { CollectorMetricExporter } from '../../src/platform/node'; +import { + CollectorMetricExporter, + CollectorExporterNodeConfigBase, +} from '../../src/platform/node'; import * as collectorTypes from '../../src/types'; import { @@ -47,7 +50,7 @@ const address = 'localhost:1501'; describe('CollectorMetricExporter - node with json over http', () => { let collectorExporter: CollectorMetricExporter; - let collectorExporterConfig: collectorTypes.CollectorExporterNodeConfigBase; + let collectorExporterConfig: CollectorExporterNodeConfigBase; let spyRequest: sinon.SinonSpy; let spyWrite: sinon.SinonSpy; let metrics: MetricRecord[]; diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts index 7d89e1bfd3..293f70a8f9 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts @@ -20,7 +20,10 @@ import { ReadableSpan } from '@opentelemetry/tracing'; import * as http from 'http'; import * as assert from 'assert'; import * as sinon from 'sinon'; -import { CollectorTraceExporter } from '../../src/platform/node'; +import { + CollectorTraceExporter, + CollectorExporterNodeConfigBase, +} from '../../src/platform/node'; import * as collectorTypes from '../../src/types'; import { @@ -43,7 +46,7 @@ const address = 'localhost:1501'; describe('CollectorTraceExporter - node with json over http', () => { let collectorExporter: CollectorTraceExporter; - let collectorExporterConfig: collectorTypes.CollectorExporterNodeConfigBase; + let collectorExporterConfig: CollectorExporterNodeConfigBase; let spyRequest: sinon.SinonSpy; let spyWrite: sinon.SinonSpy; let spans: ReadableSpan[];