feat: Code block recording #115

zermelo-wisen · 2024-02-20T17:39:41Z

Fixes #82

dividedmind

Block recording should work like this:

import { record } from 'appmap-node';

const appmap = record(() => {
  hello("world");
});

Note you don't need to add a flag — just have a separate façade (NOT bin.js) for importing^Wrequiring (use package.json to direct require/import to the right file) and drop the process recording when it's loaded. You'll also need to expose the AppMap type so the clients can consume the results with confidence.

You probably don't need an extra recordAsync flavor — just do async recording if the function is async and/or returns a promise.

dividedmind

Can you please also split the two refactorings (Appmap.d.ts -> AppMap.ts, emit()) into separate commits with messages explaining the rationales? It'll make it much clearer what's going on when someone reads it down the line. Thanks!

dividedmind · 2024-03-15T15:23:41Z

src/Recording.ts

@@ -212,6 +212,11 @@ export default class Recording {
  get running(): boolean {
    return !!this.stream;
  }
+
+  readAppMap(): AppMap.AppMap | undefined {


When does this return undefined?

dividedmind · 2024-03-15T15:24:20Z

src/facade.ts

+    block();
+  } finally {
+    stopCodeBlockRecording();
+    recording.finish();


Should probably say if (!recording.finish()) return.

src/facade.ts

dividedmind · 2024-03-15T15:40:49Z

test/codeBlock/index.js

+
+  assert(appmap1.events.filter((e) => e.event == "call").length == 1);
+
+  const appmap2 = await recordAsync(() => {


This should probably be an async function, right?

dividedmind · 2024-03-15T15:41:19Z

test/codeBlock/package.json

@@ -0,0 +1 @@
+{}


Why is this needed?

dividedmind · 2024-03-15T15:42:23Z

src/facade.ts

+disableGlobalRecording();
+
+export function record(block: () => unknown): AppMap.AppMap | undefined {
+  start(new Recording("block", "block", new Date().toISOString()));


The record function should probably check if the code is instrumented and throw otherwise, telling the user to run with appmap-node. Otherwise it will just mysteriously fail to produce anything. (Add a test case for that!)

dividedmind

Thanks, looks good! Is this ready for the final review?

src/Recording.ts

dividedmind · 2024-03-26T06:27:43Z

src/facade.ts

+  startCodeBlockRecording();
+  try {
+    const result = block();
+    if (result instanceof Promise) return result.then(() => finishRecording(), finishRecording);


Actually I realized it's better to use isPromise; I believe plain instanceof can return misleading results if the promise is from another realm.

dividedmind · 2024-03-26T06:36:26Z

src/Recording.ts

+    // Check here if we should record instead of requiring each
+    // possible hook to check it.
+    // Checking this inside hooks too, will save CPU cycles only
+    // by preventing unnecessary event creation.


This comment should mention that it's also checked in record — event creation can be expensive (especially stringification).

I made a change to prevent expensive event creation, with an additional commit: perf: Prevent unnecessary event object creation.

I know it complicated the hooks a bit, but returning undefined from Recording event creation methods when we should not record, forces the hook implementations (sqllite, prisma, pg, mysql, mongo, http...) to deal with it in compile time, thanks to TypeScript. Other option would be to expect them to check shouldRecord in every Recording event creation method call, which again would lead to similar changes.

I think it was already fine as it was — record() method already checked if shouldRecord(); I just thought it would be good to say that in this comment :)

Let's roll back that perf commit and keep it simple. There will be some wasted events but most of them will be short-circuited by the check in record().

Just to clarify, all the hooks which call event creation methods directly (sqlite, prisma, pg, mysql, mongo, http) will create the event objects, but the events will not be emitted when we should not record. I believe you meant these by some wasted events.

perf commit rolled back.

Just to clarify, all the hooks which call event creation methods directly (sqlite, prisma, pg, mysql, mongo, http) will create the event objects, but the events will not be emitted when we should not record. I believe you meant these by some wasted events.

Yeah, that's fine, I was more worried about the normal function calls which occur much more often, but the test in record() takes care of these.

- Renamed AppMap.d.ts to AppMap.ts to get the declaration file created automatically and put into dist in packing. - Removed namespace declaration inside AppMap.d.ts to expose the types inside to the clients. - Changed internal importers' import declarations to import type * as AppMap from ./AppMap so they can use the types with AppMap qualifier as before.

assert(this.stream); and this.stream.emit(event); statements extracted as Recording.emit(event) method.

appland-release · 2024-04-19T13:57:10Z

🎉 This PR is included in version 2.20.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

zermelo-wisen requested a review from dividedmind February 20, 2024 18:08

dividedmind requested changes Feb 29, 2024

View reviewed changes

zermelo-wisen force-pushed the feat/code-block-recording branch 3 times, most recently from 5911f61 to 870b524 Compare March 6, 2024 11:18

zermelo-wisen requested a review from dividedmind March 6, 2024 13:07

dividedmind requested changes Mar 15, 2024

View reviewed changes

zermelo-wisen force-pushed the feat/code-block-recording branch from 870b524 to 024a72f Compare March 19, 2024 11:19

dividedmind reviewed Mar 26, 2024

View reviewed changes

zermelo-wisen force-pushed the feat/code-block-recording branch 2 times, most recently from 9c3cc14 to c6256cc Compare March 26, 2024 18:06

zermelo-wisen requested a review from dividedmind March 27, 2024 07:03

zermelo-wisen added 3 commits March 28, 2024 11:37

refactor: Eliminate emit related repeated code in Recording.ts

4db1d13

assert(this.stream); and this.stream.emit(event); statements extracted as Recording.emit(event) method.

feat: Code block recording

df39f93

zermelo-wisen force-pushed the feat/code-block-recording branch from c6256cc to df39f93 Compare March 29, 2024 07:48

dividedmind approved these changes Apr 3, 2024

View reviewed changes

dividedmind merged commit 9e1048a into main Apr 3, 2024
6 checks passed

dividedmind deleted the feat/code-block-recording branch April 3, 2024 13:05

appland-release added the released label Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Code block recording #115

feat: Code block recording #115

zermelo-wisen commented Feb 20, 2024

dividedmind left a comment

dividedmind left a comment

dividedmind Mar 15, 2024

dividedmind Mar 15, 2024

dividedmind Mar 15, 2024

dividedmind Mar 15, 2024

dividedmind Mar 15, 2024

dividedmind left a comment

dividedmind Mar 26, 2024

dividedmind Mar 26, 2024

zermelo-wisen Mar 26, 2024

dividedmind Mar 27, 2024 •

edited

Loading

zermelo-wisen Mar 28, 2024

zermelo-wisen Mar 29, 2024

dividedmind Apr 3, 2024

appland-release commented Apr 19, 2024


		assert(appmap1.events.filter((e) => e.event == "call").length == 1);

		const appmap2 = await recordAsync(() => {

feat: Code block recording #115

feat: Code block recording #115

Conversation

zermelo-wisen commented Feb 20, 2024

dividedmind left a 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

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

dividedmind Mar 27, 2024 • edited Loading

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 Apr 19, 2024

dividedmind Mar 27, 2024 •

edited

Loading