fix(exporter-collector): all http export requests should share same a… (#1863)

* 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

Author: blumamir

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,26 +36,15 @@ 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) {
       this.logger.warn('Metadata cannot be set when using http');
     }
     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 {

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;
+  }
+}

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, () => {});