feat: Instrument Lambda invocations in AWS SDK #2784
Conversation
src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AwsSdk/LambdaInvokeRequestHandler.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (string.IsNullOrWhiteSpace(name)) | ||
| { | ||
| Log.Debug($"AddCloudSdkAttribute - Unable to set Cloud value on transaction because the key is null/empty"); |
There was a problem hiding this comment.
Should this say name instead of key?
There was a problem hiding this comment.
I was following the pattern in AddLambdaAttribute and AddFaasAttribute but yeah, "name" is probably clearer.
| if (BadInvocations.Add(invocationName)) | ||
| { | ||
| agent?.Logger.Debug($"Unable to parse function name '{invocationName}'"); |
There was a problem hiding this comment.
- It seems like it may be possible to run this code concurrently (two async lambda invocations), and we are not using a thread safe collection. This might cause some problems.
- Since we do not log this at the default log level, we may want to perform a is log level enabled check before storing that invocationName indefinitely.
- Do we need to put a cap on this collection so that it doesn't grow too large? Should we just log the first N times we encounter this problem?
There was a problem hiding this comment.
This comment applies to the other places where we use this collection and log a message.
There was a problem hiding this comment.
Would supportability metrics be useful for debugging these types of problems?
There was a problem hiding this comment.
- Ideally we're never going to hit this, but that is technically possible. I can make it thread safe.
- Sure
- Since it's a Set, they'd have to be invoking a significant number of unique Lambda names, all of which are un-parse-able. That seems pretty unlikely, but a size check is easy enough.
- I've replaced all the logging calls with a helper method.
- I don't think so. This is more for troubleshooting individual cases of "my Lambda calls aren't linking". If we saw a large number of parsing failures, we'd still need to see some concrete examples in order to fix it. AWS provides a regex for what's a valid function identifier, and I've tried to cover all the possibilities in the unit tests, so I'm not expecting many surprises.
There was a problem hiding this comment.
If we don't think this is likely, and we just need to capture a few examples, maybe we don't need a collection at all and just log the first 10 times we get something we can't parse?
src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AwsSdk/LambdaInvokeRequestHandler.cs
Show resolved
Hide resolved
| _arnCache.TryAdd(functionName, arn); | ||
| } | ||
| var segment = transaction.StartTransactionSegment(instrumentedMethodCall.MethodCall, "InvokeRequest"); | ||
| segment.GetExperimentalApi().MakeLeaf(); |
There was a problem hiding this comment.
This will suppress the HttpClient instrumentation, so we will no longer get the distributed tracing headers added, or any of the other external call attributes that were previously collected for these calls (prior to this instrumentation).
There was a problem hiding this comment.
That's correct, and the expected behavior. If we leave in the HttpClient segment, an additional Entity is created that can't be linked to the Lambda itself, because the URI is not unique enough. I take your point about the missing attributes, though, and will double check with the other devs on this initiative..
There was a problem hiding this comment.
Yes, and we need to ensure that we are still generating the expected metrics so that the externals UI will work, and the span.kind is correct for the span that is ultimately generated.
| } | ||
| catch (Exception e) | ||
| { | ||
| agent.Logger.Debug(e, "Unable to get RequestId from response metadata."); |
There was a problem hiding this comment.
Is it possible that this could be logged too frequently at the Debug level?
There was a problem hiding this comment.
It's possible, though we don't usually limit error logging in our wrappers. I can make it a one-time thing.
src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AwsSdk/LambdaInvokeRequestHandler.cs
Show resolved
Hide resolved
| return null; | ||
| } | ||
|
|
||
| var getResponse = _getResultFromGenericTask.GetOrAdd(task.GetType(), t => VisibilityBypasser.Instance.GeneratePropertyAccessor<object>(t, "Result")); |
There was a problem hiding this comment.
If you are using dynamic anyways, do you need to use the concurrent dictionary and visibility bypasser combination here?
There was a problem hiding this comment.
That's the pattern we use elsewhere, though now that I look at it I'm not sure why. I can simplify it.
...edApplications/Common/MultiFunctionApplicationHelpers/MultiFunctionApplicationHelpers.csproj
Outdated
Show resolved
Hide resolved
src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/AwsSdk/LambdaInvokeRequestHandler.cs
Outdated
Show resolved
Hide resolved
|
Looks like we might want to review the CodeCov report - patch coverage is a bit low... |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2784 +/- ##
==========================================
- Coverage 81.31% 81.27% -0.05%
==========================================
Files 460 461 +1
Lines 29239 29358 +119
Branches 3231 3252 +21
==========================================
+ Hits 23777 23861 +84
- Misses 4669 4701 +32
- Partials 793 796 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Instrument calls to Lambdas and Lambda Aliases from the AWS SDK, with span attributes to allow linking the calls to the actual Lambda (specifically the ARN is what's needed). Includes unit and integration tests. We are able to test without a making a successful call to a lambda, so no live testing resources are necessary.
Note that we are deliberately not creating an External segment, as that would result in an extra entity being created.