From 5c9f4e3f0f88df2aba812165c86a7ed774eb30af Mon Sep 17 00:00:00 2001 From: Liz Fong-Jones Date: Mon, 5 Jul 2021 14:56:44 -0700 Subject: [PATCH 1/7] fix regression from #2130 when host specified without protocol --- .../package.json | 8 +++ .../src/util.ts | 5 +- .../test/util.test.ts | 72 +++++++++++++++++++ 3 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 packages/opentelemetry-exporter-collector-grpc/test/util.test.ts diff --git a/packages/opentelemetry-exporter-collector-grpc/package.json b/packages/opentelemetry-exporter-collector-grpc/package.json index 6743b9a202..43c40d2467 100644 --- a/packages/opentelemetry-exporter-collector-grpc/package.json +++ b/packages/opentelemetry-exporter-collector-grpc/package.json @@ -51,12 +51,20 @@ "@types/mocha": "8.2.2", "@types/node": "14.17.4", "@types/sinon": "10.0.2", + "@types/sinon-chai": "^3.2.5", + "@typescript-eslint/eslint-plugin": "^4.28.2", + "@typescript-eslint/parser": "^4.28.2", + "chai": "^4.3.4", "codecov": "3.8.2", "cpx": "1.5.0", + "eslint": "^7.30.0", + "eslint-plugin-header": "^3.1.1", + "eslint-plugin-node": "^11.1.0", "mocha": "7.2.0", "nyc": "15.1.0", "rimraf": "3.0.2", "sinon": "10.0.0", + "sinon-chai": "^3.7.0", "ts-loader": "8.3.0", "ts-mocha": "8.0.0", "ts-node": "10.0.0", diff --git a/packages/opentelemetry-exporter-collector-grpc/src/util.ts b/packages/opentelemetry-exporter-collector-grpc/src/util.ts index 39330afc3b..e53c8b540e 100644 --- a/packages/opentelemetry-exporter-collector-grpc/src/util.ts +++ b/packages/opentelemetry-exporter-collector-grpc/src/util.ts @@ -108,8 +108,11 @@ export function send( } export function validateAndNormalizeUrl(url: string): string { + if (!url.match(/:\/\//)) { + 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/util.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts new file mode 100644 index 0000000000..f9b4366d2c --- /dev/null +++ b/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts @@ -0,0 +1,72 @@ +/* + * 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 { expect, use, should } from 'chai'; +import * as sinonChai from 'sinon-chai'; +should(); +use(sinonChai); + +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.', + }, + ]; + tests.forEach(test => { + it(test.name, () => { + const diagWarn = sinon.stub(diag, 'warn'); + try { + validateAndNormalizeUrl(test.input).should.eq(test.expected); + if (test.warn) { + diagWarn.should.have.been.calledWith(test.warn); + } else { + diagWarn.should.not.have.been.called; + } + } finally { + diagWarn.restore(); + } + }); + }); +}); From f7f43ae90a8440c23d035191bdc81aefeddce347 Mon Sep 17 00:00:00 2001 From: Liz Fong-Jones Date: Mon, 5 Jul 2021 15:10:46 -0700 Subject: [PATCH 2/7] Update util.test.ts --- .../opentelemetry-exporter-collector-grpc/test/util.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts index f9b4366d2c..b91dc8df62 100644 --- a/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts +++ b/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts @@ -15,7 +15,7 @@ */ import * as sinon from 'sinon'; -import { expect, use, should } from 'chai'; +import { use, should } from 'chai'; import * as sinonChai from 'sinon-chai'; should(); use(sinonChai); From a0fab3157d3962372fe3e2f7ecaa06bdbb84ca5d Mon Sep 17 00:00:00 2001 From: Liz Fong-Jones Date: Mon, 12 Jul 2021 08:27:44 -0700 Subject: [PATCH 3/7] Apply suggestions from code review Co-authored-by: Naseem --- .../test/util.test.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts index b91dc8df62..97e917456a 100644 --- a/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts +++ b/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts @@ -15,10 +15,7 @@ */ import * as sinon from 'sinon'; -import { use, should } from 'chai'; -import * as sinonChai from 'sinon-chai'; -should(); -use(sinonChai); +import * as assert from 'assert'; import { diag } from '@opentelemetry/api'; import { validateAndNormalizeUrl } from '../src/util'; @@ -58,11 +55,11 @@ describe('validateAndNormalizeUrl()', () => { it(test.name, () => { const diagWarn = sinon.stub(diag, 'warn'); try { - validateAndNormalizeUrl(test.input).should.eq(test.expected); + assert.strictEqual(validateAndNormalizeUrl(test.input), (test.expected)); if (test.warn) { - diagWarn.should.have.been.calledWith(test.warn); + sinon.assert.calledWith(diagWarn, test.warn); } else { - diagWarn.should.not.have.been.called; + sinon.assert.notCalled(diagWarn); } } finally { diagWarn.restore(); From c3b5739d3a81a51818189f5614cbf79140e0df0d Mon Sep 17 00:00:00 2001 From: Liz Fong-Jones Date: Mon, 12 Jul 2021 08:30:30 -0700 Subject: [PATCH 4/7] revert package.json changes --- .../opentelemetry-exporter-collector-grpc/package.json | 8 -------- 1 file changed, 8 deletions(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/package.json b/packages/opentelemetry-exporter-collector-grpc/package.json index 1849d703a8..66071b2b3c 100644 --- a/packages/opentelemetry-exporter-collector-grpc/package.json +++ b/packages/opentelemetry-exporter-collector-grpc/package.json @@ -51,20 +51,12 @@ "@types/mocha": "8.2.2", "@types/node": "14.17.4", "@types/sinon": "10.0.2", - "@types/sinon-chai": "^3.2.5", - "@typescript-eslint/eslint-plugin": "^4.28.2", - "@typescript-eslint/parser": "^4.28.2", - "chai": "^4.3.4", "codecov": "3.8.2", "cpx": "1.5.0", - "eslint": "^7.30.0", - "eslint-plugin-header": "^3.1.1", - "eslint-plugin-node": "^11.1.0", "mocha": "7.2.0", "nyc": "15.1.0", "rimraf": "3.0.2", "sinon": "10.0.0", - "sinon-chai": "^3.7.0", "ts-loader": "8.3.0", "ts-mocha": "8.0.0", "typescript": "4.3.5" From e6a0f708d101343b19a6b1643aa4103a03f67a68 Mon Sep 17 00:00:00 2001 From: Liz Fong-Jones Date: Mon, 12 Jul 2021 10:16:52 -0700 Subject: [PATCH 5/7] Update util.ts --- packages/opentelemetry-exporter-collector-grpc/src/util.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/src/util.ts b/packages/opentelemetry-exporter-collector-grpc/src/util.ts index e53c8b540e..c6836c600f 100644 --- a/packages/opentelemetry-exporter-collector-grpc/src/util.ts +++ b/packages/opentelemetry-exporter-collector-grpc/src/util.ts @@ -108,7 +108,7 @@ export function send( } export function validateAndNormalizeUrl(url: string): string { - if (!url.match(/:\/\//)) { + if (!url.match(/^([\w]{1,8}):\/\//)) { url = `https://${url}`; } const target = new URL(url); From e65d5e69bf0f50d773d435076c4b7f1e41757ddc Mon Sep 17 00:00:00 2001 From: Liz Fong-Jones Date: Mon, 12 Jul 2021 10:29:47 -0700 Subject: [PATCH 6/7] add tests as per @msnev request --- .../test/util.test.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts index 97e917456a..ba08009cd5 100644 --- a/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts +++ b/packages/opentelemetry-exporter-collector-grpc/test/util.test.ts @@ -50,6 +50,18 @@ describe('validateAndNormalizeUrl()', () => { 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, () => { From 2aa5f4e0f8ee0e4e5d025978cf02dca9a53a6687 Mon Sep 17 00:00:00 2001 From: Liz Fong-Jones Date: Mon, 12 Jul 2021 10:30:18 -0700 Subject: [PATCH 7/7] Update packages/opentelemetry-exporter-collector-grpc/src/util.ts Co-authored-by: Naseem --- packages/opentelemetry-exporter-collector-grpc/src/util.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-exporter-collector-grpc/src/util.ts b/packages/opentelemetry-exporter-collector-grpc/src/util.ts index c6836c600f..299a6e7e81 100644 --- a/packages/opentelemetry-exporter-collector-grpc/src/util.ts +++ b/packages/opentelemetry-exporter-collector-grpc/src/util.ts @@ -108,7 +108,8 @@ export function send( } export function validateAndNormalizeUrl(url: string): string { - if (!url.match(/^([\w]{1,8}):\/\//)) { + const hasProtocol = url.match(/^([\w]{1,8}):\/\//); + if (!hasProtocol) { url = `https://${url}`; } const target = new URL(url);