fix: Instrument based on source map paths

package.json

@@ -107,6 +107,7 @@
     "test/next",
     "test/typescript-esm",
     "test/prisma",
-    "test/libraryCalls"
+    "test/libraryCalls",
+    "test/sourceMapPath"
   ]
 }

src/hooks/instrument.ts

@@ -1,5 +1,5 @@
 import assert from "node:assert";
-import { fileURLToPath } from "node:url";
+import { fileURLToPath, pathToFileURL } from "node:url";
 import { debuglog } from "node:util";
 
 import { ancestor as walk } from "acorn-walk";
@@ -23,6 +23,7 @@ import { FunctionInfo, SourceLocation, createFunctionInfo, createMethodInfo } fr
 import findLast from "../util/findLast";
 import { isId } from "../util/isId";
 import CommentLabelExtractor from "./util/CommentLabelExtractor";
+import { CustomSourceMapConsumer, shouldMatchOriginalSourcePaths } from "../transform";
 
 const debug = debuglog("appmap:instrument");
 
@@ -36,37 +37,64 @@ function addTransformedFunctionInfo(fi: FunctionInfo): number {
 
 export function transform(
   program: ESTree.Program,
-  sourceMap?: SourceMapConsumer,
+  sourceMap?: CustomSourceMapConsumer,
   comments?: ESTree.Comment[],
 ): ESTree.Program {
   transformedFunctionInfos.splice(0);
-  const source = program.loc?.source;
-  const pkg = source ? config().packages.match(source) : undefined;
 
   const locate = makeLocator(sourceMap);
   const commentLabelExtractor = comments
     ? new CommentLabelExtractor(comments, sourceMap)
     : undefined;
 
-  const getFunctionLabels = (name: string, line: number, klass?: string) => {
-    const commentLabels = commentLabelExtractor?.labelsFor(line);
-    const configLabels = config.getFunctionLabels(pkg, name, klass);
+  const getFunctionLabels = (name: string, location: SourceLocation, klass?: string) => {
+    const commentLabels = commentLabelExtractor?.labelsFor(location.lineno);
+    const pkg = config().packages.match(location.path);
+    const configLabels = config().getFunctionLabels(pkg, name, klass);
     if (commentLabels && configLabels)
       return [...commentLabels, ...configLabels.filter((l) => !commentLabels.includes(l))];
     return commentLabels ?? configLabels;
   };
 
+  const originalSourceShouldBeInstrumented = new Map<string, boolean>();
+  const shouldSkipFunction = (
+    location: SourceLocation | undefined,
+    name: string,
+    qname?: string,
+  ) => {
+    if (!location) return true; // don't instrument generated code
+
+    // A function may reside in a bundled file. If we have a source map
+    // we check if the original source of the function should be instrumented.
+    if (sourceMap?.originalSources.length) {
+      if (!originalSourceShouldBeInstrumented.has(location.path)) {
+        const url = pathToFileURL(location.path);
+        const originalSource = sourceMap.originalSources.find((s) => s.href == url.href);
+        originalSourceShouldBeInstrumented.set(
+          location.path,
+          originalSource != undefined && shouldInstrument(originalSource),
+        );
+      }
+      if (!originalSourceShouldBeInstrumented.get(location.path)) return true;
+    }
+
+    // check if the function is explicitly excluded in config
+    const pkg = config().packages.match(location.path);
+    if (pkg?.exclude?.includes(name)) return true;
+    if (qname && pkg?.exclude?.includes(qname)) return true;
+  };
+
   walk(program, {
     FunctionDeclaration(fun: ESTree.FunctionDeclaration) {
       if (!hasIdentifier(fun)) return;
-      if (pkg?.exclude?.includes(fun.id.name)) return;
 
       const location = locate(fun);
-      if (!location) return; // don't instrument generated code
+      if (shouldSkipFunction(location, fun.id.name)) return;
+      assert(location);
 
       fun.body = wrapFunction(
         fun,
-        createFunctionInfo(fun, location, getFunctionLabels(fun.id.name, location.lineno)),
+        createFunctionInfo(fun, location, getFunctionLabels(fun.id.name, location)),
         false,
       );
     },
@@ -81,25 +109,14 @@ export function transform(
       const { name } = method.key;
       const qname = [klass.id.name, name].join(".");
 
-      // Not sure why eslint complains here, ?? is the wrong operator
-      // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
-      if (pkg?.exclude?.includes(name) || pkg?.exclude?.includes(qname)) {
-        debug(`Excluding ${qname}`);
-        return;
-      }
-
       const location = locate(method);
-      if (!location) return; // don't instrument generated code
+      if (shouldSkipFunction(location, name, qname)) return;
+      assert(location);
 
       debug(`Instrumenting ${qname}`);
       method.value.body = wrapFunction(
         { ...method.value },
-        createMethodInfo(
-          method,
-          klass,
-          location,
-          getFunctionLabels(name, location.lineno, klass.id.name),
-        ),
+        createMethodInfo(method, klass, location, getFunctionLabels(name, location, klass.id.name)),
         method.kind === "constructor",
       );
     },
@@ -116,7 +133,10 @@ export function transform(
           if (!(declaration.type === "VariableDeclaration" && declaration.kind === "const")) return;
           const { id } = declarator;
           if (!isId(id)) return;
-          if (pkg?.exclude?.includes(id.name)) return;
+
+          if (shouldSkipFunction(location, id.name)) return;
+          assert(location);
+
           assert(declarator.init === fun);
 
           debug(`Instrumenting ${id.name}`);
@@ -125,23 +145,27 @@ export function transform(
             createFunctionInfo(
               { ...fun, id, generator: false },
               location,
-              getFunctionLabels(id.name, location.lineno),
+              getFunctionLabels(id.name, location),
             ),
           );
           break;
         }
         // instrument CommonJS exports
         case "AssignmentExpression": {
           const id = exportName(declarator.left);
-          if (!id || pkg?.exclude?.includes(id.name)) return;
+
+          if (!id) return;
+
+          if (shouldSkipFunction(location, id.name)) return;
+          assert(location);
 
           debug(`Instrumenting ${id.name}`);
           declarator.right = wrapLambda(
             fun,
             createFunctionInfo(
               { ...fun, id, generator: false },
               location,
-              getFunctionLabels(id.name, location.lineno),
+              getFunctionLabels(id.name, location),
             ),
           );
           break;
@@ -265,14 +289,26 @@ function wrapFunction(
   return wrapped;
 }
 
-export function shouldInstrument(url: URL): boolean {
+function shouldInstrument_(url: URL) {
   if (url.protocol !== "file:") return false;
   if (url.pathname.endsWith(".json")) return false;
 
   const filePath = fileURLToPath(url);
   return !!config().packages.match(filePath);
 }
 
+// If there is a source map, check whether there is at least one original source
+// that needs to be instrumented. If so, we should send the "bundled" URL to
+// instrument.transform(), but within it, we should check each function to determine
+// if its original source URL needs to be instrumented.
+export function shouldInstrument(url: URL, sourceMap?: CustomSourceMapConsumer): boolean {
+  if (sourceMap?.originalSources.length && shouldMatchOriginalSourcePaths(url))
+    return sourceMap.originalSources.some((s) => shouldInstrument_(s));
+
+  const result = shouldInstrument_(url);
+  return result;
+}
+
 function hasIdentifier(
   fun: ESTree.FunctionDeclaration,
 ): fun is ESTree.FunctionDeclaration & { id: ESTree.Identifier } {

src/transform.ts

@@ -1,6 +1,6 @@
 import assert from "node:assert";
 import { readFileSync, writeFileSync } from "node:fs";
-import { URL, fileURLToPath } from "node:url";
+import { URL, fileURLToPath, pathToFileURL } from "node:url";
 import { debuglog } from "node:util";
 import { isNativeError } from "node:util/types";
 
@@ -20,23 +20,49 @@ const treeDebug = debuglog("appmap-tree");
 const sourceDebug = debuglog("appmap-source");
 
 export interface Hook {
-  shouldInstrument(url: URL): boolean;
+  shouldInstrument(url: URL, sourcemap?: CustomSourceMapConsumer): boolean;
   transform(
     program: ESTree.Program,
-    sourcemap?: SourceMapConsumer,
+    sourcemap?: CustomSourceMapConsumer,
     comments?: ESTree.Comment[],
   ): ESTree.Program;
   shouldIgnore?(url: URL): boolean;
 }
 
 const defaultHooks: Hook[] = [next, vitest, mocha, jest, instrument];
 
-export function findHook(url: URL, hooks = defaultHooks) {
-  return hooks.find((h) => h.shouldInstrument(url));
+export function findHook(url: URL, sourceMap?: CustomSourceMapConsumer, hooks = defaultHooks) {
+  return hooks.find((h) => h.shouldInstrument(url, sourceMap));
+}
+
+export function shouldMatchOriginalSourcePaths(url: URL) {
+  // We're not interested in source maps of libraries. For instance, in a Next.js project,
+  // if we were to consult the source map for instrumentation of a file like
+  // "...node_modules/next/dist/compiled/next-server/pages.runtime.dev.js", we would get
+  // URLs like "file:///...webpack:/next/dist/compiled/cookie/index.js" that do not include
+  // the node_modules part, resulting in unintended instrumentation, since node_modules
+  // folder is typically excluded from instrumentation.
+  return !["/node_modules/", "/.next/"].some((x) => url.pathname.includes(x));
 }
 
 export default function transform(code: string, url: URL, hooks = defaultHooks): string {
-  const hook = findHook(url, hooks);
+  let sourceMap: CustomSourceMapConsumer | undefined;
+  let sourceMapInitialized = false;
+  const getSourceMap_ = () => {
+    if (!sourceMapInitialized) {
+      try {
+        sourceMap = getSourceMap(url, code);
+      } catch (e) {
+        warn("Error getting source map for ", url, e);
+      }
+      sourceMapInitialized = true;
+    }
+    return sourceMap;
+  };
+
+  if (shouldMatchOriginalSourcePaths(url)) getSourceMap_();
+
+  const hook = findHook(url, sourceMap, hooks);
   if (!hook) return code;
 
   if (hooks.some((h) => h.shouldIgnore?.(url))) return code;
@@ -51,7 +77,7 @@ export default function transform(code: string, url: URL, hooks = defaultHooks):
       onComment: comments,
     });
 
-    const xformed = hook.transform(tree, getSourceMap(url, code), comments);
+    const xformed = hook.transform(tree, getSourceMap_(), comments);
     if (treeDebug.enabled) dumpTree(xformed, url);
     const src = generate(xformed);
     if (sourceDebug.enabled) {
@@ -75,7 +101,15 @@ function dumpTree(xformed: ESTree.Program, url: URL) {
   treeDebug("wrote transformed tree to %s", path);
 }
 
-function getSourceMap(fileUrl: URL, code: string): SourceMapConsumer | undefined {
+export class CustomSourceMapConsumer extends SourceMapConsumer {
+  public originalSources: URL[];
+  constructor(rawSourceMap: RawSourceMap) {
+    super(rawSourceMap);
+    this.originalSources = rawSourceMap.sources.map((s) => pathToFileURL(s));
+  }
+}
+
+function getSourceMap(fileUrl: URL, code: string): CustomSourceMapConsumer | undefined {
   const sourceMappingUrl = code.match(/\/\/# sourceMappingURL=(.*)/)?.[1];
   if (!sourceMappingUrl) return;
 
@@ -96,11 +130,18 @@ function getSourceMap(fileUrl: URL, code: string): SourceMapConsumer | undefined
   }
 
   sourceMap.sources = sourceMap.sources.map((source) => {
-    const url = new URL((sourceMap.sourceRoot ?? "") + source, fileUrl);
+    const rootedSource = (sourceMap.sourceRoot ?? "") + source;
+    // On Windows, we get incorrect result with URL if the original path contains spaces.
+    //   source: 'C:/Users/John Doe/projects/appmap-node/test/typescript/index.ts'
+    //   => result: 'C:/Users/John%20Doe/projects/appmap-node/test/typescript/index.ts'
+    // This check prevents it.
+    if (rootedSource.match(/^[a-zA-Z]:[/\\]/)) return rootedSource;
+
+    const url = new URL(rootedSource, fileUrl);
     return url.protocol === "file:" ? fileURLToPath(url) : url.toString();
   });
 
-  return new SourceMapConsumer(sourceMap);
+  return new CustomSourceMapConsumer(sourceMap);
 }
 
 function parseDataUrl(fileUrl: URL) {

test/__snapshots__/sourceMapPath.test.ts.snap

@@ -0,0 +1,71 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`mapping a bundled script 1`] = `
+{
+  "classMap": [
+    {
+      "children": [
+        {
+          "children": [
+            {
+              "location": "original.mjs:3",
+              "name": "foo",
+              "static": true,
+              "type": "function",
+            },
+          ],
+          "name": "original",
+          "type": "class",
+        },
+      ],
+      "name": "appmap-node",
+      "type": "package",
+    },
+  ],
+  "events": [
+    {
+      "defined_class": "original",
+      "event": "call",
+      "id": 1,
+      "lineno": 3,
+      "method_id": "foo",
+      "parameters": [
+        {
+          "class": "String",
+          "name": "msg",
+          "value": "'hello'",
+        },
+      ],
+      "path": "original.mjs",
+      "static": true,
+      "thread_id": 0,
+    },
+    {
+      "elapsed": 31.337,
+      "event": "return",
+      "id": 2,
+      "parent_id": 1,
+      "thread_id": 0,
+    },
+  ],
+  "metadata": {
+    "app": "appmap-node",
+    "client": {
+      "name": "appmap-node",
+      "url": "https://github.com/getappmap/appmap-node",
+      "version": "test node-appmap version",
+    },
+    "language": {
+      "engine": "Node.js",
+      "name": "javascript",
+      "version": "test node version",
+    },
+    "name": "test process recording",
+    "recorder": {
+      "name": "process",
+      "type": "process",
+    },
+  },
+  "version": "1.12",
+}
+`;

test/sourceMapPath.test.ts

@@ -0,0 +1,6 @@
+import { integrationTest, readAppmap, runAppmapNode } from "./helpers";
+
+integrationTest("mapping a bundled script", () => {
+  expect(runAppmapNode("./built/index.js").status).toBe(0);
+  expect(readAppmap()).toMatchSnapshot();
+});

test/sourceMapPath/README.md

@@ -0,0 +1,5 @@
+To recreate index.js & index.js.map:
+
+```
+$ yarn esbuild original.mjs --bundle --platform=node --sourcemap --outfile=built/index.js
+```