-
Notifications
You must be signed in to change notification settings - Fork 55
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
Handle sfn->Lambda context injection when Payload is used without $ (case 2) #1462
Conversation
3d17c52
to
dc1cc05
Compare
c2dad9c
to
230328c
Compare
Datadog ReportBranch report: ✅ 0 Failed, 392 Passed, 0 Skipped, 1m 29.12s Total duration (2m 0.74s time saved) |
230328c
to
d14b973
Compare
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.
One minor change but LGTM otherwise, thank you!!
return false | ||
} else { | ||
const payload = step.Parameters.Payload | ||
if (payload['Execution.$'] === '$$.Execution' && payload['State.$'] === '$$.State') { |
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.
Can we also check for StateMachine
in this statement as the current layers require it to be set
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.
Good catch!
dc1cc05
to
65bed16
Compare
d14b973
to
2dc9995
Compare
2dc9995
to
5120374
Compare
What and why?
This PR handles Step Function context injection into Lambda when
Payload
(notPayload.$
) is used in the Lambda execution step.This is case # 2 in this design doc: Fixing Step Function Instrumentation
Sub-cases:
Case 2.1: Already instrumented
Then just print a message.
Case 2.2: Already has Execution or State field
If the payload already has any of
Execution.$
,Execution
,State.$
orState
, e.g.and it's not case 2.1, then print a warning and skip context injection
Case 2.3: no Execution or State field
i.e. it's not Case 2.1 or 2.2, e.g.
then just add the two fields:
Testing
Passed the added tests
Review checklist