feat: allow message.result to be changed

[isaacs]

Allow an end handler to modify the return value of a synchronous function.

This is needed for the NestJS instrumentation, because its request handler mechanism is a factory that returns an inner arrow function, capturing values that parity with the OTel-based instrumentation requires us to report on. (These values are not present in the inner function's arguments; they are captured by a closure only.)

This could be done by wrapping both the inner and outer methods, and correlating them with a WeakMap, but this would be more brittle and complicated, for no benefit.

[isaacs]

Ah, turns out this is also important for a bunch of web frameworks we wanna instrument. NestJS was just the first one encountered. But Express, fastify, etc, all going to need this. (I guess we could do most of Express by just instrumenting the Router prototype and getting access to it the first time it's called, but returned-function-wrapping makes it a lot easier.)

[jsumners-nr]

If I'm not mistaken, we solved this by hooking the traceCallback event -- https://github.com/newrelic/node-newrelic/blob/0520c354df0ffcefc31e4581641741f03d7e525b/lib/subscribers/base.js#L102-L156

I think @bizob2828 will be able to provide a better review of this than me.

[isaacs]

@jsumners-nr It's my first time seeing this, so I could be misreading it, but it looks like that's a facility to wrap an incoming callback argument, not wrapping a returned function handler.

Eg, it looks like it's for this:

export function takesCallBack (a: number, b: string, cb: Function) {
  // wrap cb function and replace with the wrapped value
}

And not for this:

export function returnsHandler (a: number, b: string) {
  // wrap this return value
  return wrapThisFunction() {
    doSomething(a, b)
  }
}

The only way around it that I could see, would involve wrapping both functions (the exported and returned) and a complicated WeakMap communication mechanism between them.

[bizob2828]

Since it's opt-in and not default, seems ok to me if it solves a problem y'all are experiencing.

[isaacs]

@bengl @jsumners-nr Updated as we discussed this morning.

• mutableResult option is gone.
• Sync and async functions can have their results mutated.
• Result mutation is just silently ineffective for non-Promise then-ables, with appropriate caveats in the docs and comment in the (non-generated) code.