-
Notifications
You must be signed in to change notification settings - Fork 155
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
Propagate additional trace data into AWS requests on Lambda #549
Conversation
.gitignore
Outdated
@@ -5,3 +5,4 @@ docs | |||
.nyc_output/ | |||
*.lcov | |||
dist | |||
*package-lock.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we need the package-lock.json
file to keep track of the versions of the packages we use, and it is also one of the files that is modified when we are preparing for a new release. Is there a reason for adding it to .gitignore
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I thought it was auto-generated and need not be checked in. I will remove it from .gitignore
.
packages/core/lib/utils.js
Outdated
@@ -162,33 +162,37 @@ var utils = { | |||
*/ | |||
populateTraceData: function(segment, xAmznTraceId) { | |||
logger.getLogger().debug('Lambda trace data found: ' + xAmznTraceId); | |||
var data = utils.processTraceData(xAmznTraceId); | |||
var traceData = utils.processTraceData(xAmznTraceId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use let
instead of var
packages/core/lib/utils.js
Outdated
var key = pair[0].trim(); | ||
var value = pair[1].trim(); | ||
var lowerCaseKey = key.toLowerCase(); | ||
var reserved = reservedKeywords.indexOf(lowerCaseKey) !== -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to create a new lowerCaseKey
variable instead of converting the key & value to lowercase using .toLowerCase
?
nit: use let
instead of var
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to mutate the casing of additional trace data since it may contain a case-sensitive key. So I created a separate variable lowerCaseKey
from key
as the former is used in multiple places below for existing logic.
Also, I don't think we need to change the casing of the value anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR allows the SDK to parse any additional key-value pair supplied in the X-Ray trace header. And then these additional key-value pairs are injected into instrumented AWS requests made from a Lambda function.
The purpose of this PR is similar to what I did for the Python SDK. The difference there was that the segment already had functionality to save the original trace header. Here, I added a new property on the segment
additionalTraceData
which stores only the additional key-value pairs instead of the complete trace header.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.