Added support for traceCallback #28

jsumners-nr · 2025-08-20T13:01:58Z

This PR adds support for tracingChannel.traceCallback.

jsumners-nr · 2025-08-20T14:26:01Z

tests/callback_cjs/test.js

+  assert.deepStrictEqual(context, {
+    start: true
+  })
+  delete context.start


I'm not sure about the result here. I guess this is verifying that the context is updated, but I don't know what the actual expectation for the shape of the modified context should be.

I will never agree to implicit returns or difficult to read boolean checks.

jsumners-nr · 2025-08-27T11:33:53Z

src/instrumentation.rs

+                // 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),


Maybe should've been a separate PR, but binding to null could prove problematic. So I tweaked it to reference the enclosing this context.

jsumners-nr · 2025-08-27T11:35:16Z

src/lib.rs

+#![allow(clippy::bool_comparison)]
+#![allow(clippy::needless_return)]


I think !something is way more difficult to read than something == false, and I'm completely against implicit returns.

Agree about clippy::bool_comparison.

However, it's hard to be "completely against implicit returns" in Rust because every statement in Rust implicitly returns. Even if you don't use the return keyword or return anything, the statement will implicitly return the unit type ().

For example adding return to these three functions adds nothing but noise:

impl FunctionKind { #[must_use] pub fn is_async(&self) -> bool { matches!(self, FunctionKind::Async) } #[must_use] 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", } } }

You could write tracing_operator with all these returns but why would you?

pub fn tracing_operator(&self) -> &'static str { return match self { FunctionKind::Sync => return "traceSync", FunctionKind::Async => return "tracePromise", FunctionKind::Callback { .. } => return "traceCallback", }; }

Because an implicit return is mystical. I do not agree with your assessment.

The code I would write is:

impl FunctionKind { #[must_use] pub fn is_async(&self) -> bool { return matches!(self, FunctionKind::Async) } #[must_use] pub fn is_callback(&self) -> bool { return matches!(self, FunctionKind::Callback { .. }) } #[must_use] pub fn tracing_operator(&self) -> &'static str { return match self { FunctionKind::Sync => "traceSync", FunctionKind::Async => "tracePromise", FunctionKind::Callback { .. } => "traceCallback", } } }

A lambda function returning it's single statement is okay. I don't know how to put it except that for the others, it's akin to leaving off the braces on conditional statements. That is, people would rightly balk at:

if (something == true) foo

We can illustrate further:

function isSomething(input) { if (input == 'some test') true false }

In my opinion, it's much clearer and less prone to error in the future to write:

function isSomething(input) { if (input == 'some test') { return true } return false }

Either way, this PR is not the right place to propose changing major lints and coding idioms of this repository. That should be proposed in another PR.

This PR doesn't propose anything that requires modifications to existing source code.

jsumners-nr · 2025-08-27T11:37:39Z

tests/common/mod.rs

+        let import = &imports[0];
+         instrumentations = instrumentor.get_matching_instrumentations(
+            import.module_name.as_str(),
+            import.module_version.as_str(),
+            &PathBuf::from(&import.file)
+        );


Should really have a loop here to support more than one instrumentation at a time. But all of the existing tests utilize a single instrumentation. So I have left this as an exercise for later.

timfish

Nice addition and moving code to construct_trace_statement cleans it up.

I do think we should only be able to supply the callback position for a callback config but it's late so I might test a few options on my flight tomorrow!

timfish · 2025-08-27T21:51:48Z

src/config.rs

@@ -62,16 +79,35 @@ 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,


This should probably be an Option<CallbackConfig>. Option can be None or Some<T>. That way if it's not supplied, deserialisation will automatically make it None and you don't need the serde default.

Having said that, see my other comment below because I'm not convinced this config should be here.

I expect that would make dealing with the value more difficult in the code where it is actually used, e.g. when building the JavaScript string to inject into the script being instrumented. In fact, I am confident of it because I encountered the difficulty when dealing with module_version:

let trace_statement = construct_trace_statement( &self.config, &id_name, self.module_version.clone().unwrap_or_default().as_str(), );

You should not be taking the default of self.module_version anyway as this will be an empty string.

You should change construct_trace_statement to take mod_version: Option<&str> and then construct the JavaScript code differently depending on whether the module_version actually exists:

fn construct_trace_statement(config: &InstrumentationConfig, channel_name: &str, mod_version: Option<&str>) -> Stmt { let ctx = if let Some(mod_version) = mod_version { format!("{{ arguments, self: this, moduleVersion: \"{}\" }}", mod_version) } else { "{ arguments, self: this }".to_string() };

Then you can call construct_trace_statement like this:

construct_trace_statement(&self.config, &id_name, self.module_version.as_deref());

Checking for the empty string is way easier all around.

timfish · 2025-08-27T22:00:21Z

src/function_query.rs

@@ -17,6 +17,7 @@ pub(crate) enum FunctionType {
 pub enum FunctionKind {
    Sync,
    Async,
+    Callback,


I think that the CallbackConfig should go here. So this line should either be Callback(CallbackConfig) or Callback { position: i32 }. Which one to choose is open for debate. I prefer Callback { position: u32 } because then we don't need the CallbackConfig struct at all. Probably whichever one deserialises from JavaScript in the cleanest way!

After this change, the callback position config can only be passed when you're actually setting up a callback hook.

How it currently stands you can pass a callback_config with position for both sync and async and what would happen with it?

How it currently stands you can pass a callback_config with position for both sync and async and what would happen with it?

It'd be ignored.

I think that the CallbackConfig should go here. So this line should either be Callback(CallbackConfig) or Callback { position: u32 }. Which one to choose is open for debate. I prefer Callback { position: u32 } because then we don't need the CallbackConfig struct at all. Probably whichever one deserialises from JavaScript in the cleanest way!

After this change, the callback position config can only be passed when you're actually setting up a callback hook.

Sorry, I don't understand any of that. As far as I can tell, the pub enum FunctionKind merely provides a thing to check against for the kind: <Sync|Async|Callback> YAML line.

timfish · 2025-08-27T22:22:06Z

src/instrumentation.rs

+
+    let operator = ["tr_ch_apm$", channel_name, ".", config.function_query.kind().tracing_operator()].join("");
+    #[allow(clippy::needless_late_init)]
+    let stmt_str;


In Rust all statements return a value so it's much cleaner to just:

let stmt_str = if config.function_query.kind().is_callback() { // TODO: figure out how we can pass a `this` argument let pos = config.callback_config.position.to_string(); ["return ", &*operator, "(__apm$traced, ", pos.as_str(), ", ", ctx.as_str(), ", this, ...arguments)"].join("") } else { ["return ", &*operator, "(__apm$traced, ", ctx.as_str(), ")"].join("") }

If the callback config gets moved to FunctionKind, creating stmt_str could become this:

let stmt_str = match config.function_query.kind() { FunctionKind::Callback { position } => { format!("return {operator}(__apm$traced, {position}, {ctx}, this, ...arguments)") }, _ => format!("return {operator}(__apm$traced, {ctx})") };

Prefer format!() over concatenation as converts to string for you and is easier to read what the output will look like.

I think all of that is way more difficult to read. The code in the PR is as legible as I can make it in this language.

timfish · 2025-08-27T23:20:24Z

Here are the changes I would make:
pr-review.patch

We also need to add a test the JavaScript tests to ensure this new config can be deserialised from there.

jsumners-nr · 2025-08-28T12:00:28Z

We also need to add a test the JavaScript tests to ensure this new config can be deserialised from there.

I don't understand this one. I added a test?

bizob2828 · 2025-08-28T21:16:56Z

src/instrumentation.rs

@@ -119,19 +119,20 @@ 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),
            ],
        };

        let traced_fn = self.new_fn(traced_body, vec![]);


so i was testing all of this out and I found that traceCallback doesn't work if we wrap the functions as arrow functions, but will if they are defined as FnExpr. There are two places in this file that call new_fn and I think if we want to wrap the functions in a semantically correct way, the new_fn will have to be updated to no longer use ArrowExpr but FnExpr. Looking at the docs the struct isn't as verbose as ArrowExpr though: https://rustdoc.swc.rs/swc_core/ecma/ast/struct.FnExpr.html. I changed the code to output function expressions but now classes that call super. are failing so there needs to be some binding that occurs to all these functions I think

This is a test that asserts it's doing the right thing:

const diagnostics_channel = require('node:diagnostics_channel'); const channels = diagnostics_channel.tracingChannel('my-channel'); const { AsyncLocalStorage } = require('node:async_hooks'); const assert = require('node:assert') const myStore = new AsyncLocalStorage(); let expectedCalls = 0 // The start channel sets the initial store data to something // and stores that store data value on the trace context object channels.start.bindStore(myStore, (data) => { console.log('calling start store') expectedCalls++ return data }); channels.asyncStart.bindStore(myStore, (data) => { console.log('calling async start store') expectedCalls++ return data }); channels.subscribe({ start() { console.log('calling start') expectedCalls++ }, end() { console.log('calling end') expectedCalls++ }, error() { console.log('calling error') expectedCalls++ }, asyncStart() { console.log('calling asyncStart') expectedCalls++ }, asyncEnd() { console.log('calling asyncEnd') expectedCalls++ } }) function test() { const origArgs = arguments function traced() { function wrapped(name, obj, cb) { console.log('cb name should be wrapped', cb.name) assert.equal(cb.name, 'wrappedCallback') cb(null, 'test') } return wrapped.apply(this, arguments) } return channels.traceCallback(traced, -1, { some: 'thing'}, this, ...origArgs) } test('hi', { key: 'value'}, (err, data) => { assert.deepEqual(err, null) assert.equal(data, 'test') setImmediate(() => { assert.equal(expectedCalls, 6) }) })

but if you change the wraps to arrow functions, like this repo does, they fail.

timfish · 2025-08-29T15:19:58Z

We also need to add a test the JavaScript tests to ensure this new config can be deserialised from there.

I don't understand this one. I added a test?

You added a Rust test but all the JavaScript tests are in ./tests/wasm/tests.test.mjs and that file has not been modified.

jsumners-nr commented Aug 20, 2025

View reviewed changes

jsumners-nr force-pushed the issue-26 branch from 18f01af to d982393 Compare August 20, 2025 17:35

jsumners-nr marked this pull request as ready for review August 26, 2025 17:40

jsumners-nr requested review from timfish and bizob2828 August 26, 2025 17:41

jsumners-nr added 9 commits August 26, 2025 15:09

Added support for traceCallback

e99954c

address nonsense lints

d68d201

I will never agree to implicit returns or difficult to read boolean checks.

address formatting errors

5870dbd

tweak snap

19ffd6b

tweak snap

6f81ae8

add callback config

1c34a9a

fix wasm crash

ffbdfb9

just effin format on save!

f05c0f3

😒

d3fecdb

jsumners-nr force-pushed the issue-26 branch from 90ec3b3 to d3fecdb Compare August 26, 2025 19:09

revert cargo files

1520740

jsumners-nr commented Aug 27, 2025

View reviewed changes

timfish reviewed Aug 27, 2025

View reviewed changes

bizob2828 reviewed Aug 28, 2025

View reviewed changes

jsumners-nr marked this pull request as draft August 29, 2025 11:43

This was referenced Aug 29, 2025

Clear code DataDog/orchestrion-js#25

Closed

Allow clearer code #30

Open

		// 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),

		#![allow(clippy::bool_comparison)]
		#![allow(clippy::needless_return)]

Added support for traceCallback #28

Are you sure you want to change the base?

Added support for traceCallback #28

Uh oh!

Conversation

jsumners-nr commented Aug 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timfish Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timfish left a comment

Choose a reason for hiding this comment

Uh oh!

timfish Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timfish Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timfish Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsumners-nr Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timfish commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsumners-nr commented Aug 28, 2025

Uh oh!

bizob2828 Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timfish commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

timfish Aug 27, 2025 •

edited

Loading

timfish Aug 27, 2025 •

edited

Loading

timfish Aug 27, 2025 •

edited

Loading

timfish Aug 27, 2025 •

edited

Loading

jsumners-nr Aug 28, 2025 •

edited

Loading

timfish commented Aug 27, 2025 •

edited

Loading

bizob2828 Aug 28, 2025 •

edited

Loading

timfish commented Aug 29, 2025 •

edited

Loading