Fix for incomplete transaction names on new-relic dashboard

[sandeep89]

• Avoid polluting the partial name, which eventaully is the transaction name
• Hence do not worry it at cleanup

[martinkuba]

@sandeep89 can you please provide a test case?

[sandeep89]

Yes sure, will do.

[sandeep89]

Actually we do have test-case for this change, the problem was in the nested routes where the callback chains are nested (a specific express 4.0 version issue)
File:- test/unit/instrumentation/express.test.js

it("should set the transaction's scope after matchRequest is called", function () {
      helper.runInTransaction(agent, function () {
        var transaction = agent.getTransaction()
        transaction.verb = 'GET'

        var match = stub.Router.prototype.matchRequest
        expect(match()).eql({path : 'test/:id'})
        expect(transaction.partialName).equal('Expressjs/GET//test/:id')
      })
    })

[martinkuba]

@sandeep89 This does not show me the issue you are trying to solve. Can you please add a test to this file that shows the specific Express setup that causes incorrect transaction name?
https://github.com/newrelic/node-newrelic/blob/master/test/versioned/express4-ejs/express4-transaction-naming.tap.js

[sandeep89]

@martinkuba , added testcase which if you try to run with the original repo would fail and is a valid part of express ecosystem.

[martinkuba]

@sandeep89 We are naming based on the route that actually responded. In your case router2/path1 did not respond. Consider adding the following route to your test case

    app.get('/:var1/:var2/:var3', function(req, res, next) {
      next()
    })

What should the name be now - /:router1/:router2/path1 or /:var1/:var2/:var3?

[sandeep89]

@martinkuba , In my case the middleware did pass through the route, and eventually it passes through the success callback handler which is the last app.use

var router1 = express.Router()
var router2 = express.Router()

app.use('/:router1', router1)
router1.use('/:router2', router2)

router2.get('/:var1/:var2/:var3', function(req, res, next) {
  next()
})
app.use(function(req, res, next){
  res.send();
})
runTest(t, '/router1/router2/:var1/:var2/:var3', '/:router1/:router2/:var1/:var2/:var3')

so for a route '/:var1/:var2/:var3', in route2 we should expect transaction to be logged as /:router1/:router2/:var1/:var2/:var3, since the call back corresponding to the matching address was called and eventually the required data is passed to a success handler which generalizes the output and sends it.

[martinkuba]

@sandeep89 I meant adding this as a second route to the main app (not router2) - the point being that there could be multiple routes that could match the same URL.

app.use('/:router1', router1)
router1.use('/:router2', router2)

router2.get('/path1', function(req, res, next) {
  next();
})

app.get('/:var1/:var2/:var3', function(req, res, next) {
  next()
})

app.use(function(req, res, next){
  res.end();
})

Wouldn't you want the transaction to be named based on the route that generated the response, instead of the ones that executed but called next()? Calling next() in a route handler basically means "I am not responding, so passing control to other handlers that might".

[sandeep89]

@martinkuba, Agree about the returning concept, but we do follow a pattern when we have common success and error handlers, both in restify and express.
I think we should put a scope for that to, else we will loose all the routes and the transactions would be logged for '/' which is not useful actually.

And duplicating the method call for each route looks painful to me :(,

Also please not that the method which I am trying to use captures the exact route which matched, so even if there are conflicts in routes, it would not be our concern as the instrumentation would work on the request object and pull the required path and route.

[sandeep89]

@martinkuba , please suggest a work around for this.

Transation naming would be real pain for me I have to do it for all the routes which is around 200 in my service

[martinkuba]

@sandeep89 We cannot accept this PR as is because it will break other use cases (see my previous comment). You can also see the broken tests by running:
node test/versioned/express4-ejs/app-use.tap.js

I would also be hesitant to make this change because it would break the opposite assumption, i.e. if I don't reply from a route, then it should not be named as that route. We cannot do both.

Why not have a common function for sending the reply and calling it from each route instead of calling next()?

You could also use naming rules in your configuration file.
https://github.com/newrelic/node-newrelic/#rules-for-naming-and-ignoring-requests
If you have 200 unique routes, it means 200 rules, but at least you do not have to modify your code, and it's all in one place.

[sandeep89]

@martinkuba sure will try and create a workaround of my own over the existing stuff, thanks for the reply