Proposal for better transaction API

[mgartner]

This is a proposal for a change of the API to support a more Javascript-esque style. Motivation for this proposal comes from my own experience failing to get background transaction tracing working with Bluebird promises. The magical finding of the current transaction in newrelic.endTransaction() is unnecessary, confusing, and broken. This idea is also shared by others: https://discuss.newrelic.com/t/createbackgroundtransaction-strange-behavior/10416/2

Obviously this isn't functional, it's just a proposal of how the createBackgroundTransaction interface should look. Usage of this change would look something like this.

var newrelic = require('newrelic');

newrelic.createBackgroundTransaction('a:txn', function (tx) {
  doSomethingAsync(function () {
    tx.end();
  });
});

Modifying the interface to allow this should allow instrumenting transactions while using promises, even without adding first-class promise support:

newrelic.createBackgroundTransaction('a:txn', function (tx) {
  fs.readFileAsync('file.json')
  .then(JSON.parse)
  .then(function(val) {
    console.log(val.success);
  })
  .catch(SyntaxError, function(e) {
    console.error("invalid json in file");
  })
  .finally(function () {
    tx.end()
  });
});

[wraithan]

This would be a backwards incompatible change, and therefore wont make it into the agent until we are ready to bump the major version.

Your change requires the user to write more code, change function signatures, and means the user will have to write wrapper functions for any case where they can't change the function signature directly. The endTransaction() API is a sharp point in the API. But the Transaction object's API is not a public one and we'd need to shim in an object between the user and the object.

We purposely used a higher order function (also known as a decorator) API instead of one that calls the handler directly so minimal changes to the user's code would be needed. This allows the user to call their function directly if they have cases where it shouldn't be instrumented.

I'm going to close this PR as the changes can't be merged and it is unlikely that this API will support such a change without a major version bump.

[mgartner]

Your change requires the user to write more code

My suggestion requires the same amount or less code to integrate with the agent. See my first provided example of usage and compare that with the example from your documentation (modified to match my method names):

var newrelic = require('newrelic');

newrelic.createBackgroundTransaction('a:txn', function () {
  doSomethingAsync(function () {
    newrelic.endTransaction();
  });
})();

the user will have to write wrapper functions for any case where they can't change the function signature directly

My suggestion doesn't require any change to function signatures.

This really doesn't seem like something that should be overlooked at this point. Calling a magical newrelic.endTransaction() with no reference to the transaction it is referencing is just plain wrong.

[wraithan]

It does change the signature of the function that is handed to the agent. It has to take a tx object, Currently that isn't the case and that is intentional. Also it means users don't have to pass the tx object through their system, changing signatures along the way. You are assuming the end of an async chain is always within closure scope, which isn't always the case.

The newrelic.endTransaction() allows the user to end their transaction where they expect it to end, but will throw debug messages if that doesn't happen, allowing the user to know that there were uninstrumented calls along the way, which is also important.

Also, to the primary reason why I closed this. It is a backwards incompatible change. If you'd like a feature request for a new API endpoint that calls your function immediately and passes a transaction in, please speak to support about that and we can get it on the roadmap where it is appropriate.

[wraithan]

I was thinking about it more. You are right that providing an API that works this way would be good. That said we can't modify the existing API call to do it due to backwards incompatibility problems, it would need to be a separate and new API call.

Adding new API surface is something we have to discuss as a team as it expands our contract with end users. We'd need to proxy the transaction object to not expose the API that it has (as we reserve the right to change that) but aside from that, it seems pretty sane. Another solution instead of proxying the transaction object would be to provide a done callback that the user could call when their transaction should be ended. That feels even more JS-y.

[mgartner]

I didn't think about the backwards incompatibility, but you're right on that point. No need to drop the current functionality.

I also agree that the done callback would work well too. 👍