-
Notifications
You must be signed in to change notification settings - Fork 151
[AWS Lambda] Add request-id as header to Lambda start/end invocation #7835
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
Changes from all commits
8303e45
11cb308
6ca3394
adda5d8
09ccbaa
21040c0
85a92b9
b5116dd
9498940
b6a25b2
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 |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ | |
| using System.Net; | ||
| using System.Text; | ||
| using System.Threading.Tasks; | ||
| using Datadog.Trace.ClrProfiler.AutoInstrumentation.AWS.SDK; | ||
| using Datadog.Trace.ClrProfiler.CallTarget; | ||
| using Datadog.Trace.Headers; | ||
| using Datadog.Trace.Propagators; | ||
| using Datadog.Trace.Telemetry; | ||
|
|
@@ -22,6 +24,7 @@ internal abstract class LambdaCommon | |
| private const string PlaceholderOperationName = "placeholder-operation"; | ||
| private const double ServerlessMaxWaitingFlushTime = 3; | ||
| private const string LogLevelEnvName = "DD_LOG_LEVEL"; | ||
| private const string LambdaRuntimeAwsRequestIdHeader = "lambda-runtime-aws-request-id"; | ||
|
|
||
| internal static Scope CreatePlaceholderScope(Tracer tracer, NameValueHeadersCollection headers) | ||
| { | ||
|
|
@@ -38,11 +41,16 @@ internal static Scope CreatePlaceholderScope(Tracer tracer, NameValueHeadersColl | |
| return tracer.TracerManager.ScopeManager.Activate(span, false); | ||
| } | ||
|
|
||
| internal static Scope SendStartInvocation(ILambdaExtensionRequest requestBuilder, string data, IDictionary<string, string> context) | ||
| internal static Scope SendStartInvocation(ILambdaExtensionRequest requestBuilder, string data, ILambdaContext context) | ||
rithikanarayan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| var request = requestBuilder.GetStartInvocationRequest(); | ||
| WriteRequestPayload(request, data); | ||
| WriteRequestHeaders(request, context); | ||
| WriteRequestHeaders(request, context?.ClientContext?.Custom); | ||
|
Member
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 either Does if (context?.ClientContext?.Custom is { } c)
{
WriteRequestHeaders(request, c);
}
Contributor
Author
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. WriteRequestHeaders starts by checking if context is null (if it is null it does nothing). This was how it was structured prior to my changes, so I am choosing to leave it that way. |
||
| if (context?.AwsRequestId != null) | ||
| { | ||
| request.Headers.Add(LambdaRuntimeAwsRequestIdHeader, context.AwsRequestId); | ||
| } | ||
|
|
||
| using var response = (HttpWebResponse)request.GetResponse(); | ||
|
|
||
| var headers = response.Headers.Wrap(); | ||
|
|
@@ -55,9 +63,9 @@ internal static Scope SendStartInvocation(ILambdaExtensionRequest requestBuilder | |
| return CreatePlaceholderScope(tracer, headers); | ||
| } | ||
|
|
||
| internal static void SendEndInvocation(ILambdaExtensionRequest requestBuilder, Scope scope, bool isError, string data) | ||
| internal static void SendEndInvocation(ILambdaExtensionRequest requestBuilder, Scope scope, object state, bool isError, string data) | ||
|
Member
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. In general we should try to avoid using the To avoid that, we should cast the |
||
| { | ||
| var request = requestBuilder.GetEndInvocationRequest(scope, isError); | ||
| var request = requestBuilder.GetEndInvocationRequest(scope, state, isError); | ||
| WriteRequestPayload(request, data); | ||
| using var response = (HttpWebResponse)request.GetResponse(); | ||
|
|
||
|
|
@@ -67,8 +75,10 @@ internal static void SendEndInvocation(ILambdaExtensionRequest requestBuilder, S | |
| } | ||
| } | ||
|
|
||
| internal static async Task EndInvocationAsync(string returnValue, Exception exception, Scope scope, ILambdaExtensionRequest requestBuilder) | ||
| internal static async Task EndInvocationAsync(string returnValue, Exception exception, object stateObject, ILambdaExtensionRequest requestBuilder) | ||
|
Member
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. Same comment here about the using |
||
| { | ||
| var state = (CallTargetState)stateObject!; | ||
|
Member
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 is a dangerous cast that will throw an exception if if (state is CallTargetState)
{
...Even better would be to use |
||
| var scope = state.Scope; | ||
| try | ||
| { | ||
| await Task.WhenAll( | ||
|
|
@@ -90,7 +100,7 @@ await Task.WhenAll( | |
| span.SetException(exception); | ||
| } | ||
|
|
||
| SendEndInvocation(requestBuilder, scope, exception != null, returnValue); | ||
| SendEndInvocation(requestBuilder, scope, state.State, exception != null, returnValue); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,12 +35,17 @@ WebRequest ILambdaExtensionRequest.GetStartInvocationRequest() | |
| return request; | ||
| } | ||
|
|
||
| WebRequest ILambdaExtensionRequest.GetEndInvocationRequest(Scope scope, bool isError) | ||
| WebRequest ILambdaExtensionRequest.GetEndInvocationRequest(Scope scope, object state, bool isError) | ||
|
Member
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. Here's a good example of why not to not use Or if someone mistakenly checks |
||
| { | ||
| var request = WebRequest.Create(Uri + EndInvocationPath); | ||
| request.Method = "POST"; | ||
| request.Headers.Set(HttpHeaderNames.TracingEnabled, "false"); | ||
|
|
||
| if (state != null) | ||
| { | ||
| request.Headers.Set("lambda-runtime-aws-request-id", (string)state); | ||
| } | ||
|
Comment on lines
+44
to
+47
Member
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. Although it should never happen, if The more modern way of doing this in C# is by combining the type check with a if (state is string requestId)
{
request.Headers.Set("lambda-runtime-aws-request-id", requestId);
} |
||
|
|
||
| if (scope is { Span: var span }) | ||
| { | ||
| // TODO: add support for 128-bit trace ids in serverless | ||
|
|
||
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.
[naming conventions]
We should use
requestIdinstead ofrequestid(this is the nit-pickiest comment in this PR, I promise)[type safety]
requestidcan bestringsinceILambdaContext.AwsRequestIdis declared as string. We want to avoid usingobjectas much as possible.