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

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

Open
askhogan opened this Issue Feb 5, 2015 · 17 comments

Comments

Projects
None yet
10 participants
@askhogan
Copy link

askhogan commented Feb 5, 2015

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 askhogan changed the title Including newrelic and CLS together overwrites CLS Including newrelic and CLS together overwrites your custom CLS data Feb 5, 2015

@othiym23

This comment has been minimized.

Copy link
Owner

othiym23 commented Mar 7, 2015

@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

This comment has been minimized.

Copy link

hayes commented Mar 7, 2015

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

This comment has been minimized.

Copy link
Author

askhogan commented Mar 7, 2015

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

@askhogan

This comment has been minimized.

Copy link
Author

askhogan commented Mar 7, 2015

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

This comment has been minimized.

Copy link

hayes commented Mar 9, 2015

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

@alexmic

This comment has been minimized.

Copy link

alexmic commented Dec 17, 2015

+1, using newrelic==1.22.0.

@othiym23 othiym23 added the bug label Jan 29, 2016

@holm

This comment has been minimized.

Copy link

holm commented Jan 30, 2016

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

This comment has been minimized.

Copy link

andressrg commented Feb 22, 2016

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

Related to strongloop/loopback#1984

@ianwremmel

This comment has been minimized.

Copy link

ianwremmel commented Feb 27, 2016

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

This comment has been minimized.

Copy link

andressrg commented Feb 27, 2016

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

This comment has been minimized.

Copy link

ianwremmel commented Feb 27, 2016

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

@vdeturckheim

This comment has been minimized.

Copy link

vdeturckheim commented Mar 16, 2017

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

This comment has been minimized.

Copy link
Collaborator

Qard commented Mar 17, 2017

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

This comment has been minimized.

Copy link

jamischarles commented Mar 20, 2017

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

@vdeturckheim

This comment has been minimized.

Copy link

vdeturckheim commented Mar 21, 2017

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

@othiym23

This comment has been minimized.

Copy link
Owner

othiym23 commented Mar 22, 2017

@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

This comment has been minimized.

Copy link

vdeturckheim commented Mar 22, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.