feat: Prisma support #112

zermelo-wisen · 2024-02-09T20:35:41Z

Fixes #79

dustinbyrne · 2024-02-12T15:51:44Z

src/hooks/prisma.ts

+}
+
+prismaHook.applicable = function (id: string) {
+  return ["@prisma/client"].includes(id);


Can we configure this? We have internal services which share a pre-configured, pre-built Prisma client, shipped as its own module. Because of this, we don't ever import @prisma/client directly.

It's worth noting that I've tried this PR with our own service after modifying this line and it does work great. 👍

I'll make the module name (id) a configuration parameter, if I understand correctly.

dustinbyrne · 2024-02-12T16:02:50Z

src/hooks/prisma.ts

+        assert("$on" in thisArg && typeof thisArg.$on === "function");
+        thisArg.$on("query", (queryEvent: QueryEvent) => {
+          const call = recording.sqlQuery(dbType, queryEvent.query);
+          recording.functionReturn(call.id, undefined, queryEvent.duration);


I'd double check the unit of time here. I tried a request which issued three queries taking 2ms, 0ms, 1ms according to the logs. The AppMap states the duration was 2000ms, 0ms, 1000ms.

dividedmind

Great job figuring this out! Just a couple of changes I'd suggest before merging. A mention in the README about support for custom prisma client locations would be nice, too.

.yarn/cache/@next-swc-win32-x64-msvc-npm-14.0.4-e7cf0df5d6-8.zip

src/config.ts

src/hooks/prisma.ts

dividedmind · 2024-02-13T20:54:30Z

src/hooks/prisma.ts

+      generator: false,
+      id: action,
+      params: params,
+      location: { path: "unknown", lineno: -1 },


Let's generate location as @prisma/client/${model} (or custom client name, eg. appmap-prisma-client/${model}) — this will make it look much nicer in sequence diagrams. Also, the line numbers for different signatures should be different — I believe location is the key by which code objects are identified in the clients.

dividedmind · 2024-02-13T20:55:29Z

src/hooks/prisma.ts

+          const call = recording.sqlQuery(dbType, queryEvent.query);
+          const elapsedMs = queryEvent.duration / 1000.0;
+          recording.functionReturn(call.id, undefined, elapsedMs);
+        });


That's pretty clever :) Great job figuring this out!

dividedmind · 2024-02-13T20:56:58Z

src/hooks/prisma.ts

+        assert("$on" in thisArg && typeof thisArg.$on === "function");
+        thisArg.$on("query", (queryEvent: QueryEvent) => {
+          const call = recording.sqlQuery(dbType, queryEvent.query);
+          const elapsedMs = queryEvent.duration / 1000.0;


I believe this should be called elapsedSec — the doc says duration already is in ms.

dividedmind · 2024-02-13T21:00:53Z

src/hooks/prisma.ts

+const functionInfos = new Map<string, FunctionInfo>();
+
+function getFunctionInfo(model: string, action: string, params: ESTree.Parameter[]) {
+  const key = model + "." + action;


Is it possible for different calls to the same action to be parametrized differently? If so, this will create just one signature for all of them which will be correct only for the first one. In this case it might make more sense to pass them to the map as a single argument, or explode them all into (data, include, where) filling in undefined where one isn't provided, to avoid having multiple signatures for the same thing in the AppMap (note they would also need to differ in location because that's how the client code distinguishes code objects).

dividedmind · 2024-02-13T21:02:18Z

src/loader.ts

@@ -44,7 +44,8 @@ export const load: NodeLoaderHooksAPI2["load"] = async function load(url, contex
  // For these modules, we preempt import with CommonJS require
  // to allow our hooks to modify the loaded module in cache
  // (which is shared between ESM and CJS for builtins at least).
-  if (["node:http", "node:https", "http", "https"].includes(url)) forceRequire(url);
+  if (["node:http", "node:https", "http", "https", "@prisma/client"].includes(url))


This should take custom clients into account.

dividedmind · 2024-02-13T21:21:00Z

test/prisma/script.js

+
+main()
+  .then(async () => {
+    await prisma.$disconnect();


Doesn't it auto-disconnect? Maybe this then/catch is not necessary.

dividedmind · 2024-02-14T19:27:29Z

src/loader.ts

@@ -44,7 +45,8 @@ export const load: NodeLoaderHooksAPI2["load"] = async function load(url, contex
  // For these modules, we preempt import with CommonJS require
  // to allow our hooks to modify the loaded module in cache
  // (which is shared between ESM and CJS for builtins at least).
-  if (["node:http", "node:https", "http", "https"].includes(url)) forceRequire(url);
+  if (["node:http", "node:https", "http", "https", config.prismaClientModuleIds].includes(url))


Needs a splat.

Oh exactly, thank you.

dividedmind · 2024-02-14T19:29:20Z

test/prisma.test.ts

+
+integrationTest("mapping Prisma tests", () => {
+  const testDir = `${cwd()}/test/prisma`;
+  copyFileSync(`${testDir}/original.db`, `${testDir}/test.db`);


Is there a reason to copy this over instead of creating it in situ?

original.db consists of the required schema with empty tables. I didn't want to create it in every test run because creating it is not related to the test. test.db gets modified in every run so I wanted to provide a predefined db state for the test.

dividedmind · 2024-02-14T19:32:03Z

test/prisma.test.ts

+import { spawnSync } from "node:child_process";
+
+integrationTest("mapping Prisma tests", () => {
+  const testDir = `${cwd()}/test/prisma`;


That's not very robust — what if jest is run from a different directory? You can use resolveTarget() to get it.

dividedmind · 2024-02-14T19:35:28Z

src/hooks/prisma.ts

+              requestParams.action,
+              ["data", "include", "where"].map((k) => {
+                return { type: "Identifier", name: k } as ESTree.Identifier;
+              }),


Note this can be a constant in getFunctionInfo now.

appland-release · 2024-02-27T14:16:39Z

🎉 This PR is included in version 2.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

zermelo-wisen requested a review from dividedmind February 9, 2024 20:35

zermelo-wisen marked this pull request as draft February 10, 2024 08:31

zermelo-wisen force-pushed the feat/prisma-support branch from c392a63 to 17c06a5 Compare February 10, 2024 13:01

dustinbyrne self-requested a review February 12, 2024 15:20

dustinbyrne reviewed Feb 12, 2024

View reviewed changes

zermelo-wisen force-pushed the feat/prisma-support branch 3 times, most recently from ef55baa to 0a36fa0 Compare February 13, 2024 19:13

zermelo-wisen requested a review from dustinbyrne February 13, 2024 19:28

dividedmind requested changes Feb 13, 2024

View reviewed changes

zermelo-wisen force-pushed the feat/prisma-support branch 4 times, most recently from 1c00d6e to 13a2735 Compare February 14, 2024 13:00

zermelo-wisen requested a review from dividedmind February 14, 2024 13:48

dustinbyrne marked this pull request as ready for review February 14, 2024 15:19

dividedmind reviewed Feb 14, 2024

View reviewed changes

feat: Prisma support

1c1f24c

zermelo-wisen force-pushed the feat/prisma-support branch from 13a2735 to 1c1f24c Compare February 14, 2024 20:06

zermelo-wisen requested a review from dividedmind February 14, 2024 20:10

dividedmind approved these changes Feb 27, 2024

View reviewed changes

dividedmind merged commit 93242e1 into main Feb 27, 2024
6 checks passed

dividedmind deleted the feat/prisma-support branch February 27, 2024 14:14

appland-release added the released label Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Prisma support #112

feat: Prisma support #112

zermelo-wisen commented Feb 9, 2024

dustinbyrne Feb 12, 2024

dustinbyrne Feb 12, 2024 •

edited

Loading

zermelo-wisen Feb 12, 2024

dustinbyrne Feb 12, 2024

dividedmind left a comment

dividedmind Feb 13, 2024

dividedmind Feb 13, 2024

dividedmind Feb 13, 2024

dividedmind Feb 13, 2024

dividedmind Feb 13, 2024

dividedmind Feb 13, 2024

dividedmind Feb 14, 2024

zermelo-wisen Feb 14, 2024

dividedmind Feb 14, 2024

zermelo-wisen Feb 14, 2024

dividedmind Feb 14, 2024

dividedmind Feb 14, 2024

appland-release commented Feb 27, 2024

feat: Prisma support #112

feat: Prisma support #112

Conversation

zermelo-wisen commented Feb 9, 2024

Choose a reason for hiding this comment

dustinbyrne Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dividedmind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appland-release commented Feb 27, 2024

dustinbyrne Feb 12, 2024 •

edited

Loading