Addressing comments

packages/winston-transport/package.json

@@ -42,7 +42,7 @@
     "@types/mocha": "7.0.2",
     "@types/node": "18.6.5",
     "@types/sinon": "10.0.18",
-    "@types/triple-beam": "^1.3.2",
+    "@types/triple-beam": "1.3.2",
     "mocha": "7.2.0",
     "nyc": "15.1.0",
     "rimraf": "5.0.5",

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

@@ -59,7 +59,7 @@ logger.info('foobar');
 
 | Option                  | Type              | Description |
 | ----------------------- | ----------------- | ----------- |
-| `disableLogSending`     | `boolean`         | Whether to disable [log sending](#log-sending). Default `false`. |
+| `enableLogSending`      | `boolean`         | Whether to enable [log sending](#log-sending). Default `false`. |
 | `disableLogCorrelation` | `boolean`         | Whether to disable [log correlation](#log-correlation). Default `false`. |
 | `logHook`               | `LogHookFunction` | An option hook to inject additional context to a log record after trace-context has been added. This requires `disableLogCorrelation` to be false. |  
 
@@ -69,7 +69,7 @@ Winston Logger will automatically send log records to the OpenTelemetry Logs SDK
 
 If the OpenTelemetry SDK is not configured with a Logger provider, then this will be a no-op.
 
-Log sending can be disabled with the `disableLogSending: true` option. Log sending is only available for Winston version 3 and later.
+Log sending can be enabled with the `disableLogSending: false` option. Log sending is only available for Winston version 3 and later.
 
 ```bash
 npm install --save @opentelemetry/winston-transport

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

@@ -6,7 +6,7 @@
   "types": "build/src/index.d.ts",
   "repository": "open-telemetry/opentelemetry-js-contrib",
   "scripts": {
-    "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'",
+    "test": "nyc ts-mocha --inspect-brk -p tsconfig.json 'test/**/*.test.ts'",
     "test-all-versions": "tav",
     "tdd": "npm run test -- --watch-extensions ts --watch",
     "clean": "rimraf build/*",

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

@@ -14,13 +14,7 @@
  * limitations under the License.
  */
 
-import {
-  context,
-  trace,
-  isSpanContextValid,
-  Span,
-  SpanContext,
-} from '@opentelemetry/api';
+import { context, trace, isSpanContextValid, Span } from '@opentelemetry/api';
 import {
   InstrumentationBase,
   InstrumentationNodeModuleDefinition,
@@ -129,10 +123,10 @@ export class WinstonInstrumentation extends InstrumentationBase {
     this._config = config;
   }
 
-  private _callHook(span: Span, record?: Record<string, string>) {
+  private _callHook(span: Span, record: Record<string, string>) {
     const hook = this.getConfig().logHook;
 
-    if (!hook || !record) {
+    if (!hook) {
       return;
     }
 
@@ -154,19 +148,8 @@ export class WinstonInstrumentation extends InstrumentationBase {
         this: never,
         ...args: Parameters<typeof original>
       ) {
-        const config = instrumentation.getConfig();
         const record = args[0];
-
-        if (!config.disableLogCorrelation) {
-          const span = trace.getActiveSpan();
-          if (span) {
-            const spanContext = span.spanContext();
-            if (isSpanContextValid(spanContext)) {
-              injectRecord(spanContext, record);
-              instrumentation._callHook(span, record);
-            }
-          }
-        }
+        instrumentation._handleLogCorrelation(record);
         return original.apply(this, args);
       };
     };
@@ -179,35 +162,25 @@ export class WinstonInstrumentation extends InstrumentationBase {
         this: never,
         ...args: Parameters<typeof original>
       ) {
-        let record: Record<string, any> = {};
-        const config = instrumentation.getConfig();
-        if (!config.disableLogCorrelation) {
-          const span = trace.getSpan(context.active());
-          if (span) {
-            const spanContext = span.spanContext();
-            if (isSpanContextValid(spanContext)) {
-              record = injectRecord(spanContext, record);
-              instrumentation._callHook(span, record);
-              // Inject in metadata argument
-              let isDataInjected = false;
-              for (let i = args.length - 1; i >= 0; i--) {
-                if (typeof args[i] === 'object') {
-                  args[i] = Object.assign(args[i], record);
-                  isDataInjected = true;
-                  break;
-                }
-              }
-              if (!isDataInjected) {
-                const insertAt =
-                  typeof args[args.length - 1] === 'function'
-                    ? args.length - 1
-                    : args.length;
-
-                args.splice(insertAt, 0, record);
-              }
-            }
+        const record: Record<string, any> = {};
+        instrumentation._handleLogCorrelation(record);
+        // Inject in metadata argument
+        let isDataInjected = false;
+        for (let i = args.length - 1; i >= 0; i--) {
+          if (typeof args[i] === 'object') {
+            args[i] = Object.assign(args[i], record);
+            isDataInjected = true;
+            break;
           }
         }
+        if (!isDataInjected) {
+          const insertAt =
+            typeof args[args.length - 1] === 'function'
+              ? args.length - 1
+              : args.length;
+
+          args.splice(insertAt, 0, record);
+        }
 
         return original.apply(this, args);
       };
@@ -222,7 +195,7 @@ export class WinstonInstrumentation extends InstrumentationBase {
         ...args: Parameters<typeof original>
       ) {
         const config = instrumentation.getConfig();
-        if (!config.disableLogSending) {
+        if (config.enableLogSending) {
           if (args && args.length > 0) {
             // Try to load Winston transport
             try {
@@ -251,21 +224,24 @@ export class WinstonInstrumentation extends InstrumentationBase {
       };
     };
   }
-}
 
-function injectRecord(
-  spanContext: SpanContext,
-  record?: Record<string, string>
-) {
-  const fields = {
-    trace_id: spanContext.traceId,
-    span_id: spanContext.spanId,
-    trace_flags: `0${spanContext.traceFlags.toString(16)}`,
-  };
-
-  if (!record) {
-    return fields;
+  private _handleLogCorrelation(record: Record<string, string>) {
+    if (!this.getConfig().disableLogCorrelation) {
+      const span = trace.getSpan(context.active());
+      if (span) {
+        const spanContext = span.spanContext();
+        if (isSpanContextValid(spanContext)) {
+          const fields = {
+            trace_id: spanContext.traceId,
+            span_id: spanContext.spanId,
+            trace_flags: `0${spanContext.traceFlags.toString(16)}`,
+          };
+          const enhancedRecord = Object.assign(record, fields);
+          this._callHook(span, enhancedRecord);
+          return enhancedRecord;
+        }
+      }
+    }
+    return record;
   }
-
-  return Object.assign(record, fields);
 }

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

@@ -22,11 +22,11 @@ export type LogHookFunction = (span: Span, record: Record<string, any>) => void;
 
 export interface WinstonInstrumentationConfig extends InstrumentationConfig {
   /**
-   * Whether to disable the automatic sending of log records to the
+   * Whether to enable the automatic sending of log records to the
    * OpenTelemetry Logs SDK.
    * @default false
    */
-  disableLogSending?: boolean;
+  enableLogSending?: boolean;
 
   /**
    * Whether to disable the injection trace-context fields, and possibly other

plugins/node/opentelemetry-instrumentation-winston/test/winston.test.ts

@@ -200,6 +200,10 @@ describe('WinstonInstrumentation', () => {
 
     it('emit LogRecord', () => {
       if (!isWinston2) {
+        instrumentation.setConfig({
+          enableLogSending: true,
+        });
+        initLogger();
         const span = tracer.startSpan('abc');
         context.with(trace.setSpan(context.active(), span), () => {
           testEmitLogRecord(span);
@@ -209,6 +213,9 @@ describe('WinstonInstrumentation', () => {
 
     it('emit LogRecord with extra attibutes', () => {
       if (!isWinston2) {
+        instrumentation.setConfig({
+          enableLogSending: true,
+        });
         const extraAttributes = {
           extraAttribute1: 'attributeValue1',
           extraAttribute2: 'attributeValue2',
@@ -231,8 +238,7 @@ describe('WinstonInstrumentation', () => {
 
     it('does not emit LogRecord if config off', () => {
       instrumentation.setConfig({
-        enabled: true,
-        disableLogSending: true,
+        enableLogSending: false,
       });
       initLogger();
       const span = tracer.startSpan('abc');
@@ -320,6 +326,9 @@ describe('WinstonInstrumentation', () => {
 
   describe('emit logRecord severity', () => {
     beforeEach(() => {
+      instrumentation.setConfig({
+        enableLogSending: true,
+      });
       memoryLogExporter.getFinishedLogRecords().length = 0; // clear
     });