chore: review findings

plugins/node/opentelemetry-instrumentation-dns/README.md

@@ -34,23 +34,13 @@ const dnsInstrumentation = new DnsInstrumentation({
 });
 ```
 
-### Zipkin
-
-If you use Zipkin, you must use `ignoreHostnames` in order to not trace those calls. If the server is local. You can set:
-
-```js
-const dnsInstrumentation = new DnsInstrumentation({
-  ignoreHostnames: ['localhost']
-});
-```
-
 ### Dns Instrumentation Options
 
 Dns instrumentation has currently one option. You can set the following:
 
 | Options | Type | Description |
 | ------- | ---- | ----------- |
-| [`ignoreHostnames`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-instrumentation-dns/src/types.ts#L98) | `IgnoreMatcher[]` | Dns instrumentation will not trace all requests that match hostnames |
+| [`ignoreHostnames`](https://github.com/open-telemetry/opentelemetry-js-contrib/blob/master/packages/opentelemetry-instrumentation-dns/src/types.ts#L99) | `IgnoreMatcher[]` | Dns instrumentation will not trace all requests that match hostnames |
 
 ## Useful links
 
@@ -62,11 +52,11 @@ Dns instrumentation has currently one option. You can set the following:
 
 Apache 2.0 - See [LICENSE][license-url] for more information.
 
-[gitter-image]: https://badges.gitter.im/open-telemetry/opentelemetry-js.svg
+[gitter-image]: https://badges.gitter.im/open-telemetry/opentelemetry-js-contrib.svg
 [gitter-url]: https://gitter.im/open-telemetry/opentelemetry-node?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge
-[license-url]: https://github.com/open-telemetry/opentelemetry-js/blob/master/LICENSE
+[license-url]: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/master/LICENSE
 [license-image]: https://img.shields.io/badge/license-Apache_2.0-green.svg?style=flat
-[dependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/status.svg?path=packages/opentelemetry-instrumentation-dns
-[dependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js?path=packages%2Fopentelemetry-instrumentation-dns
-[devDependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js/dev-status.svg?path=packages/opentelemetry-instrumentation-dns
-[devDependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js?path=packages%2Fopentelemetry-instrumentation-dns&type=dev
+[dependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js-contrib/status.svg?path=packages/opentelemetry-instrumentation-dns
+[dependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js-contrib?path=packages%2Fopentelemetry-instrumentation-dns
+[devDependencies-image]: https://david-dm.org/open-telemetry/opentelemetry-js-contrib/dev-status.svg?path=packages/opentelemetry-instrumentation-dns
+[devDependencies-url]: https://david-dm.org/open-telemetry/opentelemetry-js-contrib?path=packages%2Fopentelemetry-instrumentation-dns&type=dev

plugins/node/opentelemetry-instrumentation-dns/package.json

@@ -64,6 +64,7 @@
   "dependencies": {
     "@opentelemetry/api": "^0.14.0",
     "@opentelemetry/instrumentation": "^0.14.0",
+    "@opentelemetry/semantic-conventions": "^0.14.0",
     "semver": "^7.3.2"
   }
 }

plugins/node/opentelemetry-instrumentation-dns/src/dns.ts

@@ -15,20 +15,20 @@
  */
 
 import { LookupAddress } from 'dns';
-import { Span, SpanKind, SpanOptions } from '@opentelemetry/api';
+import { Span, SpanKind } from '@opentelemetry/api';
+import { GeneralAttribute } from '@opentelemetry/semantic-conventions';
 import {
   InstrumentationBase,
   InstrumentationNodeModuleDefinition,
   isWrapped,
+  safeExecuteInTheMiddle,
 } from '@opentelemetry/instrumentation';
 import * as semver from 'semver';
 import { AddressFamily } from './enums/AddressFamily';
-import { AttributeNames } from './enums/AttributeNames';
 import {
   Dns,
   DnsInstrumentationConfig,
   LookupCallbackSignature,
-  LookupFunctionSignature,
   LookupPromiseSignature,
 } from './types';
 import * as utils from './utils';
@@ -111,29 +111,41 @@ export class DnsInstrumentation extends InstrumentationBase<Dns> {
       const argsCount = args.length;
       plugin._logger.debug('wrap lookup callback function and starts span');
       const name = utils.getOperationName('lookup');
-      const span = plugin._startDnsSpan(name, {
+      const span = plugin.tracer.startSpan(name, {
+        kind: SpanKind.CLIENT,
         attributes: {
-          [AttributeNames.PEER_HOSTNAME]: hostname,
+          [GeneralAttribute.NET_PEER_HOSTNAME]: hostname,
         },
       });
 
       const originalCallback = args[argsCount - 1];
       if (typeof originalCallback === 'function') {
         args[argsCount - 1] = plugin._wrapLookupCallback(
           originalCallback,
-          args[argsCount - 2],
           span
         );
-        return plugin._safeExecute(span, () =>
-          (original as LookupFunctionSignature).apply(this, [
-            hostname,
-            ...args,
-            // eslint-disable-next-line @typescript-eslint/no-explicit-any
-          ] as any)
+        return safeExecuteInTheMiddle(
+          () => original.apply(this, [hostname, ...args]),
+          error => {
+            if (error != null) {
+              utils.setError(error, span, process.version);
+              span.end();
+            }
+          }
         );
       } else {
-        const promise = plugin._safeExecute(span, () =>
-          (original as LookupPromiseSignature).apply(this, [hostname, ...args])
+        const promise = safeExecuteInTheMiddle(
+          () =>
+            (original as LookupPromiseSignature).apply(this, [
+              hostname,
+              ...args,
+            ]),
+          error => {
+            if (error != null) {
+              utils.setError(error, span, process.version);
+              span.end();
+            }
+          }
         );
         promise.then(
           result => {
@@ -151,24 +163,11 @@ export class DnsInstrumentation extends InstrumentationBase<Dns> {
     };
   }
 
-  /**
-   * Start a new span with default attributes and kind
-   */
-  private _startDnsSpan(name: string, options: Omit<SpanOptions, 'kind'>) {
-    return this.tracer
-      .startSpan(name, { ...options, kind: SpanKind.CLIENT })
-      .setAttribute(
-        AttributeNames.COMPONENT,
-        '@opentelemetry/instrumentation-dns'
-      );
-  }
-
   /**
    * Wrap lookup callback function
    */
   private _wrapLookupCallback(
     original: Function,
-    options: unknown,
     span: Span
   ): LookupCallbackSignature {
     const plugin = this;
@@ -191,20 +190,4 @@ export class DnsInstrumentation extends InstrumentationBase<Dns> {
       return original.apply(this, arguments);
     };
   }
-
-  /**
-   * Safely handle "execute" callback
-   */
-  private _safeExecute<T extends (...args: unknown[]) => ReturnType<T>>(
-    span: Span,
-    execute: T
-  ): ReturnType<T> {
-    try {
-      return execute();
-    } catch (error) {
-      utils.setError(error, span, process.version);
-      span.end();
-      throw error;
-    }
-  }
 }

plugins/node/opentelemetry-instrumentation-dns/src/enums/AttributeNames.ts

@@ -14,10 +14,6 @@
  * limitations under the License.
  */
 export enum AttributeNames {
-  COMPONENT = 'component',
-  PEER_HOSTNAME = 'peer.hostname',
-  PEER_PORT = 'peer.port',
-  PEER_SERVICE = 'peer.service',
   // NOT ON OFFICIAL SPEC
   DNS_ERROR_CODE = 'dns.error_code',
   DNS_ERROR_NAME = 'dns.error_name',

plugins/node/opentelemetry-instrumentation-dns/src/types.ts

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import * as dns from 'dns';
+import type * as dns from 'dns';
 import { InstrumentationConfig } from '@opentelemetry/instrumentation';
 
 export type Dns = typeof dns;

plugins/node/opentelemetry-instrumentation-dns/test/utils/assertSpan.ts

@@ -16,11 +16,12 @@
 
 import { SpanKind, Status, StatusCode } from '@opentelemetry/api';
 import { hrTimeToNanoseconds } from '@opentelemetry/core';
+import { ReadableSpan } from '@opentelemetry/tracing';
+import { GeneralAttribute } from '@opentelemetry/semantic-conventions';
 import * as assert from 'assert';
+import type { LookupAddress } from 'dns';
 import { AttributeNames } from '../../src/enums/AttributeNames';
-import { ReadableSpan } from '@opentelemetry/tracing';
 import * as utils from '../../src/utils';
-import { LookupAddress } from 'dns';
 
 export const assertSpan = (
   span: ReadableSpan,
@@ -39,16 +40,12 @@ export const assertSpan = (
 
   assert.strictEqual(span.kind, SpanKind.CLIENT);
 
-  assert.strictEqual(
-    span.attributes[AttributeNames.COMPONENT],
-    '@opentelemetry/instrumentation-dns'
-  );
   assert.strictEqual(
     span.attributes[AttributeNames.DNS_ERROR_MESSAGE],
     span.status.message
   );
   assert.strictEqual(
-    span.attributes[AttributeNames.PEER_HOSTNAME],
+    span.attributes[GeneralAttribute.NET_PEER_HOSTNAME],
     validations.hostname
   );