address review feedback.

open-telemetry · Apr 30, 2021 · 9ef56a0 · 9ef56a0
1 parent 368ff76
commit 9ef56a0
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 22 deletions.
diff --git a/packages/opentelemetry-exporter-collector-grpc/README.md b/packages/opentelemetry-exporter-collector-grpc/README.md
@@ -18,8 +18,6 @@ npm install --save @opentelemetry/exporter-collector-grpc
 The CollectorTraceExporter in Node expects the URL to only be the hostname. It will not work with `/v1/trace`.
 
 ```js
-const Graceful = require('node-graceful');
-
 const { BasicTracerProvider, SimpleSpanProcessor } = require('@opentelemetry/tracing');
 const { CollectorTraceExporter } =  require('@opentelemetry/exporter-collector-grpc');
 
@@ -34,9 +32,8 @@ const exporter = new CollectorTraceExporter(collectorOptions);
 provider.addSpanProcessor(new SimpleSpanProcessor(exporter));
 
 provider.register();
-
-Graceful.on("exit", async () => {
-  await provider.shutdown();
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
 });
 ```
 
@@ -45,7 +42,6 @@ By default, plaintext connection is used. In order to use TLS in Node.js, provid
 ```js
 const fs = require('fs');
 const grpc = require('@grpc/grpc-js');
-const Graceful = require('node-graceful');
 
 const { BasicTracerProvider, SimpleSpanProcessor } = require('@opentelemetry/tracing');
 const { CollectorTraceExporter } =  require('@opentelemetry/exporter-collector-grpc');
@@ -62,9 +58,8 @@ const exporter = new CollectorTraceExporter(collectorOptions);
 provider.addSpanProcessor(new SimpleSpanProcessor(exporter));
 
 provider.register();
-
-Graceful.on("exit", async () => {
-  await provider.shutdown();
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
 });
 ```
 
@@ -84,7 +79,6 @@ The exporter can be configured to send custom metadata with each request as in t
 
 ```js
 const grpc = require('@grpc/grpc-js');
-const Graceful = require('node-graceful');
 
 const { BasicTracerProvider, SimpleSpanProcessor } = require('@opentelemetry/tracing');
 const { CollectorTraceExporter } =  require('@opentelemetry/exporter-collector-grpc');
@@ -105,9 +99,8 @@ const exporter = new CollectorTraceExporter(collectorOptions);
 provider.addSpanProcessor(new SimpleSpanProcessor(exporter));
 
 provider.register();
-
-Graceful.on("exit", async () => {
-  await provider.shutdown();
+['SIGINT', 'SIGTERM'].forEach(signal => {
+  process.on(signal, () => provider.shutdown().catch(console.error));
 });
 ```
 
@@ -128,15 +121,18 @@ const collectorOptions = {
 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

diff --git a/packages/opentelemetry-exporter-collector-grpc/src/util.ts b/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 { parse } from 'url';
 import {
   CollectorExporterConfigNode,
   GRPCQueueItem,
@@ -105,9 +106,12 @@ export function send<ExportItem, ServiceRequest>(
 }
 
 export function fixUrl(url: string): string {
-  const serverAddress = url.replace(/^(http|grpc)s?:\/\//, '');
-  if (serverAddress.includes('/')) {
-    diag.warn('URL path cannot be set when using grpc');
+  const target = parse(url);
+  if (target.pathname !== '/') {
+    diag.warn('URL path should not be set when using grpc, ignoring.');
   }
-  return serverAddress;
+  if (target.protocol !== '' && !target.protocol.match(/(http|grpc)s?/)) {
+    diag.warn('URL protocol should match (http|grpc)s?, ignoring.');
+  }
+  return target.host;
 }
diff --git a/packages/opentelemetry-exporter-collector-grpc/test/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/CollectorMetricExporter.test.ts
@@ -185,7 +185,7 @@ const testCollectorMetricExporter = (params: TestParams) =>
           url: address + '/v1/metrics',
         });
         const args = spyLoggerWarn.args[0];
-        assert.strictEqual(args[0], 'URL path cannot be set when using grpc');
+        assert.strictEqual(args[0], 'URL path should not be set when using grpc, ignoring.');
       });
     });
 

diff --git a/packages/opentelemetry-exporter-collector-grpc/test/CollectorTraceExporter.test.ts b/packages/opentelemetry-exporter-collector-grpc/test/CollectorTraceExporter.test.ts
@@ -162,7 +162,7 @@ const testCollectorExporter = (params: TestParams) =>
           url: address + '/v1/trace',
         });
         const args = spyLoggerWarn.args[0];
-        assert.strictEqual(args[0], 'URL path cannot be set when using grpc');
+        assert.strictEqual(args[0], 'URL path should not be set when using grpc, ignoring.');
       });
     });