othiym23 / node-continuation-local-storage

implementation of https://github.com/joyent/node/issues/5243
BSD 2-Clause "Simplified" License
1.13k stars 207 forks source link

Including newrelic and CLS together overwrites your custom CLS data #33

Open askhogan opened 9 years ago

askhogan commented 9 years ago

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.

othiym23 commented 9 years ago

@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?

hayes commented 9 years ago

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.

askhogan commented 9 years ago

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

askhogan commented 9 years ago

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.

hayes commented 9 years ago

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

alexmic commented 8 years ago

+1, using newrelic==1.22.0.

holm commented 8 years ago

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

andressrg commented 8 years ago

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

ianwremmel commented 8 years ago

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?

andressrg commented 8 years ago

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?

ianwremmel commented 8 years ago

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

vdeturckheim commented 7 years ago

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.

Qard commented 7 years ago

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.

jamischarles commented 7 years ago

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

vdeturckheim commented 7 years ago

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

othiym23 commented 7 years ago

@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.)

vdeturckheim commented 7 years ago

@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!