chore: url validation & README to prevent gRPC footguns.

packages/opentelemetry-exporter-collector-grpc/README.md

@@ -23,57 +23,74 @@ const { CollectorTraceExporter } =  require('@opentelemetry/exporter-collector-g
 
 const collectorOptions = {
   serviceName: 'basic-service',
-  url: '<opentelemetry-collector-url>' // url is optional and can be omitted - default is localhost:4317
+  // url is optional and can be omitted - default is localhost:4317
+  url: '<collector-hostname>:<port>',
 };
 
 const provider = new BasicTracerProvider();
 const exporter = new CollectorTraceExporter(collectorOptions);
 provider.addSpanProcessor(new SimpleSpanProcessor(exporter));
 
 provider.register();
-
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
+});
 ```
 
 By default, plaintext connection is used. In order to use TLS in Node.js, provide `credentials` option like so:
 
 ```js
 const fs = require('fs');
-const grpc = require('grpc');
+const grpc = require('@grpc/grpc-js');
+
 const { BasicTracerProvider, SimpleSpanProcessor } = require('@opentelemetry/tracing');
 const { CollectorTraceExporter } =  require('@opentelemetry/exporter-collector-grpc');
 
 const collectorOptions = {
   serviceName: 'basic-service',
-  url: '<opentelemetry-collector-url>', // url is optional and can be omitted - default is localhost:4317
-  credentials: grpc.credentials.createSsl(
-    fs.readFileSync('./ca.crt'),
-    fs.readFileSync('./client.key'),
-    fs.readFileSync('./client.crt')
-  )
+  // url is optional and can be omitted - default is localhost:4317
+  url: '<collector-hostname>:<port>',
+  credentials: grpc.credentials.createSsl(),
 };
 
 const provider = new BasicTracerProvider();
 const exporter = new CollectorTraceExporter(collectorOptions);
 provider.addSpanProcessor(new SimpleSpanProcessor(exporter));
 
 provider.register();
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
+});
+```
+
+To use mutual authentication, pass to the `createSsl()` constructor:
+
+```js
+  credentials: grpc.credentials.createSsl(
+    fs.readFileSync('./ca.crt'),
+    fs.readFileSync('./client.key'),
+    fs.readFileSync('./client.crt')
+  ),
 ```
 
-To see how to generate credentials, you can refer to the script used to generate certificates for tests [here](./test/certs/regenerate.sh)
+To generate credentials for mutual authentication, you can refer to the script used to generate certificates for tests [here](./test/certs/regenerate.sh)
 
 The exporter can be configured to send custom metadata with each request as in the example below:
 
 ```js
-const grpc = require('grpc');
+const grpc = require('@grpc/grpc-js');
+
 const { BasicTracerProvider, SimpleSpanProcessor } = require('@opentelemetry/tracing');
 const { CollectorTraceExporter } =  require('@opentelemetry/exporter-collector-grpc');
 
 const metadata = new grpc.Metadata();
+// For instance, an API key or access token might go here.
 metadata.set('k', 'v');
 
 const collectorOptions = {
   serviceName: 'basic-service',
-  url: '<opentelemetry-collector-url>', // url is optional and can be omitted - default is localhost:4317
+  // url is optional and can be omitted - default is localhost:4317
+  url: '<collector-hostname>:<port>',
   metadata, // // an optional grpc.Metadata object to be sent with each request
 };
 
@@ -82,6 +99,9 @@ const exporter = new CollectorTraceExporter(collectorOptions);
 provider.addSpanProcessor(new SimpleSpanProcessor(exporter));
 
 provider.register();
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
+});
 ```
 
 Note, that this will only work if TLS is also configured on the server.
@@ -95,20 +115,24 @@ const { MeterProvider } = require('@opentelemetry/metrics');
 const { CollectorMetricExporter } =  require('@opentelemetry/exporter-collector-grpc');
 const collectorOptions = {
   serviceName: 'basic-service',
-  url: '<opentelemetry-collector-url>', // url is optional and can be omitted - default is localhost:55681
+  // url is optional and can be omitted - default is localhost:4317
+  url: '<collector-hostname>:<port>',
 };
 const exporter = new CollectorMetricExporter(collectorOptions);
 
 // Register the exporter
-const meter = new MeterProvider({
+const provider = new MeterProvider({
   exporter,
   interval: 60000,
-}).getMeter('example-meter');
+})
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
+});
 
 // Now, start recording data
+const meter = provider.getMeter('example-meter');
 const counter = meter.createCounter('metric_name');
 counter.add(10, { 'key': 'value' });
-
 ```
 
 ## Running opentelemetry-collector locally to see the traces

