node-continuation-local-storage icon indicating copy to clipboard operation
node-continuation-local-storage copied to clipboard

Including newrelic and CLS together overwrites your custom CLS data

Open askhogan opened this issue 10 years ago • 17 comments
trafficstars

I am having an interesting issue. CLS was created for commerical pursuits for newrelic. I have newrelic and like it. You gracefully open sourced the app. I want to use CLS and newrelic. Each time I include both newrelic and CLS the newrelic library wipes my CLS stores out. There are no naming conflicts.

askhogan avatar Feb 05 '15 05:02 askhogan

@askhogan This sounds like a question for @hayes -- It could be as simple as making sure that newrelic is required before continuation-local-storage in your application, or there could be some deeper conflict between the two. I'm no longer up to speed with how New Relic uses (or doesn't use) CLS, and so the only bits I can help you with are the pieces that are CLS-specific.

Michael, what's the story here? Do I need to do something to shim CLS to avoid getting stepped on by newrelic? Or is my advice above enough?

othiym23 avatar Mar 07 '15 16:03 othiym23

I can't think of any reason that would be happening. I am curious what version of newrelic you were using. If there is some kind of conflict I would like to get that fixed right away. There was an issue we had with modules that used an older version of async-listener, but I don't think CLS has used that in a long time.

hayes avatar Mar 07 '15 16:03 hayes

newrelic "version": "1.16.1" continuation-local-storage "version": "3.1.2",

askhogan avatar Mar 07 '15 21:03 askhogan

I use CLS in my log4js logger wrapper to add context like username and organization so I can pinpoint what errors are affecting which users. I use newrelic for the transaction traces.

askhogan avatar Mar 07 '15 21:03 askhogan

I am not sure what is causing this, but I would recommend trying it with newrelic@>=1.17.0

hayes avatar Mar 09 '15 20:03 hayes

+1, using newrelic==1.22.0.

alexmic avatar Dec 17 '15 16:12 alexmic

Although I cannot add much detail, I can say I had the exact same problem. Eventually I had to give up on New Relic.

holm avatar Jan 30 '16 23:01 holm

I'm having the same problem using loopback. Can't use new relic on my loopback application because of cls

Related to https://github.com/strongloop/loopback/issues/1984

andressrg avatar Feb 22 '16 17:02 andressrg

Seeing the same issue

continuation-local-storage: 3.1.6 newrelic: 1.25.4

Any chance the problem is because newrelic treats cls as a bundled dependency? Could that be causing node to load two different versions of cls?

ianwremmel avatar Feb 27 '16 01:02 ianwremmel

This issue happens on Node 4.3.1 for me, but downgrading to Node 0.12.10 fixed it.

@ianwremmel which Node version are you using?

andressrg avatar Feb 27 '16 01:02 andressrg

I've used 4.3.0 and 4.3.1. I think I fixed it by requiring cls right before newrelic.

ianwremmel avatar Feb 27 '16 01:02 ianwremmel

Hey, I am having a newrelic-cls issue too: the following code:

// require('newrelic');
const CLS = require('continuation-local-storage');
const Express = require('express');
const app = Express();
const Ns = CLS.createNamespace('ns');
app.get('/', (req, res) => {
    Ns.run(() => {
        Ns.set('a', 1);
        Promise.resolve()
            .then(() => {
                console.log(Ns.get('a'));
                return '100'
            })
            .then((response) => {
                console.log(Ns.get('a'));
                return res.json(response);
            });
    });
});

app.listen(8080, () => {
    console.log('ok');
});

outputs on request:

1
1

But if I uncomment the import of newrelic, I get:

1
undefined

I'm using the latest versions of the CLS and of newrelic. I'll try to dig that tomorrow, but if anybody has a clue of where it comes from.. I assume it is the wrapping of Promises by newrelic that causes the issue.

vdeturckheim avatar Mar 16 '17 18:03 vdeturckheim

Probably the double-wrapping hides the __asl_wrapper property attached here: https://github.com/othiym23/async-listener/blob/master/index.js#L450

@hayes Might know better.

Qard avatar Mar 17 '17 19:03 Qard

I saw the same with appdynamics code. Requiring CLS first fixed the issue for me.

jamischarles avatar Mar 20 '17 22:03 jamischarles

As I am bundling the CLS into a RASP solution, it is hard to ensure the clients declare newrelic after sqreen.

vdeturckheim avatar Mar 21 '17 08:03 vdeturckheim

@vdeturckheim I'd have to investigate the newrelic and AppDynamics agents to be sure, but I believe that the fact that this works with one order and not the other means that the APM agents aren't as careful about leaving the wrapped functions in a state where they can be wrapped again later. If this is the case, then the issue is with them, rather than CLS, and there's little that CLS can do about it. This is why the documentation for CLS (and, when it used CLS as an enabling module, the newrelic agent) was so insistent about it being loaded first. If you can't get newrelic and other agents to change, perhaps you can partially remedy the problem by making clear what needs to happen in your own docs?

I wish I had a better solution for you here, but these are the wrinkles that come from having a technique that relies upon extensively monkeypatching the core platform. (And the reason why newrelic probably doesn't follow the rules is the same reason it abandoned using CLS – reduced overhead and increased performance, which are important requirements for a monitoring solution intended to be run in production.)

othiym23 avatar Mar 22 '17 00:03 othiym23

@othiym23 thanks a lot for this clear answer. This outcome is the one I have been suspecting for a while!

Ageed regarding the fact that the issue is on NewRelic side. In the past, requiring sqreen before newrelic caused a few issues in NewRelic but they are fixed now. So we'll go this way.

Thanks again for this great module and your answer!

vdeturckheim avatar Mar 22 '17 08:03 vdeturckheim