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

CorrelationContext.getCurrentContext() returning undefined when deeply nested #296

Closed
tirish opened this issue Aug 15, 2017 · 6 comments
Closed
Milestone

Comments

@tirish
Copy link

tirish commented Aug 15, 2017

Our auto-collected dependencies are inconsistently being correlated to the request (operation).

It appears that CorrelationContext.getCurrentContext() is working properly if called directly within the middleware (express) but if called further down (within callbacks and/or promises) it is returning undefined.

I have hacked in a workaround using continuation-local-storage:

var cls = require('continuation-local-storage');
var ns = cls.createNamespace('my.request');


//hack hack hack
var correlationContextManager = require('applicationinsights/out/AutoCollection/CorrelationContextManager').CorrelationContextManager;
var origGetCurrentContext = correlationContextManager.getCurrentContext;
function getCurrentContext(){
  return ns.get('ctx') || origGetCurrentContext();
}
correlationContextManager.getCurrentContext = getCurrentContext;

module.exports = function (req, res, next) {

  ns.bindEmitter(req);
  ns.bindEmitter(res);
  ns.run(function(){
    ns.set('ctx', origGetCurrentContext());
    next();
  });

};

We have all auto-collection enabled:

appInsights.setup(config.appInsightsKey)
  .start();

We use bluebird promises heavily, which may affect how the underlying zones function.

@OsvaldoRosado
Copy link
Member

OsvaldoRosado commented Aug 15, 2017

The general guidance here is to use appInsights.wrapWithCorrelationContext(fn) when you find libraries that are breaking the underlying Zones propagation. You pass this method a function fn and it returns a new function you can call in its place. Whenever the function is called, the context available at the time of calling wrapWithCorrelationContext will be restored within it.

For this particular case (bluebird) you can also choose to patch zone.js directly with bluebird support: https://github.com/angular/zone.js/blob/master/NON-STANDARD-APIS.md#user-content-currently-supported-non-standard-common-apis

@tirish
Copy link
Author

tirish commented Aug 15, 2017

When attempting to apply the patch for bluebird like so:

appInsights.setup(config.appInsightsKey)
  .start();

var Bluebird = require('bluebird');
require('zone.js/dist/zone-bluebird');
Zone[Zone['__symbol__']('bluebird')] (Bluebird);

I am getting the exception: Zone.__load_patch is not a function. Note I had to do it after starting app insights, as calling require('zone.js') multiple times causes an exception.

@OsvaldoRosado
Copy link
Member

This looks to me like you're seeing the effects of multiple versions of zone.js being loaded. Notably the version of zone.js currently depended on in this SDK does not include the bluebird patch files or the __load_patch method it uses. This makes me suspect you've npm installed another version of zone.js that's incompatible. We should perhaps switch zone.js to peerDependency so this isn't possible

@OsvaldoRosado
Copy link
Member

OsvaldoRosado commented Aug 15, 2017

Since we depend on an older version of Zone.js, I think the only option forward is to use appInsights.wrapWithCorrelationContext

@tirish
Copy link
Author

tirish commented Aug 15, 2017

Simply wrapping the middleware next did not make a difference. Having to wrap every single top-level promise would be a significant lift.

@markwolff
Copy link
Contributor

Closing this as Zone.js has been deprecated in favor of cls-hooked, so this is likely no longer an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants