From e99954cde6b161a945f1a04200b476443c59fa1b Mon Sep 17 00:00:00 2001 From: James Sumners Date: Tue, 19 Aug 2025 15:14:51 -0400 Subject: [PATCH 01/10] Added support for traceCallback --- src/function_query.rs | 8 +++++- src/instrumentation.rs | 56 +++++++++++++++++++++++++------------- tests/Readme.md | 47 ++++++++++++++++++++++++++++++++ tests/callback_cjs/mod.js | 12 ++++++++ tests/callback_cjs/mod.rs | 24 ++++++++++++++++ tests/callback_cjs/test.js | 46 +++++++++++++++++++++++++++++++ tests/common/mod.rs | 30 +++++++++++++++++++- tests/instrumentor_test.rs | 1 + 8 files changed, 203 insertions(+), 21 deletions(-) create mode 100644 tests/Readme.md create mode 100644 tests/callback_cjs/mod.js create mode 100644 tests/callback_cjs/mod.rs create mode 100644 tests/callback_cjs/test.js diff --git a/src/function_query.rs b/src/function_query.rs index e8e7a4a..cef3599 100644 --- a/src/function_query.rs +++ b/src/function_query.rs @@ -17,6 +17,7 @@ pub(crate) enum FunctionType { pub enum FunctionKind { Sync, Async, + Callback } impl FunctionKind { @@ -24,12 +25,17 @@ impl FunctionKind { pub fn is_async(&self) -> bool { matches!(self, FunctionKind::Async) } + + pub fn is_callback(&self) -> bool { + matches!(self, FunctionKind::Callback) + } #[must_use] pub fn tracing_operator(&self) -> &'static str { match self { FunctionKind::Sync => "traceSync", FunctionKind::Async => "tracePromise", + FunctionKind::Callback => "traceCallback" } } } @@ -43,7 +49,7 @@ impl FunctionKind { #[derive(Debug, Clone)] pub enum FunctionQuery { // The order here matters because this enum is untagged, serde will try - // choose the first variant that matches the data. + // to choose the first variant that matches the data. ClassMethod { class_name: String, method_name: String, diff --git a/src/instrumentation.rs b/src/instrumentation.rs index 984767d..3218779 100644 --- a/src/instrumentation.rs +++ b/src/instrumentation.rs @@ -119,7 +119,8 @@ impl Instrumentation { ctxt: SyntaxContext::empty(), stmts: vec![ quote!("const __apm$wrapped = $wrapped;" as Stmt, wrapped: Expr = wrapped_fn.into()), - quote!("return __apm$wrapped.apply(null, __apm$original_args);" as Stmt), + // TODO: instead of a static `this` we should be able to declare the value + quote!("return __apm$wrapped.apply(this, __apm$original_args);" as Stmt), ], }; @@ -127,11 +128,11 @@ impl Instrumentation { let id_name = self.config.get_identifier_name(); let ch_ident = ident!(format!("tr_ch_apm${}", &id_name)); - let trace_ident = ident!(format!( - "tr_ch_apm${}.{}", + let trace_statement = construct_trace_statement( + &self.config, &id_name, - self.config.function_query.kind().tracing_operator() - )); + (&self.module_version).clone().unwrap_or(String::new()) + ); body.stmts = vec![ quote!("const __apm$original_args = arguments" as Stmt), @@ -140,18 +141,7 @@ impl Instrumentation { "if (!$ch.hasSubscribers) return __apm$traced();" as Stmt, ch = ch_ident ), - match &self.module_version { - Some(version) => quote!( - "return $trace(__apm$traced, { arguments, self: this, moduleVersion: $version } );" - as Stmt, - trace = trace_ident, - version: Expr = version.as_str().into(), - ), - None => quote!( - "return $trace(__apm$traced, { arguments, self: this } );" as Stmt, - trace = trace_ident, - ), - }, + trace_statement ]; self.has_injected = true; @@ -235,7 +225,7 @@ impl Instrumentation { } // The rest of these functions are from `VisitMut`, except they return a boolean to indicate - // whether recusrsing through the tree is necessary, rather than calling + // whether recursing through the tree is necessary, rather than calling // `visit_mut_children_with`. pub fn visit_mut_module(&mut self, node: &mut Module) -> bool { @@ -356,7 +346,7 @@ impl Instrumentation { pub fn visit_mut_assign_expr(&mut self, node: &mut AssignExpr) -> bool { // TODO(bengl) This is by far the hardest bit. We're trying to infer a name for this - // function expresion using the surrounding code, but it's not always possible, and even + // function expression using the surrounding code, but it's not always possible, and even // where it is, there are so many ways to give a function expression a "name", that the // code paths here can get pretty hairy. Right now this is only covering some basic cases. // The following cases are missing: @@ -398,3 +388,31 @@ pub fn get_script_start_index(script: &Script) -> usize { } 0 } + +/// Builds a tracing channel trace invocation. For example, the result may +/// look like: +/// +/// ```js +/// return tr_ch_amp$myChannel.traceSync(__apm$traced, { arguments, self: this, moduleVersion: "1.0.0" }) +/// ``` +fn construct_trace_statement(config: &InstrumentationConfig, channel_name: &String, mod_version: String) -> Stmt { + let mut ctx = "{ arguments, self: this }".to_string(); + if mod_version.len() > 0 { + ctx = ["{ arguments, self: this, moduleVersion: \"", mod_version.as_str(), "\" }"].join(""); + } + + let operator = ["tr_ch_apm$", channel_name, ".", config.function_query.kind().tracing_operator()].join(""); + let stmt_str; + if config.function_query.kind().is_callback() == true { + // TODO: read callback position from configuration + // TODO: figure out how we can pass a `this` argument + stmt_str = ["return ", &*operator, "(__apm$traced, -1, ", ctx.as_str(), ", this, ...arguments)"].join(""); + } else { + stmt_str = ["return ", &*operator, "(__apm$traced, ", ctx.as_str(), ")"].join(""); + } + + return quote!( + "$stmt" as Stmt, + stmt = Ident::new(Atom::from(stmt_str), Span::default(), SyntaxContext::empty()) + ) +} \ No newline at end of file diff --git a/tests/Readme.md b/tests/Readme.md new file mode 100644 index 0000000..b46d9cb --- /dev/null +++ b/tests/Readme.md @@ -0,0 +1,47 @@ +# Unit Tests + +Unit tests in this project follow a specific structure: + +1. A directory named the same as the unit test. For example, +the `index_cjs` test will be within the `index_cjs/` directory. +2. A `mod.rs` file that contains the unit test. +3. A `mod.{js,mjs}` file that contains a JavaScript script to apply +the test against, i.e. this replicates a potential module from +npmjs.com. +4. A `test.js` file that exercises the patched code. +5. A new `mod` line in the [`instrumentor_test.rs`](./instrumentor_test.rs) +file. + +To run a specific test from the command line: + +```sh +# To run the index_cjs test only: +cargo test index_cjs +``` + +Each unit test should utilize the `transpile_and_test` function +exported by [`common/mod.rs`](./common/mod.rs). This function: + +1. Reads in the unit test file to get the base path to the test. +2. Reads in the JavaScript module file. +3. Transforms the read-in JavaScript according to the defined configuration. +4. Generates a new `instrumented.js` file within the test directory. This +file contains the `mod.{js,mjs}` code patched by our tool. +5. Runs `node test.js` within the test directory. The result of this test +should be a `0` exit code for the process. + +## Running In A Debugger + +### RustRover + +To run a specific test through the [RustRover](https://www.jetbrains.com/rust/) +debugger: + +1. Create a new run configuration of type "Cargo". +2. For the "command", enter (e.g. for the `index_cjs` test): + + test --package orchestrion-js --test instrumentor_test index_cjs::index_cjs -- --exact +3. Set breakpoints and run the profile. + +For a shortcut, open the desired `mod.rs` test file and click the "run" +button in the gutter. \ No newline at end of file diff --git a/tests/callback_cjs/mod.js b/tests/callback_cjs/mod.js new file mode 100644 index 0000000..171f50d --- /dev/null +++ b/tests/callback_cjs/mod.js @@ -0,0 +1,12 @@ +'use strict' + +module.exports = Foo + +function Foo() {} + +Foo.prototype.doWork = function doWork(input, callback) { + if (input === 'fail') { + return callback(Error('boom')) + } + return callback(null, 'done') +} \ No newline at end of file diff --git a/tests/callback_cjs/mod.rs b/tests/callback_cjs/mod.rs new file mode 100644 index 0000000..700d896 --- /dev/null +++ b/tests/callback_cjs/mod.rs @@ -0,0 +1,24 @@ +use crate::common::*; +use orchestrion_js::*; + +#[test] +fn callback_cjs() { + transpile_and_test_with_imports( + file!(), + false, + Config::new_single(InstrumentationConfig::new( + "foo_ch", + ModuleMatcher::new("foo", ">=1.0.0", "index.js").unwrap(), + FunctionQuery::FunctionExpression { + expression_name: "doWork".to_string(), + kind: FunctionKind::Callback, + index: 0 + } + )), + &[PackageImport { + module_name: "foo".to_string(), + module_version: "1.1.1".to_string(), + file: "index.js".to_string() + }], + ) +} \ No newline at end of file diff --git a/tests/callback_cjs/test.js b/tests/callback_cjs/test.js new file mode 100644 index 0000000..90278d3 --- /dev/null +++ b/tests/callback_cjs/test.js @@ -0,0 +1,46 @@ +'use strict' + +const assert = require('node:assert') +const { getContext } = require('../common/preamble.js'); +const context = getContext('orchestrion:foo:foo_ch'); +const Foo = require('./instrumented.js') + +const start = process.hrtime.bigint() +const timer1 = setInterval(timeout, 1_000) +const timer2 = setInterval(timeout, 1_000) +const foo = new Foo() + +foo.doWork('success', (error, result) => { + assert.equal(error, undefined) + assert.equal(result, 'done') + + assert.deepStrictEqual(context, { + start: true + }) + delete context.start + + clearInterval(timer1) +}) + +foo.doWork('fail', (error, result) => { + assert.equal(error.message, 'boom') + assert.equal(undefined, result) + + assert.deepStrictEqual(context, { + end: true, + start: true + }) + delete context.end + delete context.start + + clearInterval(timer2) +}) + +function timeout() { + const current = process.hrtime.bigint() + if ((current - start) > 3e+10) { + // Exceeded 30 seconds, terminate it all. + clearInterval(timer1) + clearInterval(timer2) + } +} diff --git a/tests/common/mod.rs b/tests/common/mod.rs index f3dc816..1a013e6 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -12,14 +12,42 @@ use swc::config::IsModule; static TEST_MODULE_NAME: &str = "undici"; static TEST_MODULE_PATH: &str = "index.mjs"; +/// PackageImport represents metadata around a module that we are pretending +/// was imported from a npm module. So, if we are pretending to test +/// instrumentation of a module `@foo/bar`, the `module_name` would be +/// `@foo/bar`, the `module_version` would be a simple version string like +/// `0.1.0`, and the `file` would be some filename within that package like +/// `index.js` or `lib/utils.js`. +/// +/// This information will be used to match a unit test to the instrumentation. +pub struct PackageImport { + pub module_name: String, + pub module_version: String, + pub file: String +} + pub fn transpile_and_test(test_file: &str, mjs: bool, config: Config) { + transpile_and_test_with_imports(test_file, mjs, config, &[]); +} + +pub fn transpile_and_test_with_imports(test_file: &str, mjs: bool, config: Config, imports: &[PackageImport]) { let test_file = PathBuf::from(test_file); let test_dir = test_file.parent().expect("Couldn't find test directory"); let file_path = PathBuf::from("index.mjs"); let instrumentor = Instrumentor::new(config); - let mut instrumentations = + let mut instrumentations; + if imports.len() > 0 { + let import = &imports[0]; + instrumentations = instrumentor.get_matching_instrumentations( + import.module_name.as_str(), + import.module_version.as_str(), + &PathBuf::from(&import.file) + ); + } else { + instrumentations = instrumentor.get_matching_instrumentations(TEST_MODULE_NAME, "0.0.1", &file_path); + } let extension = if mjs { "mjs" } else { "js" }; let instrumentable = test_dir.join(format!("mod.{extension}")); diff --git a/tests/instrumentor_test.rs b/tests/instrumentor_test.rs index 6e2f7e9..340aed4 100644 --- a/tests/instrumentor_test.rs +++ b/tests/instrumentor_test.rs @@ -5,6 +5,7 @@ mod common; mod arguments_mutation; +mod callback_cjs; mod class_expression_cjs; mod class_method_cjs; mod constructor_cjs; From d68d2019fed860cc60c9791e0daab0b1cbcbd552 Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 20 Aug 2025 09:14:29 -0400 Subject: [PATCH 02/10] address nonsense lints I will never agree to implicit returns or difficult to read boolean checks. --- src/function_query.rs | 3 ++- src/instrumentation.rs | 11 +++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/function_query.rs b/src/function_query.rs index cef3599..1b75b0a 100644 --- a/src/function_query.rs +++ b/src/function_query.rs @@ -25,7 +25,8 @@ impl FunctionKind { pub fn is_async(&self) -> bool { matches!(self, FunctionKind::Async) } - + + #[must_use] pub fn is_callback(&self) -> bool { matches!(self, FunctionKind::Callback) } diff --git a/src/instrumentation.rs b/src/instrumentation.rs index 3218779..3d1fe41 100644 --- a/src/instrumentation.rs +++ b/src/instrumentation.rs @@ -131,7 +131,7 @@ impl Instrumentation { let trace_statement = construct_trace_statement( &self.config, &id_name, - (&self.module_version).clone().unwrap_or(String::new()) + self.module_version.clone().unwrap_or_default().as_str() ); body.stmts = vec![ @@ -395,13 +395,16 @@ pub fn get_script_start_index(script: &Script) -> usize { /// ```js /// return tr_ch_amp$myChannel.traceSync(__apm$traced, { arguments, self: this, moduleVersion: "1.0.0" }) /// ``` -fn construct_trace_statement(config: &InstrumentationConfig, channel_name: &String, mod_version: String) -> Stmt { +#[allow(clippy::bool_comparison)] +#[allow(clippy::needless_return)] +fn construct_trace_statement(config: &InstrumentationConfig, channel_name: &str, mod_version: &str) -> Stmt { let mut ctx = "{ arguments, self: this }".to_string(); - if mod_version.len() > 0 { - ctx = ["{ arguments, self: this, moduleVersion: \"", mod_version.as_str(), "\" }"].join(""); + if mod_version.is_ascii() == false { + ctx = ["{ arguments, self: this, moduleVersion: \"", mod_version, "\" }"].join(""); } let operator = ["tr_ch_apm$", channel_name, ".", config.function_query.kind().tracing_operator()].join(""); + #[allow(clippy::needless_late_init)] let stmt_str; if config.function_query.kind().is_callback() == true { // TODO: read callback position from configuration From 5870dbd7dae87e44e0234af1c8c2c37cd995a584 Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 20 Aug 2025 09:27:01 -0400 Subject: [PATCH 03/10] address formatting errors --- src/function_query.rs | 4 ++-- src/instrumentation.rs | 7 ++++--- tests/callback_cjs/mod.rs | 8 ++++---- tests/common/mod.rs | 3 ++- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/function_query.rs b/src/function_query.rs index 1b75b0a..9663cf2 100644 --- a/src/function_query.rs +++ b/src/function_query.rs @@ -17,7 +17,7 @@ pub(crate) enum FunctionType { pub enum FunctionKind { Sync, Async, - Callback + Callback, } impl FunctionKind { @@ -36,7 +36,7 @@ impl FunctionKind { match self { FunctionKind::Sync => "traceSync", FunctionKind::Async => "tracePromise", - FunctionKind::Callback => "traceCallback" + FunctionKind::Callback => "traceCallback", } } } diff --git a/src/instrumentation.rs b/src/instrumentation.rs index 3d1fe41..8ada1cc 100644 --- a/src/instrumentation.rs +++ b/src/instrumentation.rs @@ -131,7 +131,7 @@ impl Instrumentation { let trace_statement = construct_trace_statement( &self.config, &id_name, - self.module_version.clone().unwrap_or_default().as_str() + self.module_version.clone().unwrap_or_default().as_str(), ); body.stmts = vec![ @@ -141,7 +141,7 @@ impl Instrumentation { "if (!$ch.hasSubscribers) return __apm$traced();" as Stmt, ch = ch_ident ), - trace_statement + trace_statement, ]; self.has_injected = true; @@ -397,6 +397,7 @@ pub fn get_script_start_index(script: &Script) -> usize { /// ``` #[allow(clippy::bool_comparison)] #[allow(clippy::needless_return)] +#[rustfmt::skip] fn construct_trace_statement(config: &InstrumentationConfig, channel_name: &str, mod_version: &str) -> Stmt { let mut ctx = "{ arguments, self: this }".to_string(); if mod_version.is_ascii() == false { @@ -418,4 +419,4 @@ fn construct_trace_statement(config: &InstrumentationConfig, channel_name: &str, "$stmt" as Stmt, stmt = Ident::new(Atom::from(stmt_str), Span::default(), SyntaxContext::empty()) ) -} \ No newline at end of file +} diff --git a/tests/callback_cjs/mod.rs b/tests/callback_cjs/mod.rs index 700d896..ca81f77 100644 --- a/tests/callback_cjs/mod.rs +++ b/tests/callback_cjs/mod.rs @@ -12,13 +12,13 @@ fn callback_cjs() { FunctionQuery::FunctionExpression { expression_name: "doWork".to_string(), kind: FunctionKind::Callback, - index: 0 - } + index: 0, + }, )), &[PackageImport { module_name: "foo".to_string(), module_version: "1.1.1".to_string(), - file: "index.js".to_string() + file: "index.js".to_string(), }], ) -} \ No newline at end of file +} diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 1a013e6..8072b4d 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -23,13 +23,14 @@ static TEST_MODULE_PATH: &str = "index.mjs"; pub struct PackageImport { pub module_name: String, pub module_version: String, - pub file: String + pub file: String, } pub fn transpile_and_test(test_file: &str, mjs: bool, config: Config) { transpile_and_test_with_imports(test_file, mjs, config, &[]); } +#[rustfmt::skip] pub fn transpile_and_test_with_imports(test_file: &str, mjs: bool, config: Config, imports: &[PackageImport]) { let test_file = PathBuf::from(test_file); let test_dir = test_file.parent().expect("Couldn't find test directory"); From 19ffd6b2b95b45e54bb0045ef0e83eef5355dbe9 Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 20 Aug 2025 09:43:34 -0400 Subject: [PATCH 04/10] tweak snap --- src/instrumentation.rs | 2 +- tests/wasm/__snapshots__/tests.test.mjs.snap | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/instrumentation.rs b/src/instrumentation.rs index 8ada1cc..d41f483 100644 --- a/src/instrumentation.rs +++ b/src/instrumentation.rs @@ -400,7 +400,7 @@ pub fn get_script_start_index(script: &Script) -> usize { #[rustfmt::skip] fn construct_trace_statement(config: &InstrumentationConfig, channel_name: &str, mod_version: &str) -> Stmt { let mut ctx = "{ arguments, self: this }".to_string(); - if mod_version.is_ascii() == false { + if mod_version.is_empty() == false { ctx = ["{ arguments, self: this, moduleVersion: \"", mod_version, "\" }"].join(""); } diff --git a/tests/wasm/__snapshots__/tests.test.mjs.snap b/tests/wasm/__snapshots__/tests.test.mjs.snap index 92eec37..ba6536e 100644 --- a/tests/wasm/__snapshots__/tests.test.mjs.snap +++ b/tests/wasm/__snapshots__/tests.test.mjs.snap @@ -38,7 +38,7 @@ module.exports = class Up { const __apm$wrapped = ()=>{ console.log('fetch'); }; - return __apm$wrapped.apply(null, __apm$original_args); + return __apm$wrapped.apply(this, __apm$original_args); }; if (!tr_ch_apm$up_fetch.hasSubscribers) return __apm$traced(); return tr_ch_apm$up_fetch.traceSync(__apm$traced, { From 6f81ae8f14be21256d71fe7129d1828daf35f6e1 Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 20 Aug 2025 10:07:43 -0400 Subject: [PATCH 05/10] tweak snap --- tests/wasm/__snapshots__/tests.test.mjs.snap | 24 +++++--------------- 1 file changed, 6 insertions(+), 18 deletions(-) diff --git a/tests/wasm/__snapshots__/tests.test.mjs.snap b/tests/wasm/__snapshots__/tests.test.mjs.snap index ba6536e..11763e8 100644 --- a/tests/wasm/__snapshots__/tests.test.mjs.snap +++ b/tests/wasm/__snapshots__/tests.test.mjs.snap @@ -41,11 +41,7 @@ module.exports = class Up { return __apm$wrapped.apply(this, __apm$original_args); }; if (!tr_ch_apm$up_fetch.hasSubscribers) return __apm$traced(); - return tr_ch_apm$up_fetch.traceSync(__apm$traced, { - arguments, - self: this, - moduleVersion: "1.0.0" - }); + return tr_ch_apm$up_fetch.traceSync(__apm$traced, { arguments, self: this, moduleVersion: "1.0.0" }); } }; ", @@ -91,14 +87,10 @@ export class Up { const __apm$wrapped = ()=>{ console.log('fetch'); }; - return __apm$wrapped.apply(null, __apm$original_args); + return __apm$wrapped.apply(this, __apm$original_args); }; if (!tr_ch_apm$up_fetch.hasSubscribers) return __apm$traced(); - return tr_ch_apm$up_fetch.traceSync(__apm$traced, { - arguments, - self: this, - moduleVersion: "1.0.0" - }); + return tr_ch_apm$up_fetch.traceSync(__apm$traced, { arguments, self: this, moduleVersion: "1.0.0" }); } } ", @@ -144,17 +136,13 @@ export class Up { const __apm$wrapped = (url)=>{ console.log('fetch'); }; - return __apm$wrapped.apply(null, __apm$original_args); + return __apm$wrapped.apply(this, __apm$original_args); }; if (!tr_ch_apm$up_fetch.hasSubscribers) return __apm$traced(); - return tr_ch_apm$up_fetch.traceSync(__apm$traced, { - arguments, - self: this, - moduleVersion: "1.0.0" - }); + return tr_ch_apm$up_fetch.traceSync(__apm$traced, { arguments, self: this, moduleVersion: "1.0.0" }); } } ", - "map": "{"version":3,"file":"module.js","sources":["module.ts"],"sourceRoot":"","names":[],"mappings":";;;AAEA,MAAM,CAAA,MAAO,EAAE;IACX,aAAA;;;;;;;;;YACI,OAAO,CAAC,GAAG,CAAC,aAAa,CAAC,CAAC;;;;;;;;;;;;;;;;IAC/B,CAAC;IACD,KAAK,IAAS,EAAA;;;mCAAR;gBACF,OAAO,CAAC,GAAG,CAAC,OAAO,CAAC,CAAC;;;;;;;;;;IACzB,CAAC;CACJ"}", + "map": "{"version":3,"file":"module.js","sources":["module.ts"],"sourceRoot":"","names":[],"mappings":";;;AAEA,MAAM,CAAA,MAAO,EAAE;IACX,aAAA;;;;;;;;;YACI,OAAO,CAAC,GAAG,CAAC,aAAa,CAAC,CAAC;;;;;;;;;;;;;;;;IAC/B,CAAC;IACD,KAAK,IAAS,EAAA;;;mCAAR;gBACF,OAAO,CAAC,GAAG,CAAC,OAAO,CAAC,CAAC;;;;;;IACzB,CAAC;CACJ"}", } `; From 1c34a9a448181f5acb3459fdd5546ac07de8eafa Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 20 Aug 2025 13:29:35 -0400 Subject: [PATCH 06/10] add callback config --- src/config.rs | 39 +++++++++++++++++++++++++++++++++++++-- src/instrumentation.rs | 4 ++-- tests/callback_cjs/mod.rs | 3 ++- 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/config.rs b/src/config.rs index 3726ab5..14f9f41 100644 --- a/src/config.rs +++ b/src/config.rs @@ -47,6 +47,23 @@ impl ModuleMatcher { } } +#[cfg_attr(feature = "wasm", derive(tsify::Tsify))] +#[cfg_attr( + feature = "serde", + derive(serde::Serialize, serde::Deserialize), + serde(rename_all = "camelCase") +)] +#[derive(Debug, Clone)] +/// `CallbackConfig` represents details needed when wrapping a function that +/// accepts a callback. These details will be used to construct the +/// `tracingChannel.traceCallback` invocation. +pub struct CallbackConfig { + /// `position` is the ordinal of the callback function within the wrapped + /// function's parameter list. A value of `-1` indicates that the callback + /// is the last parameter in the list. The value is zero based. + pub position: i32, +} + #[cfg_attr( feature = "serde", derive(serde::Serialize, serde::Deserialize), @@ -62,16 +79,34 @@ pub struct InstrumentationConfig { pub channel_name: String, pub module: ModuleMatcher, pub function_query: FunctionQuery, + pub callback_config: CallbackConfig, } +#[allow(clippy::needless_return)] impl InstrumentationConfig { #[must_use] pub fn new(channel_name: &str, module: ModuleMatcher, function_query: FunctionQuery) -> Self { - Self { + return Self::new_with_callback_config( + channel_name, + module, + function_query, + CallbackConfig { position: -1 }, + ); + } + + #[must_use] + pub fn new_with_callback_config( + channel_name: &str, + module: ModuleMatcher, + function_query: FunctionQuery, + callback_config: CallbackConfig, + ) -> Self { + return Self { channel_name: channel_name.to_string(), module, function_query, - } + callback_config, + }; } #[must_use] diff --git a/src/instrumentation.rs b/src/instrumentation.rs index d41f483..9bc72c2 100644 --- a/src/instrumentation.rs +++ b/src/instrumentation.rs @@ -408,9 +408,9 @@ fn construct_trace_statement(config: &InstrumentationConfig, channel_name: &str, #[allow(clippy::needless_late_init)] let stmt_str; if config.function_query.kind().is_callback() == true { - // TODO: read callback position from configuration // TODO: figure out how we can pass a `this` argument - stmt_str = ["return ", &*operator, "(__apm$traced, -1, ", ctx.as_str(), ", this, ...arguments)"].join(""); + let pos = config.callback_config.position.to_string(); + stmt_str = ["return ", &*operator, "(__apm$traced, ", pos.as_str(), ", ", ctx.as_str(), ", this, ...arguments)"].join(""); } else { stmt_str = ["return ", &*operator, "(__apm$traced, ", ctx.as_str(), ")"].join(""); } diff --git a/tests/callback_cjs/mod.rs b/tests/callback_cjs/mod.rs index ca81f77..acc9513 100644 --- a/tests/callback_cjs/mod.rs +++ b/tests/callback_cjs/mod.rs @@ -6,7 +6,7 @@ fn callback_cjs() { transpile_and_test_with_imports( file!(), false, - Config::new_single(InstrumentationConfig::new( + Config::new_single(InstrumentationConfig::new_with_callback_config( "foo_ch", ModuleMatcher::new("foo", ">=1.0.0", "index.js").unwrap(), FunctionQuery::FunctionExpression { @@ -14,6 +14,7 @@ fn callback_cjs() { kind: FunctionKind::Callback, index: 0, }, + CallbackConfig { position: 1 }, )), &[PackageImport { module_name: "foo".to_string(), From ffbdfb984c52e574c5da448ed0491ec80affd37f Mon Sep 17 00:00:00 2001 From: James Sumners Date: Tue, 26 Aug 2025 13:23:02 -0400 Subject: [PATCH 07/10] fix wasm crash --- Cargo.lock | 1 + Cargo.toml | 1 + src/config.rs | 8 +++++++- src/instrumentation.rs | 2 -- src/lib.rs | 2 ++ 5 files changed, 11 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 08e522d..a95489e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1151,6 +1151,7 @@ version = "0.1.0" dependencies = [ "assert_cmd", "getrandom", + "log", "nodejs-semver", "serde", "swc", diff --git a/Cargo.toml b/Cargo.toml index 031f7cb..9cd9462 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,6 +27,7 @@ wasm-bindgen = { version = "0.2", optional = true } tsify = { version='0.5', features = ["js"], optional = true} # we need this to enable the js feature getrandom = { version = "*", features = ["wasm_js"], optional = true } +log = "0.4.27" [dev-dependencies] assert_cmd = "2" diff --git a/src/config.rs b/src/config.rs index 14f9f41..e858bf9 100644 --- a/src/config.rs +++ b/src/config.rs @@ -79,10 +79,11 @@ pub struct InstrumentationConfig { pub channel_name: String, pub module: ModuleMatcher, pub function_query: FunctionQuery, + #[tsify(optional)] + #[serde(default = "InstrumentationConfig::empty_callback_config")] pub callback_config: CallbackConfig, } -#[allow(clippy::needless_return)] impl InstrumentationConfig { #[must_use] pub fn new(channel_name: &str, module: ModuleMatcher, function_query: FunctionQuery) -> Self { @@ -149,4 +150,9 @@ impl InstrumentationConfig { pub fn matches(&self, module_name: &str, version: &str, file_path: &PathBuf) -> bool { self.module.matches(module_name, version, file_path) } + + #[must_use] + pub fn empty_callback_config() -> CallbackConfig { + return CallbackConfig{ position: -1 } + } } diff --git a/src/instrumentation.rs b/src/instrumentation.rs index 9bc72c2..6375c5b 100644 --- a/src/instrumentation.rs +++ b/src/instrumentation.rs @@ -395,8 +395,6 @@ pub fn get_script_start_index(script: &Script) -> usize { /// ```js /// return tr_ch_amp$myChannel.traceSync(__apm$traced, { arguments, self: this, moduleVersion: "1.0.0" }) /// ``` -#[allow(clippy::bool_comparison)] -#[allow(clippy::needless_return)] #[rustfmt::skip] fn construct_trace_statement(config: &InstrumentationConfig, channel_name: &str, mod_version: &str) -> Stmt { let mut ctx = "{ arguments, self: this }".to_string(); diff --git a/src/lib.rs b/src/lib.rs index 0618089..5d2d704 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,6 +13,8 @@ #![deny(clippy::complexity)] #![deny(clippy::correctness)] #![deny(clippy::unwrap_used)] +#![allow(clippy::bool_comparison)] +#![allow(clippy::needless_return)] /** * Unless explicitly stated otherwise all files in this repository are licensed under the Apache-2.0 License. From f05c0f31e7fed57d8c3d2fdf515f8b35720ae00a Mon Sep 17 00:00:00 2001 From: James Sumners Date: Tue, 26 Aug 2025 13:27:55 -0400 Subject: [PATCH 08/10] just effin format on save! --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index e858bf9..51f3750 100644 --- a/src/config.rs +++ b/src/config.rs @@ -153,6 +153,6 @@ impl InstrumentationConfig { #[must_use] pub fn empty_callback_config() -> CallbackConfig { - return CallbackConfig{ position: -1 } + return CallbackConfig { position: -1 } } } From d3fecdb72a6f482c798ddd02c4ed9300bab90892 Mon Sep 17 00:00:00 2001 From: James Sumners Date: Tue, 26 Aug 2025 13:33:03 -0400 Subject: [PATCH 09/10] =?UTF-8?q?=F0=9F=98=92?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.rs b/src/config.rs index 51f3750..f579741 100644 --- a/src/config.rs +++ b/src/config.rs @@ -153,6 +153,6 @@ impl InstrumentationConfig { #[must_use] pub fn empty_callback_config() -> CallbackConfig { - return CallbackConfig { position: -1 } + return CallbackConfig { position: -1 }; } } From 1520740df90d92ae1d31dc960d31d849ab3e9df5 Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 27 Aug 2025 07:32:32 -0400 Subject: [PATCH 10/10] revert cargo files --- Cargo.lock | 1 - Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a95489e..08e522d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1151,7 +1151,6 @@ version = "0.1.0" dependencies = [ "assert_cmd", "getrandom", - "log", "nodejs-semver", "serde", "swc", diff --git a/Cargo.toml b/Cargo.toml index 9cd9462..031f7cb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,6 @@ wasm-bindgen = { version = "0.2", optional = true } tsify = { version='0.5', features = ["js"], optional = true} # we need this to enable the js feature getrandom = { version = "*", features = ["wasm_js"], optional = true } -log = "0.4.27" [dev-dependencies] assert_cmd = "2"