fix(exporters): do not append trailing '/' when URL contains path (op…

…en-telemetry#3274)
weyert · Oct 5, 2022 · 1864a9a · 1864a9a
1 parent e91cac5
commit 1864a9a
Show file tree

Hide file tree

Showing 9 changed files with 39 additions and 21 deletions.
diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md
@@ -13,6 +13,7 @@ All notable changes to experimental packages in this project will be documented
 ### :bug: (Bug Fix)
 
 * fix(node-sdk): move `@opentelemetry/semantic-conventions` to `dependencies` [#3283](https://github.com/open-telemetry/opentelemetry-js/pull/3283) @mhassan1
+* fix(exporters): do not append trailing '/' when URL contains path [#3274](https://github.com/open-telemetry/opentelemetry-js/pull/3274) @pichlermarc
 
 ### :books: (Refine Doc)
 

diff --git a/experimental/packages/exporter-trace-otlp-http/src/platform/browser/OTLPTraceExporter.ts b/experimental/packages/exporter-trace-otlp-http/src/platform/browser/OTLPTraceExporter.ts
@@ -53,7 +53,7 @@ export class OTLPTraceExporter
     return typeof config.url === 'string'
       ? config.url
       : getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT.length > 0
-        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
+        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT)
         : getEnv().OTEL_EXPORTER_OTLP_ENDPOINT.length > 0
           ? appendResourcePathToUrl(getEnv().OTEL_EXPORTER_OTLP_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
           : DEFAULT_COLLECTOR_URL;

diff --git a/experimental/packages/exporter-trace-otlp-http/src/platform/node/OTLPTraceExporter.ts b/experimental/packages/exporter-trace-otlp-http/src/platform/node/OTLPTraceExporter.ts
@@ -52,7 +52,7 @@ export class OTLPTraceExporter
     return typeof config.url === 'string'
       ? config.url
       : getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT.length > 0
-        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
+        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT)
         : getEnv().OTEL_EXPORTER_OTLP_ENDPOINT.length > 0
           ? appendResourcePathToUrl(getEnv().OTEL_EXPORTER_OTLP_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
           : DEFAULT_COLLECTOR_URL;

diff --git a/experimental/packages/exporter-trace-otlp-proto/src/OTLPTraceExporter.ts b/experimental/packages/exporter-trace-otlp-proto/src/OTLPTraceExporter.ts
@@ -54,7 +54,7 @@ export class OTLPTraceExporter
     return typeof config.url === 'string'
       ? config.url
       : getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT.length > 0
-        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
+        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT)
         : getEnv().OTEL_EXPORTER_OTLP_ENDPOINT.length > 0
           ? appendResourcePathToUrl(getEnv().OTEL_EXPORTER_OTLP_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
           : DEFAULT_COLLECTOR_URL;

diff --git a/...kages/opentelemetry-exporter-metrics-otlp-http/src/platform/browser/OTLPMetricExporter.ts b/...kages/opentelemetry-exporter-metrics-otlp-http/src/platform/browser/OTLPMetricExporter.ts
@@ -45,7 +45,7 @@ class OTLPExporterBrowserProxy extends OTLPExporterBrowserBase<ResourceMetrics,
     return typeof config.url === 'string'
       ? config.url
       : getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT.length > 0
-        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
+        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT)
         : getEnv().OTEL_EXPORTER_OTLP_ENDPOINT.length > 0
           ? appendResourcePathToUrl(getEnv().OTEL_EXPORTER_OTLP_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
           : DEFAULT_COLLECTOR_URL;

diff --git a/...packages/opentelemetry-exporter-metrics-otlp-http/src/platform/node/OTLPMetricExporter.ts b/...packages/opentelemetry-exporter-metrics-otlp-http/src/platform/node/OTLPMetricExporter.ts
@@ -49,7 +49,7 @@ class OTLPExporterNodeProxy extends OTLPExporterNodeBase<ResourceMetrics, IExpor
     return typeof config.url === 'string'
       ? config.url
       : getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT.length > 0
-        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
+        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT)
         : getEnv().OTEL_EXPORTER_OTLP_ENDPOINT.length > 0
           ? appendResourcePathToUrl(getEnv().OTEL_EXPORTER_OTLP_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
           : DEFAULT_COLLECTOR_URL;

diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/src/OTLPMetricExporter.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/src/OTLPMetricExporter.ts
@@ -52,7 +52,7 @@ class OTLPMetricExporterNodeProxy extends OTLPProtoExporterNodeBase<ResourceMetr
     return typeof config.url === 'string'
       ? config.url
       : getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT.length > 0
-        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
+        ? appendRootPathToUrlIfNeeded(getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT)
         : getEnv().OTEL_EXPORTER_OTLP_ENDPOINT.length > 0
           ? appendResourcePathToUrl(getEnv().OTEL_EXPORTER_OTLP_ENDPOINT, DEFAULT_COLLECTOR_RESOURCE_PATH)
           : DEFAULT_COLLECTOR_URL;

diff --git a/experimental/packages/otlp-exporter-base/src/util.ts b/experimental/packages/otlp-exporter-base/src/util.ts
@@ -53,14 +53,19 @@ export function appendResourcePathToUrl(url: string, path: string): string {
 /**
  * Adds root path to signal specific endpoint when endpoint contains no path part and no root path
  * @param url
- * @param path
  * @returns url
  */
-export function appendRootPathToUrlIfNeeded(url: string, path: string): string {
-  if (!url.includes(path) && !url.endsWith('/')) {
-    url = url + '/';
+export function appendRootPathToUrlIfNeeded(url: string): string {
+  try {
+    const parsedUrl = new URL(url);
+    if (parsedUrl.pathname === '') {
+      parsedUrl.pathname = parsedUrl.pathname + '/';
+    }
+    return parsedUrl.toString();
+  } catch {
+    diag.warn(`Could not parse export URL: '${url}'`);
+    return url;
   }
-  return url;
 }
 
 /**
@@ -83,7 +88,7 @@ export function configureExporterTimeout(timeoutMillis: number | undefined): num
 function getExporterTimeoutFromEnv(): number {
   const definedTimeout =
     Number(getEnv().OTEL_EXPORTER_OTLP_TRACES_TIMEOUT ??
-    getEnv().OTEL_EXPORTER_OTLP_TIMEOUT);
+      getEnv().OTEL_EXPORTER_OTLP_TIMEOUT);
 
   if (definedTimeout <= 0) {
     // OTLP exporter configured timeout - using default value of 10000ms

diff --git a/experimental/packages/otlp-exporter-base/test/common/util.test.ts b/experimental/packages/otlp-exporter-base/test/common/util.test.ts
@@ -20,7 +20,7 @@ import { diag } from '@opentelemetry/api';
 import {
   parseHeaders,
   appendResourcePathToUrl,
-  appendRootPathToUrlIfNeeded
+  appendRootPathToUrlIfNeeded,
 } from '../../src/util';
 
 describe('utils', () => {
@@ -84,23 +84,35 @@ describe('utils', () => {
   describe('appendRootPathToUrlIfNeeded - specifc signal http endpoint', () => {
     it('should append root path when missing', () => {
       const url = 'http://foo.bar';
-      const resourcePath = 'v1/traces';
 
-      const finalUrl = appendRootPathToUrlIfNeeded(url, resourcePath);
+      const finalUrl = appendRootPathToUrlIfNeeded(url);
       assert.strictEqual(finalUrl, url + '/');
     });
     it('should not append root path and return same url', () => {
       const url = 'http://foo.bar/';
-      const resourcePath = 'v1/traces';
 
-      const finalUrl = appendRootPathToUrlIfNeeded(url, resourcePath);
+      const finalUrl = appendRootPathToUrlIfNeeded(url);
       assert.strictEqual(finalUrl, url);
     });
-    it('should append root path when url contains resource path', () => {
-      const url = 'http://foo.bar/v1/traces';
-      const resourcePath = 'v1/traces';
+    it('should not append root path when url contains resource path', () => {
+      {
+        const url = 'http://foo.bar/v1/traces';
+
+        const finalUrl = appendRootPathToUrlIfNeeded(url);
+        assert.strictEqual(finalUrl, url);
+      }
+      {
+        const url = 'https://endpoint/something';
+
+        const finalUrl = appendRootPathToUrlIfNeeded(url);
+        assert.strictEqual(finalUrl, url);
+      }
+    });
+
+    it('should not change string when url is not parseable', () => {
+      const url = 'this is not a URL';
 
-      const finalUrl = appendRootPathToUrlIfNeeded(url, resourcePath);
+      const finalUrl = appendRootPathToUrlIfNeeded(url);
       assert.strictEqual(finalUrl, url);
     });
   });