From c780cd4d90c83cfdba3b69a165b00cdb20addd86 Mon Sep 17 00:00:00 2001 From: Laurynas Keturakis Date: Wed, 12 Jul 2023 13:29:33 +0200 Subject: [PATCH 1/6] fix undefined "this" error for decorators --- packages/lib/src/wrappers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/lib/src/wrappers.ts b/packages/lib/src/wrappers.ts index a2b9ea17..93e28fa5 100644 --- a/packages/lib/src/wrappers.ts +++ b/packages/lib/src/wrappers.ts @@ -250,7 +250,7 @@ export function autometrics( function instrumentedFunction() { try { - const result = fn(...params); + const result = fn.apply(this, params); if (isPromise>(result)) { return result .then((res: Awaited>) => { From 08e9af219746864ac960389ea11c78250255c14a Mon Sep 17 00:00:00 2001 From: Laurynas Keturakis Date: Wed, 12 Jul 2023 14:49:10 +0200 Subject: [PATCH 2/6] refactor getModulePath so it doesn't require string parsing and gets file names for decorators correctly as well --- packages/lib/src/utils.ts | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/packages/lib/src/utils.ts b/packages/lib/src/utils.ts index a06de9cd..b4e16897 100644 --- a/packages/lib/src/utils.ts +++ b/packages/lib/src/utils.ts @@ -20,13 +20,20 @@ export function getRuntime(): Runtime { // HACK: this entire function is a hacky way to acquire the module name for a // given function e.g.: dist/index.js export function getModulePath(): string | undefined { - Error.stackTraceLimit = 5; - const stack = new Error()?.stack?.split("\n"); + const defaultPrepareStackTrace = Error.prepareStackTrace; + Error.prepareStackTrace = (error, stack) => stack; + // @ts-ignore: TS doesn't recognize that we've overwritten + // the default "stack" property + const { stack: stackConstructor }: { stack: NodeJS.CallSite[] } = new Error(); + Error.prepareStackTrace = defaultPrepareStackTrace; // we have to make sure to reset this to normal + + const stack = stackConstructor.map((callSite) => ({ + name: callSite.getFunctionName(), + file: callSite.getFileName(), + })); let rootDir: string; - const runtime = getRuntime(); - if (runtime === "browser") { rootDir = ""; } else if (runtime === "deno") { @@ -45,29 +52,28 @@ export function getModulePath(): string | undefined { } /** + * Finds the original wrapped function, first it checks if it's a decorator, + * and returns that filename or gets the 3th item of the stack trace: * * 0: Error - * 1: at getModulePath() ... - * 2: at autometrics() ... - * 3: at ... -> 4th line is always the original caller + * 1: at getModulePath ... + * 2: at autometrics ... + * 3: at ... -> 4th line is always the original wrapped function */ - const originalCaller = 3 as const; + const wrappedFunctionPath = + stack.find((call) => { + if (call.name === "__decorateClass") return true; + })?.file ?? stack[2]?.file; - // The last element in this array will have the full path - const fullPath = stack[originalCaller].split(" ").pop(); - - const containsFileProtocol = fullPath.includes("file://"); + const containsFileProtocol = wrappedFunctionPath.includes("file://"); // We split away everything up to the root directory of the project, // if the path contains file:// we need to remove it - let modulePath = fullPath.replace( + const modulePath = wrappedFunctionPath.replace( containsFileProtocol ? `file://${rootDir}` : rootDir, "", ); - // We split away the line and column numbers index.js:14:6 - modulePath = modulePath.substring(0, modulePath.indexOf(":")); - return modulePath; } From 9f8a78162a213bcf2d9827489c62e3e9faddb150 Mon Sep 17 00:00:00 2001 From: Laurynas Keturakis Date: Wed, 12 Jul 2023 14:49:13 +0200 Subject: [PATCH 3/6] add tests --- packages/lib/tests/integration.test.ts | 37 +++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/packages/lib/tests/integration.test.ts b/packages/lib/tests/integration.test.ts index d8c010d6..c30c8029 100644 --- a/packages/lib/tests/integration.test.ts +++ b/packages/lib/tests/integration.test.ts @@ -1,4 +1,4 @@ -import { autometrics, init } from "../src"; +import { Autometrics, autometrics, init } from "../src"; import { describe, test, expect, beforeAll, afterEach } from "vitest"; import { AggregationTemporality, @@ -57,4 +57,39 @@ describe("Autometrics integration test", () => { expect(serialized).toMatch(errorCountMetric); }); + + test("class method", async () => { + const callCountMetric = + /function_calls_count_total\{\S*function="helloWorld"\S*module="\/packages\/lib\/tests\/integration.test.ts"\S*\} 2/gm; + const durationMetric = + /function_calls_duration_bucket\{\S*function="helloWorld"\S*module="\/packages\/lib\/tests\/integration.test.ts"\S*\}/gm; + + // @Autometrics decorator is likely to be used along-side other decorators + // this tests for any conflicts + function Bar() { + return function Bar( + target: Object, + propertyKey: string, + descriptor: PropertyDescriptor, + ) { + const originalMethod = descriptor.value; + descriptor.value = originalMethod; + }; + } + + @Autometrics() + class Foo { + @Bar() + helloWorld() {} + } + + const foo = new Foo(); + foo.helloWorld(); + foo.helloWorld(); + + const serialized = await collectAndSerialize(exporter); + + expect(serialized).toMatch(callCountMetric); + expect(serialized).toMatch(durationMetric); + }); }); From 361e79463ff7696427c533ea7d43fd462ef30548 Mon Sep 17 00:00:00 2001 From: Laurynas Keturakis Date: Wed, 12 Jul 2023 15:12:32 +0200 Subject: [PATCH 4/6] update changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddc0717d..74e1b0e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## [Unreleased] + +- Fixed an issue where decorators were changing `this` values for the methods they'd be wrapping, breaking them. +- Improved how `getModulePath` utility works, passing stack trace as structured data, and making it more robust. + ## [v0.5.3] - 2023-06-26 ### Changed From 042ea1d324044e51da412a9095d9cc2d1440a38d Mon Sep 17 00:00:00 2001 From: Laurynas Keturakis Date: Wed, 12 Jul 2023 15:31:09 +0200 Subject: [PATCH 5/6] use typecasting instead of ts-ignore --- packages/lib/src/utils.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/lib/src/utils.ts b/packages/lib/src/utils.ts index b4e16897..f26a3057 100644 --- a/packages/lib/src/utils.ts +++ b/packages/lib/src/utils.ts @@ -21,10 +21,10 @@ export function getRuntime(): Runtime { // given function e.g.: dist/index.js export function getModulePath(): string | undefined { const defaultPrepareStackTrace = Error.prepareStackTrace; - Error.prepareStackTrace = (error, stack) => stack; - // @ts-ignore: TS doesn't recognize that we've overwritten - // the default "stack" property - const { stack: stackConstructor }: { stack: NodeJS.CallSite[] } = new Error(); + Error.prepareStackTrace = (_, stack) => stack; + const { stack: stackConstructor } = new Error() as Error & { + stack: NodeJS.CallSite[]; + }; Error.prepareStackTrace = defaultPrepareStackTrace; // we have to make sure to reset this to normal const stack = stackConstructor.map((callSite) => ({ From 537883913f4676f809c33152abcd184981b6b338 Mon Sep 17 00:00:00 2001 From: Laurynas Keturakis Date: Wed, 12 Jul 2023 15:33:30 +0200 Subject: [PATCH 6/6] return values immediately --- packages/lib/src/utils.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/lib/src/utils.ts b/packages/lib/src/utils.ts index f26a3057..48880fbb 100644 --- a/packages/lib/src/utils.ts +++ b/packages/lib/src/utils.ts @@ -69,12 +69,10 @@ export function getModulePath(): string | undefined { // We split away everything up to the root directory of the project, // if the path contains file:// we need to remove it - const modulePath = wrappedFunctionPath.replace( + return wrappedFunctionPath.replace( containsFileProtocol ? `file://${rootDir}` : rootDir, "", ); - - return modulePath; } type ALSContext = {