From cd40b7aa3137fa3cfe637d2af49789629126c435 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Sun, 24 Jan 2021 16:37:38 +0200 Subject: [PATCH 1/5] fix(exporter-collector): all http export requests should share same agent --- .../node/CollectorExporterNodeBase.ts | 17 ++-------- .../src/platform/node/util.ts | 31 ++++++++++++++----- .../test/node/CollectorTraceExporter.test.ts | 13 ++++++++ 3 files changed, 40 insertions(+), 21 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts index 2f3d2977de..96194ddb66 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporterNodeBase.ts @@ -21,7 +21,7 @@ import { CollectorExporterBase } from '../../CollectorExporterBase'; import { CollectorExporterNodeConfigBase } from './types'; import * as collectorTypes from '../../types'; import { parseHeaders } from '../../util'; -import { sendWithHttp } from './util'; +import { createHttpAgent, sendWithHttp } from './util'; /** * Collector Metric Exporter abstract base class @@ -36,8 +36,7 @@ export abstract class CollectorExporterNodeBase< > { DEFAULT_HEADERS: Record = {}; headers: Record; - keepAlive: boolean = true; - httpAgentOptions: http.AgentOptions | https.AgentOptions = {}; + agent: http.Agent | https.Agent | undefined; constructor(config: CollectorExporterNodeConfigBase = {}) { super(config); if ((config as any).metadata) { @@ -45,17 +44,7 @@ export abstract class CollectorExporterNodeBase< } this.headers = parseHeaders(config.headers, this.logger) || this.DEFAULT_HEADERS; - if (typeof config.keepAlive === 'boolean') { - 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); - } + this.agent = createHttpAgent(this.logger, config); } onInit(_config: CollectorExporterNodeConfigBase): void { diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts index edab1f2227..2ab3c88f90 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts @@ -18,6 +18,8 @@ import * as http from 'http'; import * as https from 'https'; import * as collectorTypes from '../../types'; import { CollectorExporterNodeBase } from './CollectorExporterNodeBase'; +import { CollectorExporterNodeConfigBase } from '.'; +import { Logger } from '@opentelemetry/api'; /** * Sends data using http @@ -46,16 +48,10 @@ export function sendWithHttp( 'Content-Type': contentType, ...collector.headers, }, + agent: collector.agent, }; const request = parsedUrl.protocol === 'http:' ? http.request : https.request; - const Agent = parsedUrl.protocol === 'http:' ? http.Agent : https.Agent; - if (collector.keepAlive) { - options.agent = new Agent({ - ...collector.httpAgentOptions, - keepAlive: true, - }); - } const req = request(options, (res: http.IncomingMessage) => { let data = ''; @@ -81,3 +77,24 @@ export function sendWithHttp( req.write(data); req.end(); } + +export function createHttpAgent( + logger: Logger, + config: CollectorExporterNodeConfigBase +): http.Agent | https.Agent | undefined { + if (config.httpAgentOptions && !config.keepAlive) { + logger.warn('httpAgentOptions is used only when keepAlive is true'); + } + + if (config.keepAlive === false || !config.url) return undefined; + + try { + const parsedUrl = new url.URL(config.url as string); + const httpAgentOptions = config.httpAgentOptions ?? {}; + const Agent = parsedUrl.protocol === 'http:' ? http.Agent : https.Agent; + return new Agent({ keepAlive: true, ...httpAgentOptions }); + } catch { + logger.error('collector exporter failed to create http agent'); + return undefined; + } +} diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts index 69a7b1fe57..bdf7938260 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts @@ -126,6 +126,19 @@ describe('CollectorTraceExporter - node with json over http', () => { }); }); + it('different http export request should use the same agent', done => { + collectorExporter.export(spans, () => {}); + collectorExporter.export(spans, () => {}); + + setTimeout(() => { + const [firstExportAgent, secondExportAgent] = spyRequest.args.map( + a => a[0].agent + ); + assert.strictEqual(firstExportAgent, secondExportAgent); + done(); + }); + }); + it('should successfully send the spans', done => { collectorExporter.export(spans, () => {}); From fd704c85f4578efa97fd0ff9ae7e3c46d90d0da1 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Sun, 24 Jan 2021 17:13:54 +0200 Subject: [PATCH 2/5] fix(exporter-collector): no log warning on agent options with default keepAlive --- .../opentelemetry-exporter-collector/src/platform/node/util.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts index 2ab3c88f90..727407d135 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts @@ -82,8 +82,9 @@ export function createHttpAgent( logger: Logger, config: CollectorExporterNodeConfigBase ): http.Agent | https.Agent | undefined { - if (config.httpAgentOptions && !config.keepAlive) { + if (config.httpAgentOptions && config.keepAlive === false) { logger.warn('httpAgentOptions is used only when keepAlive is true'); + return undefined; } if (config.keepAlive === false || !config.url) return undefined; From 5419312e320b918791547e41e6e3ee41f78db317 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Sun, 24 Jan 2021 18:33:29 +0200 Subject: [PATCH 3/5] fix(collector-exporter): add excpetion message to logger when agent creation fails --- .../src/platform/node/util.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts index 727407d135..4616924a65 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts @@ -94,8 +94,10 @@ export function createHttpAgent( const httpAgentOptions = config.httpAgentOptions ?? {}; const Agent = parsedUrl.protocol === 'http:' ? http.Agent : https.Agent; return new Agent({ keepAlive: true, ...httpAgentOptions }); - } catch { - logger.error('collector exporter failed to create http agent'); + } catch (err) { + logger.error( + `collector exporter failed to create http agent. err: ${err.message}` + ); return undefined; } } From eca81a46e4e97359927736d32b166c689fbfde4b Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Sun, 24 Jan 2021 21:17:51 +0200 Subject: [PATCH 4/5] chore: fix typo in test --- .../test/node/CollectorTraceExporter.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts index bdf7938260..2a16861a5b 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorTraceExporter.test.ts @@ -126,7 +126,7 @@ describe('CollectorTraceExporter - node with json over http', () => { }); }); - it('different http export request should use the same agent', done => { + it('different http export requests should use the same agent', done => { collectorExporter.export(spans, () => {}); collectorExporter.export(spans, () => {}); From f57a5621743e90406ed81d323e1931a71a6a7897 Mon Sep 17 00:00:00 2001 From: Amir Blum Date: Sun, 24 Jan 2021 21:39:35 +0200 Subject: [PATCH 5/5] style(exporter-collector): object spread undefined --- .../opentelemetry-exporter-collector/src/platform/node/util.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts index 4616924a65..96df8bccca 100644 --- a/packages/opentelemetry-exporter-collector/src/platform/node/util.ts +++ b/packages/opentelemetry-exporter-collector/src/platform/node/util.ts @@ -91,9 +91,8 @@ export function createHttpAgent( try { const parsedUrl = new url.URL(config.url as string); - const httpAgentOptions = config.httpAgentOptions ?? {}; const Agent = parsedUrl.protocol === 'http:' ? http.Agent : https.Agent; - return new Agent({ keepAlive: true, ...httpAgentOptions }); + return new Agent({ keepAlive: true, ...config.httpAgentOptions }); } catch (err) { logger.error( `collector exporter failed to create http agent. err: ${err.message}`