chore: Deprecate ActivityInboundLogInterceptor (#1284)

Author: mjameswh

packages/activity/src/index.ts

@@ -253,14 +253,17 @@ export class Context {
   /**
    * The logger for this Activity.
    *
-   * This defaults to the `Runtime`'s Logger (see {@link Runtime.logger}). If the {@link ActivityInboundLogInterceptor}
-   * is installed (by default, it is; see {@link WorkerOptions.interceptors}), then various attributes from the current
-   * Activity context will automatically be included as metadata on every log entries, and some key events of the
-   * Activity's life cycle will automatically be logged (at 'DEBUG' level for most messages; 'WARN' for failures).
+   * This defaults to the `Runtime`'s Logger (see {@link Runtime.logger}). Attributes from the current Activity context
+   * will automatically be included as metadata on every log entries, and some key events of the Activity's lifecycle
+   * will automatically be logged (at 'DEBUG' level for most messages; 'WARN' for failures).
    *
-   * To use a different Logger, either overwrite this property from an Activity Interceptor, or explicitly register the
-   * `ActivityInboundLogInterceptor` with your custom Logger. You may also subclass `ActivityInboundLogInterceptor` to
-   * customize attributes that are emitted as metadata.
+   * To customize log attributes, register a {@link ActivityOutboundCallsInterceptor} that intercepts the
+   * `getLogAttributes()` method.
+   *
+   * Modifying the context logger (eg. `context.log = myCustomLogger` or by an {@link ActivityInboundLogInterceptor}
+   * with a custom logger as argument) is deprecated. Doing so will prevent automatic inclusion of custom log attributes
+   * through the `getLogAttributes()` interceptor. To customize _where_ log messages are sent, set the
+   * {@link Runtime.logger} property instead.
    */
   public log: Logger;
 
@@ -274,13 +277,13 @@ export class Context {
     cancelled: Promise<never>,
     cancellationSignal: AbortSignal,
     heartbeat: (details?: any) => void,
-    logger: Logger
+    log: Logger
   ) {
     this.info = info;
     this.cancelled = cancelled;
     this.cancellationSignal = cancellationSignal;
     this.heartbeatFn = heartbeat;
-    this.log = logger;
+    this.log = log;
   }
 
   /**

packages/common/src/converter/failure-converter.ts

@@ -15,29 +15,42 @@ import {
 import { isError } from '../type-helpers';
 import { arrayFromPayloads, fromPayloadsAtIndex, PayloadConverter, toPayloads } from './payload-converter';
 
+function combineRegExp(...regexps: RegExp[]): RegExp {
+  return new RegExp(regexps.map((x) => `(?:${x.source})`).join('|'));
+}
+
 /**
  * Stack traces will be cutoff when on of these patterns is matched
  */
-const CUTOFF_STACK_PATTERNS = [
+const CUTOFF_STACK_PATTERNS = combineRegExp(
   /** Activity execution */
   /\s+at Activity\.execute \(.*[\\/]worker[\\/](?:src|lib)[\\/]activity\.[jt]s:\d+:\d+\)/,
   /** Workflow activation */
   /\s+at Activator\.\S+NextHandler \(.*[\\/]workflow[\\/](?:src|lib)[\\/]internals\.[jt]s:\d+:\d+\)/,
   /** Workflow run anything in context */
-  /\s+at Script\.runInContext \((?:node:vm|vm\.js):\d+:\d+\)/,
-];
+  /\s+at Script\.runInContext \((?:node:vm|vm\.js):\d+:\d+\)/
+);
+
+/**
+ * Any stack trace frames that match any of those wil be dopped.
+ * The "null." prefix on some cases is to avoid https://github.com/nodejs/node/issues/42417
+ */
+const DROPPED_STACK_FRAMES_PATTERNS = combineRegExp(
+  /** Internal functions used to recursively chain interceptors */
+  /\s+at (null\.)?next \(.*[\\/]common[\\/](?:src|lib)[\\/]interceptors\.[jt]s:\d+:\d+\)/,
+  /** Internal functions used to recursively chain interceptors */
+  /\s+at (null\.)?executeNextHandler \(.*[\\/]worker[\\/](?:src|lib)[\\/]activity\.[jt]s:\d+:\d+\)/
+);
 
 /**
  * Cuts out the framework part of a stack trace, leaving only user code entries
  */
 export function cutoffStackTrace(stack?: string): string {
   const lines = (stack ?? '').split(/\r?\n/);
   const acc = Array<string>();
-  lineLoop: for (const line of lines) {
-    for (const pattern of CUTOFF_STACK_PATTERNS) {
-      if (pattern.test(line)) break lineLoop;
-    }
-    acc.push(line);
+  for (const line of lines) {
+    if (CUTOFF_STACK_PATTERNS.test(line)) break;
+    if (!DROPPED_STACK_FRAMES_PATTERNS.test(line)) acc.push(line);
   }
   return acc.join('\n');
 }

packages/common/src/interceptors.ts

@@ -12,7 +12,10 @@ export type Next<IF, FN extends keyof IF> = Required<IF>[FN] extends AnyFunc ? O
 export type Headers = Record<string, Payload>;
 
 /**
- * Composes all interceptor methods into a single function
+ * Compose all interceptor methods into a single function.
+ *
+ * Calling the composed function results in calling each of the provided interceptor, in order (from the first to
+ * the last), followed by the original function provided as argument to `composeInterceptors()`.
  *
  * @param interceptors a list of interceptors
  * @param method the name of the interceptor method to compose

packages/test/src/helpers.ts

@@ -52,6 +52,8 @@ export function cleanStackTrace(ostack: string): string {
       .replace(/:\d+:\d+/g, '')
       .replace(/^\s*/gms, '    at ')
       .replace(/\[as fn\] /, '')
+      // Avoid https://github.com/nodejs/node/issues/42417
+      .replace(/at null\./g, 'at ')
       .replace(/\\/g, '/');
 
   return normalizedStack ? `${firstLine}\n${normalizedStack}` : firstLine;

packages/test/src/integration-tests.ts

@@ -34,7 +34,7 @@ import {
   WorkflowNotFoundError,
 } from '@temporalio/common';
 import { msToNumber, tsToMs } from '@temporalio/common/lib/time';
-import { decode, decodeFromPayloadsAtIndex, loadDataConverter } from '@temporalio/common/lib/internal-non-workflow';
+import { decode, decodeFromPayloadsAtIndex } from '@temporalio/common/lib/internal-non-workflow';
 import * as iface from '@temporalio/proto';
 import { appendDefaultInterceptors, DefaultLogger, makeTelemetryFilterString, Runtime } from '@temporalio/worker';
 import pkg from '@temporalio/worker/lib/pkg';
@@ -96,7 +96,7 @@ export function runIntegrationTests(codec?: PayloadCodec): void {
       taskQueue: 'test',
       dataConverter,
       interceptors: appendDefaultInterceptors({
-        activityInbound: [() => new ConnectionInjectorInterceptor(connection, loadDataConverter(dataConverter))],
+        activity: [() => ({ inbound: new ConnectionInjectorInterceptor(connection, loadedDataConverter) })],
       }),
       showStackTraceSources: true,
     });
@@ -222,7 +222,8 @@ export function runIntegrationTests(codec?: PayloadCodec): void {
       cleanOptionalStackTrace(err.cause.cause.stack),
       dedent`
     Error: Fail me
-        at Activity.throwAnError (test/src/activities/index.ts)
+        at throwAnError (test/src/activities/index.ts)
+        at ConnectionInjectorInterceptor.execute (test/src/activities/interceptors.ts)
     `
     );
   });
@@ -256,7 +257,8 @@ export function runIntegrationTests(codec?: PayloadCodec): void {
       dedent`
     ApplicationFailure: Fail me
         at Function.nonRetryable (common/src/failure.ts)
-        at Activity.throwAnError (test/src/activities/index.ts)
+        at throwAnError (test/src/activities/index.ts)
+        at ConnectionInjectorInterceptor.execute (test/src/activities/interceptors.ts)
       `
     );
   });

packages/test/src/load/worker.ts

@@ -182,7 +182,7 @@ async function main() {
       maxConcurrentActivityTaskPolls,
       maxConcurrentWorkflowTaskPolls,
       interceptors: {
-        activityInbound: [() => new ConnectionInjectorInterceptor(clientConnection)],
+        activity: [() => ({ inbound: new ConnectionInjectorInterceptor(clientConnection) })],
       },
       // Can't reuse the helper because it defines `test` and ava thinks it's an ava test.
       reuseV8Context: ['1', 't', 'true'].includes((process.env.REUSE_V8_CONTEXT ?? 'false').toLowerCase()),