-
Notifications
You must be signed in to change notification settings - Fork 399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add base URL in Express 4 routers #173
Conversation
Would you be able to add a test for this? Somewhere in here: https://github.com/newrelic/node-newrelic/tree/master/test/versioned/express4-ejs We use tests to make sure features continue working going forward. If you don't have time to, it may take us a bit longer to be able to write the tests and make sure this works. |
@wraithan Thanks for taking a look. I added a test, but I'm not 100% sure if it passes or not because I can't get all the way through the integration tests on my machine without it breaking. I setup all the deps using Docker, but maybe you have a machine that's all ready to go that you could run a quick test on? Thanks! |
We will be looking at this for merge in the near future. We may need to rewrite that test since it doesn't appear to actually be mounting the sub router (the IIFE doesn't return anything). We may need to be more defensive for grabbing the Agent code is about being defensive as possible while still grabbing data. Thanks for this and we'll base continued work on your patch. |
@wraithan Thanks! I'm happy to make any changes you think are needed. |
After looking at this a little more closely, the issue is slightly more complicated than it initially appears. With this fix using req.baseUrl is not a sufficient solution. the agent aggregates transactions using the route rather than the url eg This fix will take some additional work to get into a releasable state. |
@hayes Can you provide a quick code example of a routes configuration that is not handled by this patch? |
var nr = require('newrelic');
var express = require('express');
var app = express();
var users = express.Router();
app.use('/users/:id', users);
users.get('/profile', function(req, res) {
res.end('user profile');
});
var server = app.listen(3000, function() {
console.log('Listening on port %d', server.address().port);
}); In this example for a requests to |
@mjackson just wanted to give you an update, we have a fix we that should go out in the next release. |
@hayes Thank you for keeping on top of this! I'm looking forward to seeing your patch land. Please feel free to close this PR when it does :) |
@mjackson our patch for this is in the latest release https://docs.newrelic.com/docs/release-notes/agent-release-notes/nodejs-release-notes/node-agent-1121 |
…/follow-redirects-1.15.6 build(deps-dev): bump follow-redirects from 1.15.5 to 1.15.6
This patch adds the base URL to paths by default in Express 4.
See https://discuss.newrelic.com/t/node-js-agent-doesnt-put-full-express-4-router-paths-in-by-default/1776