-
Notifications
You must be signed in to change notification settings - Fork 60
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: Instrument Lambda invocations in AWS SDK #2784
base: main
Are you sure you want to change the base?
Changes from 10 commits
4434a8c
5475b49
3d213e6
7c33292
c15328c
8728cff
cdb0bf3
10a5e4b
71304dc
db2202c
a14adbb
28a5ebe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
// Copyright 2020 New Relic, Inc. All rights reserved. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text.RegularExpressions; | ||
using NewRelic.Agent.Api; | ||
|
||
namespace NewRelic.Agent.Extensions.Helpers | ||
{ | ||
public static class AwsSdkHelpers | ||
{ | ||
private static Regex RegionRegex = new Regex(@"^[a-z]{2}((-gov)|(-iso([a-z]?)))?-[a-z]+-\d{1}$", RegexOptions.Compiled); | ||
private static bool LooksLikeARegion(string text) => RegionRegex.IsMatch(text); | ||
private static bool LooksLikeAnAccountId(string text) => (text.Length == 12) && text.All(c => c >= '0' && c <= '9'); | ||
// Only log ARNs we can't parse once | ||
private static HashSet<string> BadInvocations = new HashSet<string>(); | ||
|
||
// This is the full regex pattern for an ARN: | ||
// (arn:(aws[a-zA-Z-]*)?:lambda:)?([a-z]{2}((-gov)|(-iso([a-z]?)))?-[a-z]+-\d{1}:)?(\d{12}:)?(function:)?([a-zA-Z0-9-_\.]+)(:(\$LATEST|[a-zA-Z0-9-_]+))? | ||
|
||
// If it's a full ARN, it has to start with 'arn:' | ||
// A partial ARN can contain up to 5 segments separated by ':' | ||
// 1. Region | ||
// 2. Account ID | ||
// 3. 'function' (fixed string) | ||
// 4. Function name | ||
// 5. Alias or version | ||
// Only the function name is required, the rest are all optional. e.g. you could have region and function name and nothing else | ||
public static string ConstructArn(IAgent agent, string invocationName, string region, string accountId) | ||
{ | ||
if (invocationName.StartsWith("arn:")) | ||
{ | ||
return invocationName; | ||
} | ||
var segments = invocationName.Split(':'); | ||
string functionName = null; | ||
string alias = null; | ||
string fallback = null; | ||
|
||
foreach (var segment in segments) | ||
{ | ||
if (LooksLikeARegion(segment)) | ||
{ | ||
if (string.IsNullOrEmpty(region)) | ||
{ | ||
region = segment; | ||
} | ||
else | ||
{ | ||
fallback = segment; | ||
} | ||
continue; | ||
} | ||
else if (LooksLikeAnAccountId(segment)) | ||
{ | ||
if (string.IsNullOrEmpty(accountId)) | ||
{ | ||
accountId = segment; | ||
} | ||
else | ||
{ | ||
fallback = segment; | ||
} | ||
continue; | ||
} | ||
else if (segment == "function") | ||
{ | ||
continue; | ||
} | ||
else if (functionName == null) | ||
{ | ||
functionName = segment; | ||
} | ||
else if (alias == null) | ||
{ | ||
alias = segment; | ||
} | ||
else | ||
{ | ||
if (BadInvocations.Add(invocationName)) | ||
{ | ||
agent?.Logger.Debug($"Unable to parse function name '{invocationName}'"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment applies to the other places where we use this collection and log a message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would supportability metrics be useful for debugging these types of problems? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
} | ||
return null; | ||
} | ||
} | ||
|
||
if (string.IsNullOrEmpty(functionName)) | ||
{ | ||
if (!string.IsNullOrEmpty(fallback)) | ||
{ | ||
functionName = fallback; | ||
} | ||
else | ||
{ | ||
if (BadInvocations.Add(invocationName)) | ||
{ | ||
agent?.Logger.Debug($"Unable to parse function name '{invocationName}'"); | ||
} | ||
return null; | ||
} | ||
} | ||
|
||
if (string.IsNullOrEmpty(accountId)) | ||
{ | ||
if (BadInvocations.Add(invocationName)) | ||
{ | ||
agent?.Logger.Debug($"Need account ID in order to resolve function '{invocationName}'"); | ||
} | ||
return null; | ||
} | ||
if (string.IsNullOrEmpty(region)) | ||
{ | ||
if (BadInvocations.Add(invocationName)) | ||
{ | ||
agent?.Logger.Debug($"Need region in order to resolve function '{invocationName}'"); | ||
} | ||
return null; | ||
} | ||
if (!string.IsNullOrEmpty(alias)) | ||
{ | ||
return $"arn:aws:lambda:{region}:{accountId}:function:{functionName}:{alias}"; | ||
} | ||
return $"arn:aws:lambda:{region}:{accountId}:function:{functionName}"; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
// Copyright 2020 New Relic, Inc. All rights reserved. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
using System.Collections.Concurrent; | ||
using System; | ||
using System.Threading.Tasks; | ||
using NewRelic.Agent.Api; | ||
using NewRelic.Agent.Api.Experimental; | ||
using NewRelic.Agent.Extensions.Providers.Wrapper; | ||
using NewRelic.Reflection; | ||
using NewRelic.Agent.Extensions.Helpers; | ||
|
||
namespace NewRelic.Providers.Wrapper.AwsSdk | ||
{ | ||
internal static class LambdaInvokeRequestHandler | ||
{ | ||
private static ConcurrentDictionary<Type, Func<object, object>> _getResultFromGenericTask = new(); | ||
private static ConcurrentDictionary<string, string> _arnCache = new ConcurrentDictionary<string, string>(); | ||
|
||
private static object GetTaskResult(object task) | ||
{ | ||
if (((Task)task).IsFaulted) | ||
{ | ||
return null; | ||
} | ||
|
||
var getResponse = _getResultFromGenericTask.GetOrAdd(task.GetType(), t => VisibilityBypasser.Instance.GeneratePropertyAccessor<object>(t, "Result")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the pattern we use elsewhere, though now that I look at it I'm not sure why. I can simplify it. |
||
return getResponse(task); | ||
} | ||
|
||
private static void SetRequestIdIfAvailable(IAgent agent, ITransaction transaction, dynamic response) | ||
{ | ||
try | ||
{ | ||
dynamic metadata = response.ResponseMetadata; | ||
nrcventura marked this conversation as resolved.
Show resolved
Hide resolved
|
||
string requestId = metadata.RequestId; | ||
transaction.AddCloudSdkAttribute("aws.requestId", requestId); | ||
} | ||
catch (Exception e) | ||
{ | ||
agent.Logger.Debug(e, "Unable to get RequestId from response metadata."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible that this could be logged too frequently at the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible, though we don't usually limit error logging in our wrappers. I can make it a one-time thing. |
||
} | ||
} | ||
|
||
public static AfterWrappedMethodDelegate HandleInvokeRequest(InstrumentedMethodCall instrumentedMethodCall, IAgent agent, ITransaction transaction, dynamic request, bool isAsync, string region) | ||
{ | ||
string functionName = request.FunctionName; | ||
string qualifier = request.Qualifier; | ||
if (!string.IsNullOrEmpty(qualifier) && !functionName.EndsWith(qualifier)) | ||
{ | ||
functionName = $"{functionName}:{qualifier}"; | ||
} | ||
string arn; | ||
if (!_arnCache.TryGetValue(functionName, out arn)) | ||
nrcventura marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
arn = AwsSdkHelpers.ConstructArn(agent, functionName, region, ""); | ||
_arnCache.TryAdd(functionName, arn); | ||
} | ||
var segment = transaction.StartTransactionSegment(instrumentedMethodCall.MethodCall, "InvokeRequest"); | ||
nrcventura marked this conversation as resolved.
Show resolved
Hide resolved
|
||
segment.GetExperimentalApi().MakeLeaf(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
transaction.AddCloudSdkAttribute("cloud.platform", "aws_lambda"); | ||
transaction.AddCloudSdkAttribute("aws.operation", "InvokeRequest"); | ||
transaction.AddCloudSdkAttribute("aws.region", region); | ||
|
||
|
||
if (!string.IsNullOrEmpty(arn)) | ||
{ | ||
transaction.AddCloudSdkAttribute("cloud.resource_id", arn); | ||
} | ||
|
||
if (isAsync) | ||
{ | ||
return Delegates.GetAsyncDelegateFor<Task>(agent, segment, true, InvokeTryProcessResponse, TaskContinuationOptions.ExecuteSynchronously); | ||
|
||
void InvokeTryProcessResponse(Task responseTask) | ||
{ | ||
try | ||
{ | ||
if (responseTask.Status == TaskStatus.Faulted) | ||
{ | ||
transaction.NoticeError(responseTask.Exception); | ||
} | ||
SetRequestIdIfAvailable(agent, transaction, GetTaskResult(responseTask)); | ||
} | ||
finally | ||
{ | ||
segment?.End(); | ||
} | ||
} | ||
} | ||
else | ||
{ | ||
return Delegates.GetDelegateFor<object>( | ||
onFailure: ex => segment.End(ex), | ||
onSuccess: response => | ||
{ | ||
SetRequestIdIfAvailable(agent, transaction, response); | ||
segment.End(); | ||
} | ||
); | ||
} | ||
} | ||
} | ||
} |
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 say
name
instead ofkey
?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 was following the pattern in
AddLambdaAttribute
andAddFaasAttribute
but yeah, "name" is probably clearer.