Skip to content
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

Problem using with express #100

Closed
ankitpatial opened this issue Dec 31, 2013 · 25 comments
Closed

Problem using with express #100

ankitpatial opened this issue Dec 31, 2013 · 25 comments
Labels

Comments

@ankitpatial
Copy link

I added new relic in one of my express app, but after adding package app started behaving abnormal.
Problem was with app.req object, instead of return a json object it started returning [Function: getQuery]

Any idea what can be wrong ?

I am now worried to use it in expressjs app, for now i dropped my idea to use it.

@othiym23
Copy link
Contributor

I'm sorry the agent is giving you trouble! New Relic for Node explicitly supports Express, and many people are using it successfully with Express, so let's see if we can figure out why it's not working for you:

  1. what version of Node are you running?
  2. what version of Express are you running?
  3. how are you loading New Relic? (It should be via require('newrelic') at the very top of your app's main module – New Relic needs to be the first thing loaded.)
  4. how are you configuring your Express middleware chain? (cutting and pasting all your app.use directives, in the order they appear in your app, would be useful here)
  5. what is the code snippet where you're fetching app.req? Pasting that here or putting it in a gist would also be helpful.

If you can get us that information, I'm pretty sure we can get New Relic working for you.

@ankitpatial
Copy link
Author

here is the asked detail.

  1. NodeJs v0.10.24
  2. Express v3.4.7
  3. I am using node cluster so it is as:
    var cluster = require( 'cluster' );
    require('new relic');

i tried moving require('new relic'); to each thread for cluster.
4.
var express = require('express'),
MongoSessionStore = require('express-session-mongo'),
http = require('http'),
passport = require('passport'),
winston = require('winston'),
config = require('./config'),
app = express();

    app.configure(function () {
        app.enable('trust proxy', true);

        app.use(express.favicon());
        app.use(express.logger('dev'));
        app.use(express.compress());
        app.use(express.methodOverride());

        app.use(express.json());
        app.use(express.urlencoded());


        app.use(express.cookieParser('secret1'));

        app.use(express.session({
            secret: 'secret',
            store: new MongoSessionStore({ server: db})
        }));

        //Passport configuration
        app.use(passport.initialize());
        app.use(passport.session());

        //winston logger setup
        require('./app/helpers/winston_mongodb').MongoDB;
        winston.add(winston.transports.MongoDB, require('./app/repositories/db'));
        winston.remove(winston.transports.Console);

        app.use(function (req, res, next) { // register request extensions.
            require('./app/helpers/req_helper')(req);
            next();
        });

        app.use(function (req, res, next) {
            winston.info('Req Start', req.getLogMeta({ ns: 'App', ip: req.ip, ips: req.ips, headers: req.headers }));
            next();
        });

        app.use(app.router);// don't register any middleware after it
    });

    app.configure('development', function() {
        app.use(express.errorHandler());
    });

    require('./app/index')(app);

    app.listen(3301);

5th. Sorry app.req is a typo i mean to say req.query

@othiym23
Copy link
Contributor

othiym23 commented Jan 3, 2014

Thanks for the details. Everything you've included looks fine to me.

Just to be clear, before you were accessing req.query and it was returning the deserialized query object, and with New Relic installed, it's returning a getQuery function instead? It's possible that New Relic is doing something that would cause this, but this isn't an issue I've encountered before, and I'm not sure what could cause it. Do you have a small standalone example that shows the behavior?

@ankitpatial
Copy link
Author

I will post code soon

@jeffwhelpley
Copy link

+1 on this. I am seeing the same issue. req.query returns the function getQuery whenever I have newrelic enabled.

@jeffwhelpley
Copy link

After many hours of debugging, I think I found out why I was encountering this issue. I used Express.js as my web server, but the web server code also makes some calls out to external APIs. For those external calls, I was using restify as a JSON client. I would imagine that you guys likely tested Restify as a server, but for whatever reason, if restify is referenced anywhere within my web server code, it oddly turns req.query into the function getQuery. As soon as I comment out //var restify = require('restify') then everything is fine. Once I knew that was definitely the issue, I just switched my client to use the request lib instead of restify for JSON client calls. So, there is still an issue here with the restify conflict, but the workaround for me is to use request lib.

@othiym23
Copy link
Contributor

That is great, useful detective work! Thanks so much, Jeff!

We actually don't have any tests combining Express and Restify, so the first step towards fixing this will be writing some. @ankitpatial, are you also using the Restify client in your application?

@ankitpatial
Copy link
Author

No, i am not using Restify. I will soon try to debug my app to know the cause

@ankitpatial
Copy link
Author

I pinpointed the problem, its some conflict with "manta" module that causing this problem.
You can reproduce it easily by including "manta" package and use it in app.

I have still not found the exact cause, but this what I can report for now.

@ankitpatial
Copy link
Author

"manta" package is using restify inside it. Restify is causing issue because it overriding Request object methods.
Strange thing is it is not causing any issue if newrelic module is not included.

Any insight on it, why combining new relic with resitify breaks express?

@othiym23
Copy link
Contributor

Great! It's good to know that it's the combination of Express and Restify that's causing the issue. Just so people watching this issue know, the Node team is currently heads down on feature development, and we'll get back to this issue after we've wrapped up our current chunk of feature work. Thanks for your patience!

@krishagel
Copy link

In case anyone else ever comes across this as I have, here is a potential work-around, you can use restify's getQuery function and then parse that with the querystring module.

var qsValues = querystring.parse(req.getQuery());

@juanpin
Copy link

juanpin commented Mar 24, 2014

I'm having the same problem. Any request that has query parameters is broken when I enable require newrelic. I use manta, express 3.

@othiym23
Copy link
Contributor

I'll be looking into this more deeply shortly -- we're fixing a bunch of bugs right now, and enough people have run into this one that it merits more in-depth investigation. A test case that exhibits this problem would be very helpful for me in fixing this issue, if anyone feels like passing one along.

@Harrison-M
Copy link

I've created a repository with a set of test cases. Note that the bug does not appear if restify is required before newrelic. In the interim, would it be an acceptable stopgap to include restify before newrelic? The documentation insists that newrelic should be placed first.

@othiym23
Copy link
Contributor

@Harrison-M if you require Restify before newrelic, much of the instrumentation won't be applied, and so at the very least you won't see any metrics for outbound HTTP requests. It could cause other problems with transaction tracing as well. I have some hope that Restify will get upgraded in such a way that it won't cause this problem anymore.

Also maybe it doesn't happen with Express 4? New Relic is about to ship support for Express 4, and the Express dev team has claimed that the new version does less magic with the prototype.

Thanks for the test cases! They're nice and tiny!

@Harrison-M
Copy link

@othiym23 You're quite welcome! Unfortunately, those test cases are using Express 4, and all of them succeed except index.js (the bug case).

@othiym23
Copy link
Contributor

Dang! Shoulda checked the package.json more closely (and not gotten my hopes up)! Thanks! @wraithan, you may want to take a look at these!

@Harrison-M
Copy link

Any luck with this? For reference, here's my output from the test cases I posted above:

♖  newrelic-restify master ♖  node index.js
[Function: getQuery]
/?test=success
{ name: 'AssertionError',
  actual: undefined,
  expected: 'success',
  operator: '==',
  message: '"undefined" == "success"' }

♖  newrelic-restify master ♖  node newreliconly.js
{ test: 'success' }
/?test=success
Newrelic okay

♖  newrelic-restify master ♖  node restifyonly.js
{ test: 'success' }
/?test=success
Restify okay

♖  newrelic-restify master ♖  node restifyfirst.js
{ test: 'success' }
/?test=success
Newrelic + Restify okay

@freshlogic
Copy link

FWIW, I just tried this using newrelic 1.6.0 on an express 3.4.6 app that uses the JSON client from restify 2.8.1 and things are still broken.

@txase
Copy link

txase commented May 26, 2014

I think I've tracked this down and am working on a fix.

When creating a Restify server, various obtrusive things happen like extending IncomingMessage with a query() method. Unfortunately, Express also extends IncomingMessage with a query property.

Some apps and modules use Restify only as a REST client. When an app also uses Express for serving and getting query params from IncomingMessages, our instrumentation causes the Restify query() method extension to override the Express query property.

@ankitpatial
Copy link
Author

Great! @txase, i am glad you found the cause. I hope it will be fixed and released very soon?

@txase
Copy link

txase commented May 29, 2014

@ankitpatial We are still reviewing the fix, but I'm hopeful it will be released this week.

@txase txase closed this as completed in 16cd762 May 29, 2014
@freshlogic
Copy link

This is great! Tried it on our apps and it's working well so far 👍

@wraithan
Copy link
Contributor

Fantastic news! Kudos to @txase for figuring out all out and fixing it.

cmcadams-newrelic pushed a commit to cmcadams-newrelic/node-newrelic that referenced this issue Jan 29, 2024
…/logs-in-context/app/semver-6.3.1

chore(deps): bump semver from 6.3.0 to 6.3.1 in /logs-in-context/app
jsumners-nr pushed a commit to jsumners-nr/node-newrelic that referenced this issue Apr 11, 2024
…/json5-2.2.3

Bump json5 from 2.2.1 to 2.2.3
jsumners-nr pushed a commit to jsumners-nr/node-newrelic that referenced this issue Apr 16, 2024
bizob2828 pushed a commit to bizob2828/node-newrelic that referenced this issue Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants