Newrelic doesn't work with kraken

[djMax]

Not entirely sure why yet, but if I simply require('newrelic') at the top of the file, routes no longer work. It's meant to work with express 4, thus I implicate kraken (or maybe enrouten)

[tlivings]

Off the top of my head I am going to guess that something about the way newrelic is wrapping express apis is interfering with enrouten doing its job.

https://github.com/newrelic/node-newrelic/blob/master/lib/instrumentation/express.js

I'd argue that this isn't a kraken or enrouten issue, but rather an issue with what newrelic does to the express apis.

Edit: That being said it would be interesting to discover the exact "why".

[oblador]

FYI: I filed a bug report here on Github in June with a reproducible test case, which they confirmed but never managed to fix. Last month they simply closed all open issues and redirected everyone to their support forum, but I haven't yet filed another issue there.

So I guess you could change your router middleware or hope they eventually fix it after you file your issue. Maybe they don't consider kraken big enough to support.

[djMax]

Thanks. On relative sizes, I'd definitely suggest it's on Kraken's back to isolate and hopefully fix the incompatibility. Or at least tell them exactly what to change in their code.

[jasisk]

@oblador I'm thinking about digging into this (rainy day project, perhaps). Do you still have your test case? What about you, Max?

EDIT: worth noting I disagree that the onus is on us. I haven't even begun to look into this but I know that kraken is simply a middleware. We work within the confines of express without doing any sort of mutation. In contrast, the newrelic module—presumbly—does some instrumentation which, by definition, changes how things work. The express surface is our contract; if it changes out from underneath us, that falls outside our developer responsibilities.

That said, regardless of where the true engineering "responsibility" lies (which is effectively meaningless), it's impacting a shared portion of our users. At that point, it will boil down to priorities or—in the likely outcome of this case—who needs a rainy day project. 😀

[djMax]

Oh I agree from a software design perspective it's not on us. But from a reality perspective and relative size... On us. My appsforhere project has it commented out - commenting it back in will break it right away.

[jasisk]

Thanks. I'll take a look.

[matthewotto]

Also running into this issue. Not sure if it helps, but I've found that if there is more than one route defined - even if they are both located in controllers/index.js that enabling NewRelic will cause all routes to return a 404.

[daxsorbito]

Hi, has anyone found a workaround for this? We're pushing to prod soon.. :(

[tlivings]

I saw newrelic tweet about some new support added. Have you tried it?

https://docs.newrelic.com/docs/release-notes/agent-release-notes/nodejs-release-notes/node-agent-1120

[matthewotto]

Confirmed. Updating to the latest version (1.12.0) of the New Relic client addresses this issue.

[sixlettervariables]

Looks like this is the relevant fix:

newrelic/node-newrelic@be324bf#diff-44914992336a5a19961dc6c93a5b634fR303

[tlivings]

Great!

[jasisk]

So it looks like mount—which is an express internal event and which we rely on for our middleware-but-not-really-middleware pattern—was being eaten by their wrapped #use. That pattern, while not especially popular, has a good number of hits in other github repos. Glad it's been fixed! :)

[djMax]

I re-enabled newrelic in my app and it still fails (and when I remove it it works). Need to dig more but not sure this is fixed yet.

[maxinnos]

I'm unable to get this to work for me either. Anyone have success with it?

[tlivings]

Reopening pending additional verification.

[prnawa]

This line is causing the problem : https://github.com/newrelic/node-newrelic/blob/master/lib/instrumentation/express.js#L406

[djMax]

So have we given up on this?

[aredridel]

Didn't NewRelic come out with a new release that should fix this?

[djMax]

1.12.0 did not fix it for me, if that's the one you mean.

[aredridel]

Well drat. And they say they specifically support Kraken. Grr.

[djMax]

Interesting. Certainly possible I did something else wrong, but I can confirm that taking the require('newrelic') out makes my app work and putting it in makes all routes fail.

[aredridel]

Oh, I doubt it's you: new relic is super invasive.

[maxinnos]

I haven't been to get this to work either
On Nov 5, 2014 12:53 AM, "Aria Stewart" notifications@github.com wrote:

Oh, I doubt it's you: new relic is super invasive.

—
Reply to this email directly or view it on GitHub
#285 (comment).

[onurozkan]

any updates on this?

[lmarkus]

Went spelunking into the depths of Kraken / New Relic this afternoon.
https://github.com/lmarkus/newrelicbuster
⚠️ I'm patching a pretty big piece of code here. Everyone should still go bother New Relic to permanently fix it.

BTW... @aredridel is completely right. NewRelic gets everywhere in your code.

[totherik]

Thanks @lmarkus!

This kills some in-memory references that Kraken uses to maintain it's route stack.

Can you point me to the offending code? Maybe we can sort this out on our side, too.

[lmarkus]

Sure.
The "issue" is actually on express-enrouten.

This is the express function that NewRelic was wrapping:
node_modules/express/lib/router/index.js:471

proto.route = function(path){
  var route = new Route(path);

  var layer = new Layer(path, {
    sensitive: this.caseSensitive,
    strict: this.strict,
    end: true
  }, route.dispatch.bind(route));

  layer.route = route;

  this.stack.push(layer);
  return route;
};

A new route is pushed onto the router directly:
this.stack.push(layer);

From what I saw, Kraken keeps a reference to this object via this getter (but it was getting wiped out by NR):
node_modules/kraken-js/node_modules/express-enrouten/lib/registry.js

// Recursively search for the first non-registry (express) router.
    Object.defineProperty(registry, '_router', {
        get: function () {
            return router._router || router;
        }
    });

The problem manifested itself here:
node_modules/kraken-js/node_modules/express-enrouten/lib/directory.js:69

                debug('mounting', current, 'at', mountpath);
                subrouter = registry(mountpath, router);
                impl(subrouter);
                router.use(mountpath, subrouter._router);

New routes are appended by Express to subrouter.stack, but after the file has been walked through, express enrouten uses subrouter._router.stack. In a plain vanilla app, they both point to the same array.
NewRelic removed that reference when they inject the interceptor, by creating a new array object. The very first route added survived, all subsequent routes were lost.

[hayes]

the fix that @lmarkus created, has been released in version 1.14.0

I am hopeful that kraken will now work correctly with newrelic installed, but I have not done thorough testing of kraken itself. This only fixes the bug in our express instrumentation and it is possible there are other edge cases we have not covered. I am hoping to add proper integration tests for kraken soon, but we thought it was more important to first release fixes for bugs that had already identified.

A quick note on the original issue: The original issue brought to our attention was that kraken apps would crash almost instantly with newrelic enabled. As @jasisk pointed out, it was related to the mount event which kraken depends on. Kraken mounts a router and on mount pops off the top item on the middleware stack, and replaces it with some other middleware. Unfortunately our instrumentation adds an error handling middleware to the stack (usually as the last item) which caused kraken to pop off the wrong thing. Fixing this caused all of the repros of the issue we had received to work again, and we closed the issue. I want to apologize for not investigating further and checking for additional issues before closing that issue.

best,
Michael

[djMax]

Fantastic @hayes - really impressed by your attention to the issue!

[djMax]

May have spoken too soon... I see TypeError: Cannot read property 'partialName' of undefined. - still trying to isolate it.

[hayes]

As I said before, I did not have time to look into comparability with
kraken directly before thansgiving. But if you send me a repro, I'd be
happy to try to get it fixed as soon as possible. I am trying to get some
time on our roadmap to properly test and support kraken, but even without
that, we should not cause incompatibilities, the node agent is supposed to
be a transparent wrapper that does not have any side effects, so if there
are bugs in our express instrumentation I should be able to find time to
fix that quickly.

May have spoken too soon... I see TypeError: Cannot read property
'partialName' of undefined. - still trying to isolate it.

—
Reply to this email directly or view it on GitHub
#285 (comment).

[jasisk]

As far as I know, this has been resolved.