packages/opentelemetry-exporter-collector-grpc/src/CollectorExporterNodeBase.ts

@@ -26,6 +26,7 @@ import {
   ServiceClientType,
 } from './types';
 import { ServiceClient } from './types';
+import { fixUrl } from './util';
 
 /**
  * Collector Metric Exporter abstract base class
@@ -41,21 +42,24 @@ export abstract class CollectorExporterNodeBase<
   grpcQueue: GRPCQueueItem<ExportItem>[] = [];
   metadata?: Metadata;
   serviceClient?: ServiceClient = undefined;
+  serverAddress: string;
   private _send!: Function;
 
   constructor(config: CollectorExporterConfigNode = {}) {
     super(config);
     if (config.headers) {
       diag.warn('Headers cannot be set when using grpc');
     }
+
+    this.serverAddress = fixUrl(this.url);
     this.metadata = config.metadata;
   }
   private _sendPromise(
     objects: ExportItem[],
     onSuccess: () => void,
     onError: (error: collectorTypes.CollectorExporterError) => void
   ): void {
-    const promise = new Promise<void>(resolve => {
+    const promise = new Promise<void>((resolve) => {
       const _onSuccess = (): void => {
         onSuccess();
         _onFinish();

packages/opentelemetry-exporter-collector-grpc/src/util.ts

@@ -21,6 +21,7 @@ import { globalErrorHandler } from '@opentelemetry/core';
 import { collectorTypes } from '@opentelemetry/exporter-collector';
 import * as path from 'path';
 import { CollectorExporterNodeBase } from './CollectorExporterNodeBase';
+import { URL } from 'url';
 import {
   CollectorExporterConfigNode,
   GRPCQueueItem,
@@ -32,7 +33,6 @@ export function onInit<ExportItem, ServiceRequest>(
   config: CollectorExporterConfigNode
 ): void {
   collector.grpcQueue = [];
-  const serverAddress = removeProtocol(collector.url);
   const credentials: grpc.ChannelCredentials =
     config.credentials || grpc.credentials.createInsecure();
 
@@ -47,17 +47,17 @@ export function onInit<ExportItem, ServiceRequest>(
       oneofs: true,
       includeDirs,
     })
-    .then(packageDefinition => {
+    .then((packageDefinition) => {
       const packageObject: any = grpc.loadPackageDefinition(packageDefinition);
 
       if (collector.getServiceClientType() === ServiceClientType.SPANS) {
         collector.serviceClient = new packageObject.opentelemetry.proto.collector.trace.v1.TraceService(
-          serverAddress,
+          collector.serverAddress,
           credentials
         );
       } else {
         collector.serviceClient = new packageObject.opentelemetry.proto.collector.metrics.v1.MetricsService(
-          serverAddress,
+          collector.serverAddress,
           credentials
         );
       }
@@ -69,7 +69,7 @@ export function onInit<ExportItem, ServiceRequest>(
         });
       }
     })
-    .catch(err => {
+    .catch((err) => {
       globalErrorHandler(err);
     });
 }
@@ -105,6 +105,13 @@ export function send<ExportItem, ServiceRequest>(
   }
 }
 
-function removeProtocol(url: string): string {
-  return url.replace(/^https?:\/\//, '');
+export function fixUrl(url: string): string {
+  const target = URL(url);
+  if (target.pathname !== '/') {
+    diag.warn('URL path should not be set when using grpc, ignoring.');
+  }
+  if (target.protocol !== '' && !target.protocol?.match(/(http|grpc)s?/)) {
+    diag.warn('URL protocol should match (http|grpc)s?, ignoring.');
+  }
+  return target.host;
 }

packages/opentelemetry-exporter-collector-grpc/test/CollectorMetricExporter.test.ts

@@ -66,7 +66,7 @@ const testCollectorMetricExporter = (params: TestParams) =>
     let metrics: metrics.MetricRecord[];
     let reqMetadata: grpc.Metadata | undefined;
 
-    before(done => {
+    before((done) => {
       server = new grpc.Server();
       protoLoader
         .load(metricsServiceProtoPath, {
@@ -129,7 +129,7 @@ const testCollectorMetricExporter = (params: TestParams) =>
           )
         : undefined;
       collectorExporter = new CollectorMetricExporter({
-        url: address,
+        url: 'grpcs://' + address,
         credentials,
         serviceName: 'basic-service',
         metadata: params.metadata,
@@ -142,7 +142,7 @@ const testCollectorMetricExporter = (params: TestParams) =>
       const counter: metrics.Metric<metrics.BoundCounter> &
         Counter = mockCounter();
       const observer: metrics.Metric<metrics.BoundObserver> &
-        ValueObserver = mockObserver(observerResult => {
+        ValueObserver = mockObserver((observerResult) => {
         observerResult.observe(3, {});
         observerResult.observe(6, {});
       });
@@ -166,7 +166,7 @@ const testCollectorMetricExporter = (params: TestParams) =>
 
     describe('instance', () => {
       it('should warn about headers', () => {
-        // Need to stub/spy on the underlying logger as the "diag" instance is global
+        // Need to stub/spy on the underlying logger as the 'diag' instance is global
         const spyLoggerWarn = sinon.stub(diag, 'warn');
         collectorExporter = new CollectorMetricExporter({
           serviceName: 'basic-service',
@@ -178,16 +178,28 @@ const testCollectorMetricExporter = (params: TestParams) =>
         const args = spyLoggerWarn.args[0];
         assert.strictEqual(args[0], 'Headers cannot be set when using grpc');
       });
+      it('should warn about path in url', () => {
+        const spyLoggerWarn = sinon.stub(diag, 'warn');
+        collectorExporter = new CollectorMetricExporter({
+          serviceName: 'basic-service',
+          url: address + '/v1/metrics',
+        });
+        const args = spyLoggerWarn.args[0];
+        assert.strictEqual(
+          args[0],
+          'URL path should not be set when using grpc, ignoring.'
+        );
+      });
     });
 
     describe('export', () => {
-      it('should export metrics', done => {
+      it('should export metrics', (done) => {
         const responseSpy = sinon.spy();
         collectorExporter.export(metrics, responseSpy);
         setTimeout(() => {
           assert.ok(
             typeof exportedData !== 'undefined',
-            'resource' + " doesn't exist"
+            'resource' + ' doesn't exist'
           );
           let resource;
           if (exportedData) {
@@ -214,7 +226,7 @@ const testCollectorMetricExporter = (params: TestParams) =>
             );
             assert.ok(
               typeof resource !== 'undefined',
-              "resource doesn't exist"
+              'resource doesn't exist'
             );
             if (resource) {
               ensureResourceIsCorrect(resource);
@@ -230,14 +242,14 @@ const testCollectorMetricExporter = (params: TestParams) =>
   });
 
 describe('CollectorMetricExporter - node (getDefaultUrl)', () => {
-  it('should default to localhost', done => {
+  it('should default to localhost', (done) => {
     const collectorExporter = new CollectorMetricExporter({});
     setTimeout(() => {
       assert.strictEqual(collectorExporter['url'], 'localhost:4317');
       done();
     });
   });
-  it('should keep the URL if included', done => {
+  it('should keep the URL if included', (done) => {
     const url = 'http://foo.bar.com';
     const collectorExporter = new CollectorMetricExporter({ url });
     setTimeout(() => {