Merge branch 'main' into issue-2346

open-telemetry · Jul 21, 2021 · 21763ba · 21763ba
2 parents 39010a0 + cb06b78
commit 21763ba
Show file tree

Hide file tree

Showing 18 changed files with 272 additions and 51 deletions.
diff --git a/eslint.config.js b/eslint.config.js
@@ -10,6 +10,7 @@ module.exports = {
     "project": "./tsconfig.json"
   },
   rules: {
+    "quotes": [2, "single", { "avoidEscape": true }],
     "@typescript-eslint/no-floating-promises": 2,
     "@typescript-eslint/no-this-alias": "off",
     "brace-style": ["error", "1tbs"],

diff --git a/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts b/packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts
@@ -26,7 +26,7 @@ import {
   ServiceClientType,
 } from './types';
 import { ServiceClient } from './types';
-import { getEnv, baggageUtils } from "@opentelemetry/core";
+import { getEnv, baggageUtils } from '@opentelemetry/core';
 
 /**
  * Collector Metric Exporter abstract base class

diff --git a/packages/opentelemetry-exporter-collector-grpc/src/CollectorMetricExporter.ts b/packages/opentelemetry-exporter-collector-grpc/src/CollectorMetricExporter.ts
@@ -23,7 +23,7 @@ import { CollectorExporterConfigNode, ServiceClientType } from './types';
 import { CollectorExporterNodeBase } from './CollectorExporterNodeBase';
 import { baggageUtils, getEnv } from '@opentelemetry/core';
 import { validateAndNormalizeUrl } from './util';
-import { Metadata } from "@grpc/grpc-js";
+import { Metadata } from '@grpc/grpc-js';
 
 const DEFAULT_COLLECTOR_URL = 'localhost:4317';
 

diff --git a/packages/opentelemetry-exporter-collector-grpc/src/CollectorTraceExporter.ts b/packages/opentelemetry-exporter-collector-grpc/src/CollectorTraceExporter.ts
@@ -23,7 +23,7 @@ import {
 import { CollectorExporterConfigNode, ServiceClientType } from './types';
 import { baggageUtils, getEnv } from '@opentelemetry/core';
 import { validateAndNormalizeUrl } from './util';
-import { Metadata } from "@grpc/grpc-js";
+import { Metadata } from '@grpc/grpc-js';
 
 const DEFAULT_COLLECTOR_URL = 'localhost:4317';
 

diff --git a/packages/opentelemetry-exporter-collector-grpc/src/util.ts b/packages/opentelemetry-exporter-collector-grpc/src/util.ts
@@ -108,8 +108,12 @@ export function send<ExportItem, ServiceRequest>(
 }
 
 export function validateAndNormalizeUrl(url: string): string {
+  const hasProtocol = url.match(/^([\w]{1,8}):\/\//);
+  if (!hasProtocol) {
+    url = `https://${url}`;
+  }
   const target = new URL(url);
-  if (target.pathname !== '/') {
+  if (target.pathname && target.pathname !== '/') {
     diag.warn(
       'URL path should not be set when using grpc, the path part of the URL will be ignored.'
     );

diff --git a/packages/opentelemetry-exporter-collector-grpc/test/helper.ts b/packages/opentelemetry-exporter-collector-grpc/test/helper.ts
@@ -410,31 +410,31 @@ export function ensureResourceIsCorrect(
   assert.deepStrictEqual(resource, {
     attributes: [
       {
-        "key": "service.name",
-        "value": {
-          "stringValue": `unknown_service:${process.argv0}`,
-          "value": "stringValue"
+        'key': 'service.name',
+        'value': {
+          'stringValue': `unknown_service:${process.argv0}`,
+          'value': 'stringValue'
         }
       },
       {
-        "key": "telemetry.sdk.language",
-        "value": {
-          "stringValue": "nodejs",
-          "value": "stringValue"
+        'key': 'telemetry.sdk.language',
+        'value': {
+          'stringValue': 'nodejs',
+          'value': 'stringValue'
         }
       },
       {
-        "key": "telemetry.sdk.name",
-        "value": {
-          "stringValue": "opentelemetry",
-          "value": "stringValue"
+        'key': 'telemetry.sdk.name',
+        'value': {
+          'stringValue': 'opentelemetry',
+          'value': 'stringValue'
         }
       },
       {
-        "key": "telemetry.sdk.version",
-        "value": {
-          "stringValue": VERSION,
-          "value": "stringValue"
+        'key': 'telemetry.sdk.version',
+        'value': {
+          'stringValue': VERSION,
+          'value': 'stringValue'
         }
       },
       {

diff --git a/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts
@@ -0,0 +1,81 @@
+/*
+ * 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 * as sinon from 'sinon';
+import * as assert from 'assert';
+
+import { diag } from '@opentelemetry/api';
+import { validateAndNormalizeUrl } from '../src/util';
+
+// Tests added to detect breakage released in #2130
+describe('validateAndNormalizeUrl()', () => {
+  const tests = [
+    {
+      name: 'bare hostname should return same value',
+      input: 'api.datacat.io',
+      expected: 'api.datacat.io',
+    },
+    {
+      name: 'host:port should return same value',
+      input: 'api.datacat.io:1234',
+      expected: 'api.datacat.io:1234',
+    },
+    {
+      name: 'grpc://host:port should trim off protocol',
+      input: 'grpc://api.datacat.io:1234',
+      expected: 'api.datacat.io:1234',
+    },
+    {
+      name: 'bad protocol should warn but return host:port',
+      input: 'badproto://api.datacat.io:1234',
+      expected: 'api.datacat.io:1234',
+      warn: 'URL protocol should be http(s):// or grpc(s)://. Using grpc://.',
+    },
+    {
+      name: 'path on end of url should warn but return host:port',
+      input: 'grpc://api.datacat.io:1234/a/b/c',
+      expected: 'api.datacat.io:1234',
+      warn: 'URL path should not be set when using grpc, the path part of the URL will be ignored.',
+    },
+    {
+      name: ':// in path should not be used for protocol even if protocol not specified',
+      input: 'api.datacat.io/a/b://c',
+      expected: 'api.datacat.io',
+      warn: 'URL path should not be set when using grpc, the path part of the URL will be ignored.',
+    },
+    {
+      name: ':// in path is valid when a protocol is specified',
+      input: 'grpc://api.datacat.io/a/b://c',
+      expected: 'api.datacat.io',
+      warn: 'URL path should not be set when using grpc, the path part of the URL will be ignored.',
+    },
+  ];
+  tests.forEach(test => {
+    it(test.name, () => {
+      const diagWarn = sinon.stub(diag, 'warn');
+      try {
+        assert.strictEqual(validateAndNormalizeUrl(test.input), (test.expected));
+        if (test.warn) {
+          sinon.assert.calledWith(diagWarn, test.warn);
+        } else {
+          sinon.assert.notCalled(diagWarn);
+        }
+      } finally {
+        diagWarn.restore();
+      }
+    });
+  });
+});
diff --git a/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts b/packages/opentelemetry-exporter-collector/src/platform/browser/util.ts
@@ -55,9 +55,16 @@ export function sendWithXhr(
 ) {
   const xhr = new XMLHttpRequest();
   xhr.open('POST', url);
-  xhr.setRequestHeader('Accept', 'application/json');
-  xhr.setRequestHeader('Content-Type', 'application/json');
-  Object.entries(headers).forEach(([k, v]) => {
+
+  const defaultHeaders = {
+    'Accept': 'application/json',
+    'Content-Type': 'application/json',
+  };
+
+  Object.entries({
+    ...defaultHeaders,
+    ...headers,
+  }).forEach(([k, v]) => {
     xhr.setRequestHeader(k, v);
   });
 

diff --git a/packages/opentelemetry-exporter-collector/test/browser/util.test.ts b/packages/opentelemetry-exporter-collector/test/browser/util.test.ts
@@ -0,0 +1,128 @@
+/*
+ * 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 * as sinon from 'sinon';
+import { sendWithXhr } from '../../src/platform/browser/util';
+import { ensureHeadersContain } from '../helper';
+
+describe('util - browser', () => {
+  let server: any;
+  const body = '';
+  const url = '';
+
+  let onSuccessStub: sinon.SinonStub;
+  let onErrorStub: sinon.SinonStub;
+
+  beforeEach(() => {
+    onSuccessStub = sinon.stub();
+    onErrorStub = sinon.stub();
+    server = sinon.fakeServer.create();
+  });
+
+  afterEach(() => {
+    server.restore();
+    sinon.restore();
+  });
+
+  describe('when XMLHTTPRequest is used', () => {
+    let expectedHeaders: Record<string,string>;
+    beforeEach(()=>{
+      expectedHeaders = {
+        // ;charset=utf-8 is applied by sinon.fakeServer
+        'Content-Type': 'application/json;charset=utf-8',
+        'Accept': 'application/json',
+      }
+    });
+    describe('and Content-Type header is set', () => {
+      beforeEach(()=>{
+        const explicitContentType = {
+        'Content-Type': 'application/json',
+      };
+        sendWithXhr(body, url, explicitContentType, onSuccessStub, onErrorStub);
+      });
+      it('Request Headers should contain "Content-Type" header', done => {
+
+        setTimeout(() => {
+          const { requestHeaders } = server.requests[0];
+          ensureHeadersContain(requestHeaders, expectedHeaders);
+          done();
+        });
+      });
+      it('Request Headers should contain "Accept" header', done => {
+
+        setTimeout(() => {
+          const { requestHeaders } = server.requests[0];
+          ensureHeadersContain(requestHeaders, expectedHeaders);
+          done();
+        });
+      });
+    });
+
+    describe('and empty headers are set', () => {
+      beforeEach(()=>{
+        const emptyHeaders = {};
+        sendWithXhr(body, url, emptyHeaders, onSuccessStub, onErrorStub);
+      });
+      it('Request Headers should contain "Content-Type" header', done => {
+
+        setTimeout(() => {
+          const { requestHeaders } = server.requests[0];
+          ensureHeadersContain(requestHeaders, expectedHeaders);
+          done();
+        });
+      });
+      it('Request Headers should contain "Accept" header', done => {
+
+        setTimeout(() => {
+          const { requestHeaders } = server.requests[0];
+          ensureHeadersContain(requestHeaders, expectedHeaders);
+          done();
+        });
+      });
+    });
+    describe('and custom headers are set', () => {
+      let customHeaders: Record<string,string>;
+      beforeEach(()=>{
+        customHeaders = { aHeader: 'aValue', bHeader: 'bValue' };
+        sendWithXhr(body, url, customHeaders, onSuccessStub, onErrorStub);
+      });
+      it('Request Headers should contain "Content-Type" header', done => {
+
+        setTimeout(() => {
+          const { requestHeaders } = server.requests[0];
+          ensureHeadersContain(requestHeaders, expectedHeaders);
+          done();
+        });
+      });
+      it('Request Headers should contain "Accept" header', done => {
+
+        setTimeout(() => {
+          const { requestHeaders } = server.requests[0];
+          ensureHeadersContain(requestHeaders, expectedHeaders);
+          done();
+        });
+      });
+      it('Request Headers should contain custom headers', done => {
+
+        setTimeout(() => {
+          const { requestHeaders } = server.requests[0];
+          ensureHeadersContain(requestHeaders, customHeaders);
+          done();
+        });
+      });
+    });
+  });
+});
diff --git a/packages/opentelemetry-exporter-jaeger/src/jaeger.ts b/packages/opentelemetry-exporter-jaeger/src/jaeger.ts
@@ -156,7 +156,7 @@ export class JaegerExporter implements SpanExporter {
     }
 
     const serviceNameTag = span.tags.find(t => t.key === ResourceAttributes.SERVICE_NAME)
-    const serviceName = serviceNameTag?.vStr || "unknown_service";
+    const serviceName = serviceNameTag?.vStr || 'unknown_service';
 
     sender.setProcess({
       serviceName,

diff --git a/packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts b/packages/opentelemetry-exporter-jaeger/test/jaeger.test.ts
@@ -93,8 +93,8 @@ describe('JaegerExporter', () => {
 
       const process: ThriftProcess = exporter['_getSender']({
         tags: [{
-          key: "service.name",
-          vStr: "opentelemetry"
+          key: 'service.name',
+          vStr: 'opentelemetry'
         }]
       } as any)._process;
       assert.strictEqual(exporter['_sender']._host, 'remotehost');
@@ -109,8 +109,8 @@ describe('JaegerExporter', () => {
       const exporter = new JaegerExporter();
       const sender = exporter['_getSender']({
         tags: [{
-          key: "service.name",
-          vStr: "opentelemetry"
+          key: 'service.name',
+          vStr: 'opentelemetry'
         }]
       } as any);
       assert.strictEqual(sender._host, 'localhost');
@@ -121,8 +121,8 @@ describe('JaegerExporter', () => {
       const exporter = new JaegerExporter();
       const sender = exporter['_getSender']({
         tags: [{
-          key: "service.name",
-          vStr: "opentelemetry"
+          key: 'service.name',
+          vStr: 'opentelemetry'
         }]
       } as any);
       assert.strictEqual(sender._host, 'env-set-host');
@@ -135,8 +135,8 @@ describe('JaegerExporter', () => {
       });
       const sender = exporter['_getSender']({
         tags: [{
-          key: "service.name",
-          vStr: "opentelemetry"
+          key: 'service.name',
+          vStr: 'opentelemetry'
         }]
       } as any);
       assert.strictEqual(sender._host, 'option-set-host');

diff --git a/packages/opentelemetry-instrumentation/test/common/autoLoader.test.ts b/packages/opentelemetry-instrumentation/test/common/autoLoader.test.ts
@@ -22,7 +22,7 @@ import { InstrumentationBase, registerInstrumentations } from '../../src';
 
 class DummyTracerProvider implements TracerProvider {
   getTracer(name: string, version?: string): Tracer {
-    throw new Error("not implemented");
+    throw new Error('not implemented');
   }
 }
 class FooInstrumentation extends InstrumentationBase {

diff --git a/packages/opentelemetry-node/test/registration.test.ts b/packages/opentelemetry-node/test/registration.test.ts
@@ -78,7 +78,7 @@ describe('API registration', () => {
       contextManager: null,
     });
 
-    assert.strictEqual(context['_getContextManager'](), ctxManager, "context manager should not change");
+    assert.strictEqual(context['_getContextManager'](), ctxManager, 'context manager should not change');
 
     assert.ok(
       propagation['_getGlobalPropagator']() instanceof CompositePropagator