From 31ab0128990600116027cb36903fb3e11649a234 Mon Sep 17 00:00:00 2001
From: Amir Blum <amirgiraffe@gmail.com>
Date: Mon, 25 Jan 2021 14:35:42 +0200
Subject: [PATCH] =?UTF-8?q?fix(exporter-collector):=20all=20http=20export?=
 =?UTF-8?q?=20requests=20should=20share=20same=20a=E2=80=A6=20(#1863)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(exporter-collector): all http export requests should share same agent

* fix(exporter-collector): no log warning on agent options with default keepAlive

* fix(collector-exporter): add excpetion message to logger when agent creation fails

* chore: fix typo in test

* style(exporter-collector): object spread undefined
---
 .../node/CollectorExporterNodeBase.ts         | 17 ++--------
 .../src/platform/node/util.ts                 | 33 +++++++++++++++----
 .../test/node/CollectorTraceExporter.test.ts  | 13 ++++++++
 3 files changed, 42 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<string, string> = {};
   headers: Record<string, string>;
-  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..96df8bccca 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<ExportItem, ServiceRequest>(
       '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,26 @@ export function sendWithHttp<ExportItem, ServiceRequest>(
   req.write(data);
   req.end();
 }
+
+export function createHttpAgent(
+  logger: Logger,
+  config: CollectorExporterNodeConfigBase
+): http.Agent | https.Agent | undefined {
+  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;
+
+  try {
+    const parsedUrl = new url.URL(config.url as string);
+    const Agent = parsedUrl.protocol === 'http:' ? http.Agent : https.Agent;
+    return new Agent({ keepAlive: true, ...config.httpAgentOptions });
+  } catch (err) {
+    logger.error(
+      `collector exporter failed to create http agent. err: ${err.message}`
+    );
+    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..2a16861a5b 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 requests 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, () => {});