-
Notifications
You must be signed in to change notification settings - Fork 534
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
feat(aws-sdk): lambda client instrumentation #916
feat(aws-sdk): lambda client instrumentation #916
Conversation
…vergreenSmartPower/opentelemetry-js-contrib into aws_sdk_lambda_instrumentation
Codecov Report
@@ Coverage Diff @@
## main #916 +/- ##
==========================================
+ Coverage 95.91% 96.05% +0.14%
==========================================
Files 13 24 +11
Lines 856 1394 +538
Branches 178 289 +111
==========================================
+ Hits 821 1339 +518
- Misses 35 55 +20
|
plugins/node/opentelemetry-instrumentation-aws-sdk/package.json
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Show resolved
Hide resolved
|
||
// The length of client context is capped at 3583 bytes of base64 encoded data | ||
// (https://docs.aws.amazon.com/lambda/latest/dg/API_Invoke.html#API_Invoke_RequestSyntax) | ||
if (encodedPayload.length > 3583) { |
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.
If the function is invoked multiple times (for example, traceparent
and tracestate
for W3CTraceContextPropagator
, or the 4 fields of B3MultiPropagator
propagator), it is possible that only part of the context will be injected and some will be missing after the limit is reached.
For SQS / SNS, the inject function is first checking if all context fields can be injected successfully and only then attempts any modifications to the request.
What do you think about using a similar approach here as well?
This will also be more performant, as currently, we decode the base64 string, JSON.parse it, then JSON stringify it and encode it back to base64, again and again for each field.
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 noted that approach, but wasn't sure if there was a precise enough equivalent to the 'will this send it over the limit' question, when we first don't know the length of the key + value (the SQS/SNS equivalent is for max number of keys, the values are free in length, so don't need checking) and we would have to use a bodgy encoded => base64 length mapping formula. I'll give it a try to see if I can get all tests passing but it might need a final check before setting just in case despite best endeavours it still has managed to go over the limit.
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.
What I imagined is something like this (haven't really run it, it's just a scatch):
const injectPropagationContext = (
invocationRequest: InvocationRequest
): InvocationRequest => {
try {
const propagationKeysValues = {};
propagation.inject(context.active(), propagationKeysValues);
const parsedClientContext = invocationRequest.ClientContext
? JSON.parse(
Buffer.from(invocationRequest.ClientContext, 'base64').toString('utf8')
)
: {};
const updatedClientContext = {
...parsedClientContext,
Custom: {
...parsedClientContext.Custom,
...propagationKeysValues,
},
};
const encodedClientContext = Buffer.from(
JSON.stringify(updatedClientContext)
).toString('base64');
// The length of client context is capped at 3583 bytes of base64 encoded data
// (https://docs.aws.amazon.com/lambda/latest/dg/API_Invoke.html#API_Invoke_RequestSyntax)
if (encodedClientContext.length > 3583) {
diag.warn(
'lambda instrumentation: cannot set context propagation on lambda invoke parameters due to ClientContext length limitations.'
);
return invocationRequest;
}
return {
...invocationRequest,
ClientContext: encodedClientContext,
}
} catch(err) {
diag.error(
'lambda instrumentation: failed to set context propagation on ClientContext',
err
);
return invocationRequest;
}
};
This code will either inject the entire propagation data, or none of it. It will also JSON.parse
+ Buffer.from
and JSON.stringify
+ toString('base64')
the client context at most once, regardless of the number of propagation keys.
It will also create a copy of the request params, instead of modifying the user's object which might introduce side effects
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Outdated
Show resolved
Hide resolved
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.
Thanks for addressing everything and for writing all these high-quality tests 🥇
We are almost ready, added a few more minor comments.
@NathanielRN @willarmiros do you want to review as well?
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.
It looks good to me from a high level. I added a few comments, but I'll defer to @blumamir for the final review.
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/doc/lambda.md
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/doc/lambda.md
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-aws-sdk/doc/lambda.md
Outdated
Show resolved
Hide resolved
) { | ||
const operation = response.request.commandName; | ||
|
||
if (operation === 'Invoke') { |
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.
Should this be a switch statement too?
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.
Updated
plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
Outdated
Show resolved
Hide resolved
extractFunctionName = (commandInput: Record<string, any>): string => { | ||
return commandInput?.FunctionName; | ||
}; | ||
} |
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.
Probably a space below this would make sense?
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.
The linter keeps removing that, so unfortunately I can't
Co-authored-by: Nathaniel Ruiz Nowell <[email protected]>
Co-authored-by: Nathaniel Ruiz Nowell <[email protected]>
Co-authored-by: Nathaniel Ruiz Nowell <[email protected]>
…vergreenSmartPower/opentelemetry-js-contrib into aws_sdk_lambda_instrumentation
Which problem is this PR solving?
Implements instrumentation of the AWS SDK 'Lambda' calls (invoke) using FAAS trace semantics - this follows on from discussions on PR #877
Short description of the changes
Checklist
npm run test-all-versions
for the edited package(s) on the latest commit if applicable.