fix #594 memory leaks caused by cls-hooked #595
Conversation
| it('should start in automatic mode by creating the X-Ray namespace', function() { | ||
| assert.equal(ContextUtils.getNamespace().name, 'AWSXRay'); | ||
| assert.notEqual(cls.getNamespace('AWSXRay'), undefined); |
There was a problem hiding this comment.
get namespace will create the namespace if it doesn't exist, so this check was misleading, fixed it by checking the cls namespace directly
| } else { | ||
| cls.createNamespace(NAMESPACE); | ||
| logger.getLogger().debug('Starting the AWS X-Ray SDK in automatic mode (default).'); | ||
| } |
There was a problem hiding this comment.
Is it possible to refactor this change to not rely on a new env variable (AWS_XRAY_MANUAL_MODE)?Possibly using just the cls_mode variable?
There was a problem hiding this comment.
I wish, but this is done on the module load, which doesn't give a chance to the developer to change the cls_mode value.
To do it at a later stage will mean a breaking change. I'm willing to implement that, but I need to know that this is ok with you and how do you want to tackle that change
There was a problem hiding this comment.
Do you mind giving more detail about what the change would be in that case and why/how it would be a breaking change? Ideally, I would want to aim for a solution that fixes this issue out of the box without needing a change from the customer's end.
There was a problem hiding this comment.
Currently, you are enabling automatic mode on module load without allowing user intervention on it (that is what my env variable does). If cls-hooked didn't have the memory leak, that would be fine, but currently one can't avoid it. As such, using the cls_hooked variable is not an option (as it can't be modified by the user at the correct point in time)
Another option is a major breaking change in behavior, in which we would postpone the call to cls.createNamespace(NAMESPACE); to some later point in time, allowing the user to call the enableManualMode first. But that will mean that the cls namespace will be created at a different point in time in the application lifecycle, which is not the app load time. This can have effects on tracked contexts that I can't foresee currently.
To conclude, I don't see a way to fix that without one of:
- Manual user intervention (i.e. my solution)
- Breaking changes to the behavior of the code
If you think of a better way, I will be more than happy to accommodate it in this PR.
Looking forward to hear your thoughts :-)
There was a problem hiding this comment.
Got it, thanks for the detailed information! In that case, I agree with your approach for the fix here :)
packages/core/README.md
Outdated
| @@ -26,6 +26,8 @@ By default, the SDK is in automatic mode. You can flip the mode of the SDK using | |||
|
|
|||
| AWSXRay.enableAutomaticMode(); | |||
|
|
|||
| process.env.AWS_XRAY_MANUAL_MODE | |||
There was a problem hiding this comment.
Nit: Can this line be moved to be directly beneath AWSXRay.enableManualMode() and also include a comment with the link to this PR for context? This will be very helpful to anyone reading the documentation, especially if they are looking for a fix for this memory leak.
There was a problem hiding this comment.
I tried something else, can you suggest a better way to present it?
packages/core/README.md
Outdated
|
|
||
| process.env.AWS_XRAY_MANUAL_MODE = 'true'; |
There was a problem hiding this comment.
Maybe something like this to keep the comment directly above the AWS_XRAY_MANUAL_MODE env variable?
| process.env.AWS_XRAY_MANUAL_MODE = 'true'; | |
| // see https://github.com/aws/aws-xray-sdk-node/pull/595 for details on using this environment variable to prevent memory leaks when using manual mode | |
| process.env.AWS_XRAY_MANUAL_MODE = 'true'; |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #595 +/- ##
==========================================
+ Coverage 83.38% 83.41% +0.02%
==========================================
Files 37 37
Lines 1794 1797 +3
==========================================
+ Hits 1496 1499 +3
Misses 298 298 ☔ View full report in Codecov by Sentry. |
*Issue #594
Description of changes:
Add option to set manual mode from env variable
AWS_XRAY_MANUAL_MODEBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.